-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reuse the IoVecBuffer on TX #4589
base: main
Are you sure you want to change the base?
Conversation
On the net virtio device reuse the IoVecBuffer on the TX path Signed-off-by: Jack Thomson <jackabt@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments regarding unnecessary SAFETY
comments. Other than that LGTM
/// | ||
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that useful here?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I think these might be left-overs from when new
was unsafe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above :)
Changes
Removes the allocation of the
IoVecBuffer
in 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
.TODO
s link to an issue.contribution quality standards.
rust-vmm
.