| Message ID | 20240617131524.63662-2-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 604141c036e1b636e2a71cf6e1aa09d1e45f40c2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: fixes for checksum offloading and XDP handling | expand |
Mon, Jun 17, 2024 at 03:15:23PM CEST, hengqi@linux.alibaba.com wrote: >In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle >partially checksummed packets, and the validation of fully checksummed >packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM >negotiation. However, the specification erroneously stated: > > "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags > to zero and SHOULD supply a fully checksummed packet to the driver." > >This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM >negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag. >Essentially, the device can facilitate the validation of these packets' >checksums - a process known as RX checksum offloading - removing the need >for the driver to do so. > >This scenario is currently not implemented in the driver and requires >correction. The necessary specification correction[1] has been made and >approved in the virtio TC vote. >[1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html > >Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available") >Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
On Mon, Jun 17, 2024 at 9:15 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle > partially checksummed packets, and the validation of fully checksummed > packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM > negotiation. However, the specification erroneously stated: > > "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags > to zero and SHOULD supply a fully checksummed packet to the driver." > > This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM > negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag. > Essentially, the device can facilitate the validation of these packets' > checksums - a process known as RX checksum offloading - removing the need > for the driver to do so. > > This scenario is currently not implemented in the driver and requires > correction. The necessary specification correction[1] has been made and > approved in the virtio TC vote. > [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html > > Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > --- Acked-by: Jason Wang <jasowang@redhat.com> (Should we manually do checksum if RXCUSM is disabled?) Thanks
On Tue, 18 Jun 2024 11:01:27 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Mon, Jun 17, 2024 at 9:15 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle > > partially checksummed packets, and the validation of fully checksummed > > packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM > > negotiation. However, the specification erroneously stated: > > > > "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags > > to zero and SHOULD supply a fully checksummed packet to the driver." > > > > This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM > > negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag. > > Essentially, the device can facilitate the validation of these packets' > > checksums - a process known as RX checksum offloading - removing the need > > for the driver to do so. > > > > This scenario is currently not implemented in the driver and requires > > correction. The necessary specification correction[1] has been made and > > approved in the virtio TC vote. > > [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html > > > > Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available") > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > --- > > Acked-by: Jason Wang <jasowang@redhat.com> > > (Should we manually do checksum if RXCUSM is disabled?) > Currently we do not allow RXCUSM to be disabled. Thanks. > Thanks >
On Tue, 18 Jun 2024 11:09:02 +0800 Heng Qi wrote: > > (Should we manually do checksum if RXCUSM is disabled?) > > > > Currently we do not allow RXCUSM to be disabled. You don't have to disable checksuming in the device. Just ignore VIRTIO_NET_HDR_F_DATA_VALID if user cleared NETIF_F_RXCSUM. I know some paranoid workloads which do actually want the kernel to calculate the checksum.
On Tue, 18 Jun 2024 18:15:16 -0700, Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 18 Jun 2024 11:09:02 +0800 Heng Qi wrote: > > > (Should we manually do checksum if RXCUSM is disabled?) > > > > > > > Currently we do not allow RXCUSM to be disabled. > > You don't have to disable checksuming in the device. Yes, it is up to the device itself to decide whether to validate checksum. What I mean is that we don't allow users to disable the driver's NETIF_F_RXCSUM flag. > Just ignore VIRTIO_NET_HDR_F_DATA_VALID if user cleared NETIF_F_RXCSUM. Right. Thanks. > I know some paranoid workloads which do actually want the kernel to > calculate the checksum. >
On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote: > > > Currently we do not allow RXCUSM to be disabled. > > > > You don't have to disable checksuming in the device. > > Yes, it is up to the device itself to decide whether to validate checksum. > What I mean is that we don't allow users to disable the driver's > NETIF_F_RXCSUM flag. I understand. What I'm suggesting is that you send a follow up patch that allows it.
在 2024/6/19 下午11:08, Jakub Kicinski 写道: > On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote: >>>> Currently we do not allow RXCUSM to be disabled. >>> You don't have to disable checksuming in the device. >> Yes, it is up to the device itself to decide whether to validate checksum. >> What I mean is that we don't allow users to disable the driver's >> NETIF_F_RXCSUM flag. > I understand. What I'm suggesting is that you send a follow up patch > that allows it. OK, will do it. Thanks.
On Wed, Jun 19, 2024 at 11:44 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > 在 2024/6/19 下午11:08, Jakub Kicinski 写道: > > On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote: > >>>> Currently we do not allow RXCUSM to be disabled. > >>> You don't have to disable checksuming in the device. > >> Yes, it is up to the device itself to decide whether to validate checksum. > >> What I mean is that we don't allow users to disable the driver's > >> NETIF_F_RXCSUM flag. > > I understand. What I'm suggesting is that you send a follow up patch > > that allows it. Exactly my point as well. > > OK, will do it. > > Thanks. Thanks > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 61a57d134544..aa70a7ed8072 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -5666,8 +5666,16 @@ static int virtnet_probe(struct virtio_device *vdev) dev->features |= dev->hw_features & NETIF_F_ALL_TSO; /* (!csum && gso) case will be fixed by register_netdev() */ } - if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM)) - dev->features |= NETIF_F_RXCSUM; + + /* 1. With VIRTIO_NET_F_GUEST_CSUM negotiation, the driver doesn't + * need to calculate checksums for partially checksummed packets, + * as they're considered valid by the upper layer. + * 2. Without VIRTIO_NET_F_GUEST_CSUM negotiation, the driver only + * receives fully checksummed packets. The device may assist in + * validating these packets' checksums, so the driver won't have to. + */ + dev->features |= NETIF_F_RXCSUM; + if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) dev->features |= NETIF_F_GRO_HW;
In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle partially checksummed packets, and the validation of fully checksummed packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM negotiation. However, the specification erroneously stated: "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags to zero and SHOULD supply a fully checksummed packet to the driver." This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag. Essentially, the device can facilitate the validation of these packets' checksums - a process known as RX checksum offloading - removing the need for the driver to do so. This scenario is currently not implemented in the driver and requires correction. The necessary specification correction[1] has been made and approved in the virtio TC vote. [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available") Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> --- drivers/net/virtio_net.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)