WIP: Use `readv` for the RX path of the network device to avoid one memory copy per frame #4799

bchalios · 2024-09-12T14:21:56Z

This is still WIP. I will update the description before the PR is ready for reviewing.

Changes

...

Reason

...

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this
PR.
API changes follow the Runbook for Firecracker API changes.
User-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.
New TODOs link to an issue.
Commits meet
contribution quality standards.

This functionality cannot be added in rust-vmm.

codecov · 2024-09-12T15:12:33Z

Codecov Report

Attention: Patch coverage is 88.26531% with 23 lines in your changes missing coverage. Please review.

Project coverage is 84.33%. Comparing base (8eea9df) to head (f98af8a).

❗ Current head f98af8a differs from pull request most recent head 424b001

Please upload reports for the commit 424b001 to get more accurate results.

Files with missing lines	Patch %	Lines
src/vmm/src/devices/virtio/iovec.rs	69.49%	18 Missing ⚠️
src/vmm/src/devices/virtio/iov_deque.rs	96.82%	4 Missing ⚠️
src/vmm/src/devices/virtio/vsock/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4799      +/-   ##
==========================================
+ Coverage   84.32%   84.33%   +0.01%     
==========================================
  Files         249      250       +1     
  Lines       27521    27693     +172     
==========================================
+ Hits        23206    23355     +149     
- Misses       4315     4338      +23

Flag	Coverage Δ
5.10-c5n.metal	`84.55% <88.26%> (+0.01%)`	⬆️
5.10-m5n.metal	`84.53% <88.26%> (+<0.01%)`	⬆️
5.10-m6a.metal	`83.83% <88.26%> (+0.01%)`	⬆️
5.10-m6g.metal	`80.94% <88.26%> (+0.04%)`	⬆️
5.10-m6i.metal	`84.53% <88.26%> (+<0.01%)`	⬆️
5.10-m7g.metal	`80.94% <88.26%> (+0.04%)`	⬆️
6.1-c5n.metal	`84.55% <88.26%> (+0.01%)`	⬆️
6.1-m5n.metal	`84.53% <88.26%> (+<0.01%)`	⬆️
6.1-m6a.metal	`83.83% <88.26%> (+0.02%)`	⬆️
6.1-m6g.metal	`80.94% <88.26%> (+0.04%)`	⬆️
6.1-m6i.metal	`84.53% <88.26%> (+0.01%)`	⬆️
6.1-m7g.metal	`80.94% <88.26%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add a ring buffer type that is tailored for holding `struct iovec` objects that point to guest memory for IO. The `struct iovec` objects represent the memory that the guest passed to us as `Descriptors` in a VirtIO queue for performing some I/O operation. We plan to use this type to describe the guest memory we have available for doing network RX. This should facilitate us in optimizing the reception of data from the TAP device using `readv`, thus avoiding a memory copy. Co-authored-by: Egor Lazarchuk <[email protected]> Signed-off-by: Babis Chalios <[email protected]>

Allow IoVecBufferMut objects to store multiple DescriptorChain objects, so that we can describe guest memory meant to be used for receiving data (for example memory used for network RX) as a single (sparse) memory region. This will allow us to always keep track all the available memory we have for performing RX and use `readv` for copying memory from the TAP device inside guest memory avoiding the extra copy. In the future, it will also facilitate the implementation of mergeable buffers for the RX path of the network device. Signed-off-by: Babis Chalios <[email protected]>

Right now, we are performing two copies for writing a frame from the TAP device into guest memory. We first read the frame in an array held by the Net device and then copy that array in a DescriptorChain. In order to avoid the double copy use the readv system call to read directly from the TAP device into the buffers described by DescriptorChain. The main challenge with this is that DescriptorChain objects describe memory that is at least 65562 bytes long when guest TSO4, TSO6 or UFO are enabled or 1526 otherwise and parsing the chain includes overhead which we pay even if the frame we are receiving is much smaller than these sizes. PR firecracker-microvm#4748 reduced the overheads involved with parsing DescriptorChain objects. To further avoid this overhead, move the parsing of DescriptorChain objects out of the hot path of process_rx() where we are actually receiving a frame into process_rx_queue_event() where we get the notification that the guest added new buffers for network RX. Signed-off-by: Babis Chalios <[email protected]>

roypat · 2024-09-13T09:12:26Z

src/vmm/src/devices/virtio/iov_deque.rs

+        // SAFETY: We are calling the system call with valid arguments and properly checking its
+        // return value


To be able to claim that we are calling the syscall with valid arguments, this function should be unsafe, with the invariant being "arguments are valid for mmap"

roypat · 2024-09-13T09:20:19Z

src/vmm/src/devices/virtio/iov_deque.rs

+        // SAFETY:
+        // * `buffer` is valid both for reads and writes (allocated with `libc::PROT_READ |
+        //   libc::PROT_WRITE`. `
+        // * `buffer` is aligned at `PAGE_SIZE`
+        // * `buffer` points to memory allocated with a single system call to `libc::mmap`
+        let iov = unsafe {
+            std::slice::from_raw_parts_mut(
+                buffer.cast(),
+                2 * PAGE_SIZE / std::mem::size_of::<libc::iovec>(),
+            )
+        };


