bugfix and refactor device increate count #8782

Apokleos · 2024-01-08T09:43:00Z

runtime-rs: bugfix incorrect use of refcount before vfio attach

When there's a pod with multiple containers, there may be case that
attach point more than 2, we should not return Err in that case when
we are doing attach ops, but just return Ok.

runtime-rs: introduce dedicated function do_increase_count/do_decrease_count

Since there are many implementations of reference counting in the
drivers, all of which have the same implementation, we should try
to reduce such duplicated code as much as possible. Therefore, a
new function is introduced to solve the problem of duplicated code.

runtime-rs: refactor increase_attach_count with do_increase_count/do_decrease_count

Try to reduce duplicated code in increase_attach_count with public
new function do_increase_count/do_decrease_count.

Fixes: #8738

justxuewei

Thanks! A few comments here.

justxuewei · 2024-01-08T09:47:12Z

src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs

-            return Err(anyhow!("attach count increased failed as some reason."));
+            warn!(
+                sl!(),
+                "It's not allowed that device {:?} is attached many times,", self.device_id


I think it is more concise.

Suggested change

"It's not allowed that device {:?} is attached many times,", self.device_id

"The device {:?} is not allowed to be attached multiple times.", self.device_id

justxuewei · 2024-01-08T09:49:47Z

src/runtime-rs/crates/hypervisor/src/device/util.rs

+        0 => {
+            // When ref_count is 0, it indicates that the device is new here, In this case, we know that the device
+            // has not been attached into the Guest.
+            *ref_count += 1;
+            Ok(false)
+        }
+        // While in practice, the number of times a device is attempted to be inserted into the Guest cannot reach
+        // this maximum value, the device is theoretically allowed to do so.
+        // However, we will not allow the device to actually be inserted into the Guest more than once.
+        std::u64::MAX => Err(anyhow!("device was attached too many times")),
+        _ => {
+            // When ref_count is greater than 0, it indicates that how many times the device has been attempted to
+            // be inserted into the Guest.
+            // In this case, we know the device has been attached into the Guest.
+            *ref_count += 1;
+            Ok(true)
+        }


How about using checked_add()? FYI, https://doc.rust-lang.org/std/primitive.u32.html#method.checked_add.

Thx @justxuewei Although I know that the checked_add() method is a good choice in some cases, I don't think it's appropriate here. And I don't want to introduce too much judgment logic because of this method, so I choose to stick to the original approach.

Another reason is, the code are just moved here from its original places, and the key point is, simple and it works well.

@justxuewei Maybe saturating_add(1)/saturating_sub(1) is a good choice !

However, It's indeed better to refactor it with check_add as @justxuewei said:

checked_add(1).ok_or(anyhow!("error msg")?; return Ok(*ref_count != 1))

justxuewei · 2024-01-08T09:50:02Z

src/runtime-rs/crates/hypervisor/src/device/util.rs

+}
+
+// do_decrease_count
+pub fn do_decrease_count(ref_count: &mut u64) -> Result<bool> {


justxuewei · 2024-01-08T09:54:15Z

src/runtime-rs/crates/hypervisor/src/device/util.rs

@@ -69,9 +69,55 @@ pub(crate) fn get_virt_drive_name(mut index: i32) -> Result<String> {
    Ok(String::from(PREFIX) + std::str::from_utf8(&disk_letters)?)
 }

+// do_increase_count


Remove it or expand on this, as this comment doesn't provide any valuable information.

justxuewei · 2024-01-08T09:54:23Z

src/runtime-rs/crates/hypervisor/src/device/util.rs

+    }
+}
+
+// do_decrease_count


When there's a pod with multiple containers, there may be case that attach point more than 2, we should not return Err in that case when we are doing attach ops, but just return Ok. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Apokleos · 2024-04-02T06:44:21Z

/test

justxuewei

Lgtm, thanks! @Apokleos

studychao

Thanks, a few comments.

src/runtime-rs/crates/hypervisor/src/device/util.rs

src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs

src/runtime-rs/crates/hypervisor/src/device/util.rs

