-
Notifications
You must be signed in to change notification settings - Fork 113
refactor(drivers/net): Unify MTUs and fix buffer sizes for virtio-net #1714
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
Conversation
src/drivers/net/virtio/mod.rs
Outdated
| { | ||
| dev_cfg.raw.as_ptr().mtu().read().to_ne().into() | ||
| } else { | ||
| 1514 |
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 think this branch will cause an error when we have MTU negotiated.
If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive buffers to receive at least one receive packet of size mtu (plus low level ethernet header length) with gso_type NONE or ECN.
I don't know what MTU the device reports if guest segmentation offload is negotiated, but I think the safe behaviour is to use a constant 65550 (not the mtu) as the buffer size when it is negotiated, and if not, to use the MTU.
|
By the way, the way we handle MTUs in general is wrong. At the point the RxQueues are created, the feature negotiation is not complete and we don't know if the device supports MTU reporting, but we try to read that field here. I have to rework the driver initialisation to avoid that. |
12da4b5 to
9d489c3
Compare
I hope I got it right now. I refactored the driver initialization to only initialize the virtqueues after the feature negotiation is complete. It probably needs some more polishing, but it looks right now. I realized that if we actually use the negotiated features, I can see that |
5162471 to
de00c3f
Compare
de00c3f to
f4f55bc
Compare
f4f55bc to
d5f9981
Compare
|
Is the first committee specification draft of VIRTIO 1.3 helpful? Virtual I/O Device (VIRTIO) Version 1.3—5.1.6.3 Setting Up Receive Buffers So what about this? let mut min_buf_size = our_special_mtu;
if !VIRTIO_NET_F_MRG_RXBUF {
if any!(VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4, VIRTIO_NET_F_GUEST_USO6) {
min_buf_size = usize::max(min_buf_size, 65550);
} else {
min_buf_size = usize::max(min_buf_size, 1514);
}
}
if VIRTIO_NET_F_MRG_RXBUF {
// Potentially increase min_buf_size
// Maybe just use min_buf_size for now?
} |
d5f9981 to
7636a75
Compare
c3f3063 to
d75b96d
Compare
Signed-off-by: Jens Reidel <[email protected]>
d75b96d to
c1e22e5
Compare
See "5.1.6.3.1 Driver Requirements: Setting Up Receive Buffers" in the VirtIO specification. We only need to allocate at least the MTU if GUEST_TSO4, GUEST_TSO6 or GUEST_UFO were negotiated, otherwise we SHOULD allocate at least 1514 (1526 minus 12 bytes header) for the payload. Currently we were allocating 65550 (MTU) and always receiving 1514 in that case.