Mhhh, I don't think we can do this. The two pages contain the same physical memory, so we're creating aliasing mutable references here. But if we just store everything as *mut libc::iovec, I think it'll be fine :)

And if we use pointers, we also get rid of the lifetime parameter (which is kinda wrong to have anyway, because the iov field is self referential, and that's not something rust's lifetimes can express)

roypat · 2024-09-13T09:22:58Z

src/vmm/src/devices/virtio/iov_deque.rs

+        self.head += 1;
+        if self.head > usize::from(FIRECRACKER_MAX_QUEUE_SIZE) {
+            self.head -= usize::from(FIRECRACKER_MAX_QUEUE_SIZE);
+            self.tail -= usize::from(FIRECRACKER_MAX_QUEUE_SIZE);
+        }


I wonder if we could do the trick that the virtio spec does here, and simply store these indices as Wrapping<usize>, let them count up indefinitely, and just do modulo when we want to access an element? Although I also don't really see why we would want to do that 😂

roypat · 2024-09-13T09:25:16Z

src/vmm/src/devices/virtio/iov_deque.rs

+
+    /// Drop the first `iovec` objects that describe guest memory of `size` length.
+    ///
+    /// This will drop `iovec` buffers which describe a total memory size of up to `size`. This


Suggested change

/// This will drop `iovec` buffers which describe a total memory size of up to `size`. This

/// This will drop `iovec` buffers which describe a total memory size of at least `size`. This

no? Because if size == 1 and the first iovec contains 10 bytes, then it drop iovecs describing 10 bytes

roypat · 2024-09-13T09:25:37Z

src/vmm/src/devices/virtio/iov_deque.rs

+    pub(crate) fn as_mut_slice(&mut self) -> &mut [iovec] {
+        &mut self.iov[self.head..self.tail]
+    }


Also need to work with pointers here

roypat · 2024-09-13T09:27:33Z

src/vmm/src/devices/virtio/iovec.rs