Since there are many implementations of reference counting in the drivers, all of which have the same implementation, we should try to reduce such duplicated code as much as possible. Therefore, a new function is introduced to solve the problem of duplicated code. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Introduce a dedicated public function do_decrease_count to reduce duplicated code in drivers' decrease_attach_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Try to reduce duplicated code in increase_attach_count with public new function do_increase_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Try to reduce duplicated code in decrease_attach_count with public new function do_decrease_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

studychao

LGTM

Apokleos · 2024-04-03T11:59:09Z

/test

Apokleos changed the title ~~Device increate count~~ bugfix and refactor device increate count Jan 8, 2024

katacontainersbot added the size/large Task of significant size label Jan 8, 2024

Apokleos requested review from studychao and Tim-0731-Hzt January 8, 2024 09:43

Apokleos self-assigned this Jan 8, 2024

Apokleos added ok-to-test no-backport-needed labels Jan 8, 2024

justxuewei reviewed Jan 8, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/device/util.rs Outdated

}

}

// do_decrease_count

Copy link

Member

justxuewei Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Apokleos force-pushed the device-increate-count branch from c38c8ac to be20973 Compare February 4, 2024 11:44

Apokleos added ok-to-test and removed ok-to-test labels Feb 5, 2024

Apokleos force-pushed the device-increate-count branch 2 times, most recently from dc81a77 to 7b7153a Compare April 1, 2024 03:23

Apokleos force-pushed the device-increate-count branch 2 times, most recently from 37b4796 to 25ed1c2 Compare April 1, 2024 08:06

Apokleos requested a review from Tim-Zhang April 1, 2024 09:40

justxuewei approved these changes Apr 2, 2024

View reviewed changes

Apokleos requested a review from quanweiZhou April 3, 2024 00:51

studychao reviewed Apr 3, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/device/util.rs Show resolved Hide resolved

src/runtime-rs/crates/hypervisor/src/device/util.rs Show resolved Hide resolved

src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs Show resolved Hide resolved

studychao reviewed Apr 3, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/device/util.rs Outdated Show resolved Hide resolved

Apokleos force-pushed the device-increate-count branch from 25ed1c2 to 9b8b5be Compare April 3, 2024 07:03

Apokleos added 4 commits April 3, 2024 17:09

runtime-rs: introduce dedicated function do_decrease_count

fff64f1

Introduce a dedicated public function do_decrease_count to reduce duplicated code in drivers' decrease_attach_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

runtime-rs: refactor increase_attach_count with do_increase_count

4f0fab9

Try to reduce duplicated code in increase_attach_count with public new function do_increase_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

runtime-rs: refactor decrease_attach_count with do_decrease_count

935a1a3

Try to reduce duplicated code in decrease_attach_count with public new function do_decrease_count. Fixes: kata-containers#8738 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Apokleos force-pushed the device-increate-count branch from 9b8b5be to 935a1a3 Compare April 3, 2024 09:19

studychao approved these changes Apr 3, 2024

View reviewed changes

Apokleos merged commit 0e0a361 into kata-containers:main Apr 5, 2024
289 of 299 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix and refactor device increate count #8782

bugfix and refactor device increate count #8782

Apokleos commented Jan 8, 2024

justxuewei left a comment

justxuewei Jan 8, 2024

justxuewei Jan 8, 2024

Apokleos Feb 4, 2024

Apokleos Feb 6, 2024

Apokleos Feb 6, 2024

justxuewei Jan 8, 2024

justxuewei Jan 8, 2024

justxuewei Jan 8, 2024

Apokleos commented Apr 2, 2024

justxuewei left a comment

studychao left a comment

studychao left a comment

Apokleos commented Apr 3, 2024

	"It's not allowed that device {:?} is attached many times,", self.device_id
	"The device {:?} is not allowed to be attached multiple times.", self.device_id

bugfix and refactor device increate count #8782

bugfix and refactor device increate count #8782

Conversation

Apokleos commented Jan 8, 2024

justxuewei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apokleos commented Apr 2, 2024

justxuewei left a comment

Choose a reason for hiding this comment

studychao left a comment

Choose a reason for hiding this comment

studychao left a comment

Choose a reason for hiding this comment

Apokleos commented Apr 3, 2024