Reuse the IoVecBuffer on TX #4589
Conversation
src/vmm/src/devices/virtio/iovec.rs
Outdated
| /// | ||
| /// The descriptor chain cannot be referencing the same memory location as another chain | ||
| pub unsafe fn from_descriptor_chain(head: DescriptorChain) -> Result<Self, IoVecError> { | ||
| // SAFETY: New buffer is created from the DescriptorChain which doesnt implement clone |
| @@ -197,6 199,8 @@ impl Net { | |||
| activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(NetError::EventFd)?, | |||
| mmds_ns: None, | |||
| metrics: NetMetricsPerDevice::alloc(id), | |||
| // SAFETY: Only constructed in the VMM thread so no concurrent buffers | |||
There was a problem hiding this comment.
I don't think we need to have a SAFETY comment for this. calling IoVecBuffer::default() is not unsafe
There was a problem hiding this comment.
Yup, I think these might be left-overs from when new was unsafe?
There was a problem hiding this comment.
Ah yes must of been left over my bad
| @@ -197,6 199,8 @@ impl Net { | |||
| activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(NetError::EventFd)?, | |||
| mmds_ns: None, | |||
| metrics: NetMetricsPerDevice::alloc(id), | |||
| // SAFETY: Only constructed in the VMM thread so no concurrent buffers | |||
There was a problem hiding this comment.
Yup, I think these might be left-overs from when new was unsafe?
| continue; | ||
| } | ||
| }; | ||
| // SAFETY: This descriptor chain is only loaded into this buffer |
There was a problem hiding this comment.
Let's elaborate on these - virtio requests are handled sequentially, so no two iovecbuffers are ever "live" at the same time, meaning this one really has exclusive ownership over the memory (well, from rust's side.. the guest can do as it pleases)
There was a problem hiding this comment.
We should clear() this buffer as soon as we're done using it (e.g. at the end of this function)
There was a problem hiding this comment.
Sounds great thanks I'll update the comments and clear at the end thanks
| @@ -124,7 124,8 @@ impl VsockPacket { | |||
| /// - [`VsockError::DescChainTooShortForPacket`] if the contained vsock header describes a vsock | |||
| /// packet whose length exceeds the descriptor chain's actual total buffer length. | |||
| pub fn from_tx_virtq_head(chain: DescriptorChain) -> Result<Self, VsockError> { | |||
| let buffer = IoVecBuffer::from_descriptor_chain(chain)?; | |||
| // SAFETY: chain is only loaded into a single buffer | |||
roypat
added
the
Status: Awaiting author
On the net virtio device reuse the IoVecBuffer on the TX path Signed-off-by: Jack Thomson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4589 /- ##
=======================================
Coverage 82.12% 82.13%
=======================================
Files 255 255
Lines 31281 31291 10
=======================================
Hits 25689 25700 11
Misses 5592 5591 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changes
Removes the allocation of the
IoVecBufferin the virtio net device during tx by reusing the same instance for each message.To do this we have to implement Send on the type for it to work, I have tried to cover the unsafe sections the best I can.
Reason
Reduce the number of allocation
Ticket here: #4549
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
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.