+    pub fn drop_iovecs(&mut self, size: u32) -> Result<(), IoVecError> {
+        let dropped = self.vecs.drop_iovs(size as usize);
+
+        // Users should ask us to drop a `size` of memory that is not exactly covered by `iovec`


nit: s/should/should not ?

roypat · 2024-09-13T09:28:39Z

src/vmm/src/devices/virtio/iovec.rs

+        // objects. In other words, the sum of the lengths of all dropped `iovec` objects should be
+        // equal to the `size` we were asked to drop. If it isn't, something is seriously wrong
+        // with the VirtIO queue or the emulation logic, so fail at this point.
+        assert_eq!(u32::try_from(dropped).unwrap(), size);


Suggested change

assert_eq!(u32::try_from(dropped).unwrap(), size);

assert_eq!(dropped, usize::from(size));

do we really want to panic here though? The function returns Result, so could we just return an Err?

roypat · 2024-09-13T09:35:42Z

src/vmm/src/devices/virtio/rng/device.rs

        rand::fill(&mut rand_bytes).map_err(|err| {
            METRICS.host_rng_fails.inc();
            err
        })?;

        // It is ok to unwrap here. We are writing `iovec.len()` bytes at offset 0.
        iovec.write_all_volatile_at(&rand_bytes, 0).unwrap();
-        Ok(iovec.len())
+        Ok(u32::try_from(iovec.len()).unwrap())


this makes me sad :(

I don't think its strictly necessary, but it'd be cool to differentiate between the new "multi descriptor chain" iovecbuffermuts and the old "one descriptor chain" ones. We could add a L: Add type parameter that is either u32 or usize and specifies the length of the IoVecBufferMut. Then only the impl IoVecBufferMut<usize> block would have all the appending stuff, and in the rng device and everywhere else can continue using IoVecBufferMut<u32> without these ugly try_froms.

I mostly agree with this. In my head thought it would be nicer to just leave IoVecBufferMut alone and use IovDequeue standalone. The specialization of IoVecBufferMut feels like C++ hell, let's not go that rout.

roypat · 2024-09-13T09:35:55Z

src/vmm/src/devices/virtio/rng/device.rs

-                    if !Self::rate_limit_request(&mut self.rate_limiter, u64::from(iovec.len())) {
+                    if !Self::rate_limit_request(
+                        &mut self.rate_limiter,
+                        u64::try_from(iovec.len()).unwrap(),


usize_to_u64 here

ShadowCurse · 2024-09-16T10:09:57Z

src/vmm/src/devices/virtio/iov_deque.rs

+#[derive(Debug)]
+pub(crate) struct IovDeque<'a> {
+    iov: &'a mut [libc::iovec],
+    _memfd: Memfd,


We don't need to store memfd. After mmap is done, it is safe to drop memfd:

After the mmap() call has returned, the file descriptor, fd, can be closed immediately without invalidating the mapping.

ShadowCurse · 2024-09-16T10:17:36Z

src/vmm/src/devices/virtio/iov_deque.rs

+    }
+
+    /// Create a new [`IovDeque`] that can hold memory described by a single VirtIO queue.
+    pub(crate) fn new() -> Result<Self, IovDequeError> {


Can we remove (crate) thing? It is useless because this type only used in vmm crate anyways.

ShadowCurse · 2024-09-16T10:54:11Z

src/vmm/src/devices/virtio/rng/device.rs

        rand::fill(&mut rand_bytes).map_err(|err| {
            METRICS.host_rng_fails.inc();
            err
        })?;

        // It is ok to unwrap here. We are writing `iovec.len()` bytes at offset 0.
        iovec.write_all_volatile_at(&rand_bytes, 0).unwrap();
-        Ok(iovec.len())
+        Ok(u32::try_from(iovec.len()).unwrap())


I mostly agree with this. In my head thought it would be nicer to just leave IoVecBufferMut alone and use IovDequeue standalone. The specialization of IoVecBufferMut feels like C++ hell, let's not go that rout.

ShadowCurse · 2024-09-16T10:56:08Z

src/vmm/src/devices/virtio/iovec.rs

+    /// Drop memory from the `IoVecBufferMut`
+    ///
+    /// This will drop memory described by the `IoVecBufferMut` starting from the beginning.


We don't really drop memory, just some iovs.

/// Drop iovecs from the beginning of the `IoVecBufferMut` ...

ShadowCurse · 2024-09-16T11:05:52Z

src/vmm/src/devices/virtio/net/device.rs

@@ -510,6 +495,10 @@ impl Net {

    // We currently prioritize packets from the MMDS over regular network packets.
    fn read_from_mmds_or_tap(&mut self) -> Result<usize, NetError> {
+        if self.rx_buffer.len() == 0 {


Shouldn't clippy complain about not having is_empty method?

bchalios force-pushed the net_rx_readv branch 3 times, most recently from 266551b to f98af8a Compare September 12, 2024 15:01

bchalios force-pushed the net_rx_readv branch 2 times, most recently from 1d90b83 to da21f71 Compare September 12, 2024 22:15

bchalios force-pushed the net_rx_readv branch from da21f71 to 23551b8 Compare September 13, 2024 08:03

bchalios added 2 commits September 13, 2024 10:08

bchalios force-pushed the net_rx_readv branch from 23551b8 to 424b001 Compare September 13, 2024 08:17

roypat reviewed Sep 13, 2024

View reviewed changes

ShadowCurse reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Use `readv` for the RX path of the network device to avoid one memory copy per frame #4799

WIP: Use `readv` for the RX path of the network device to avoid one memory copy per frame #4799

bchalios commented Sep 12, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

roypat Sep 13, 2024

ShadowCurse Sep 16, 2024

roypat Sep 13, 2024

ShadowCurse Sep 16, 2024

ShadowCurse Sep 16, 2024

ShadowCurse Sep 16, 2024

ShadowCurse Sep 16, 2024

ShadowCurse Sep 16, 2024

		// SAFETY: We are calling the system call with valid arguments and properly checking its
		// return value

	/// This will drop `iovec` buffers which describe a total memory size of up to `size`. This
	/// This will drop `iovec` buffers which describe a total memory size of at least `size`. This

	assert_eq!(u32::try_from(dropped).unwrap(), size);
	assert_eq!(dropped, usize::from(size));

WIP: Use readv for the RX path of the network device to avoid one memory copy per frame #4799

Are you sure you want to change the base?

WIP: Use readv for the RX path of the network device to avoid one memory copy per frame #4799

Conversation

bchalios commented Sep 12, 2024

Changes

Reason

License Acceptance

PR Checklist

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

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

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

WIP: Use `readv` for the RX path of the network device to avoid one memory copy per frame #4799

WIP: Use `readv` for the RX path of the network device to avoid one memory copy per frame #4799

codecov bot commented Sep 12, 2024 •

edited

Loading