Reuse the IoVecBuffer on TX #4589
Conversation
On the net virtio device reuse the IoVecBuffer on the TX path Signed-off-by: Jack Thomson <jackabt@amazon.com>
| /// | ||
| /// 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
| @@ -35,19 +35,27 @@ type IoVecVec = SmallVec<[iovec; 4]>; | |||
| /// It describes a buffer passed to us by the guest that is scattered across multiple | |||
| /// memory regions. Additionally, this wrapper provides methods that allow reading arbitrary ranges | |||
| /// of data from that buffer. | |||
| #[derive(Debug)] | |||
| #[derive(Debug, Default)] | |||
| pub struct IoVecBuffer { | |||
| // container of the memory regions included in this IO vector | |||
| vecs: IoVecVec, | |||
There was a problem hiding this comment.
Mh, now that we're reusing iovec buffers, I was hoping that the IoVecVec/smallvec stuff won't be needed anymore. Can we revert e5a5d0e on top of this and run some performance tests to validate that hope? It'd simplify the code nicely
There was a problem hiding this comment.
Oh sure I can give that a try that would be nice.
| @@ -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 | |||
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.