Michael S. Tsirkin
2021-Aug-12 04:59 UTC
[RFC PATCH] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO
On Wed, Aug 11, 2021 at 08:20:03PM -0500, ivan wrote:> On Wed, Aug 11, 2021 at 3:16 AM Jason Wang <jasowang at redhat.com> wrote: > > > > Commit a02e8964eaf92 ("virtio-net: ethtool configurable LRO") tries to > > advertise LRO on behalf of the guest offloading features and allow the > > administrator to enable and disable those features via ethtool. > > > > This may lead several issues: > > > > - For the device that doesn't support control guest offloads, the > > "LRO" can't be disabled so we will get a warn in the > > dev_disable_lro() > > - For the device that have the control guest offloads, the guest > > offloads were disabled in the case of bridge etc which may slow down > > the traffic. > > > > Try to fix this by using NETIF_F_GRO_HW instead so we're not > > guaranteed to be re-segmented as original. Or we may want a new netdev > > feature like RX_GSO since the guest offloads for virtio-net is > > actually to receive GSO packet. > > > > Or we can try not advertise LRO is control guest offloads is not > > enabled. This solves the warning but will still slow down the traffic. > > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > --- > > drivers/net/virtio_net.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0416a7e00914..10c382b08bce 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -63,7 +63,7 @@ static const unsigned long guest_offloads[] = { > > VIRTIO_NET_F_GUEST_CSUM > > }; > > > > -#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \ > > +#define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \ > > (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \ > > (1ULL << VIRTIO_NET_F_GUEST_ECN) | \ > > (1ULL << VIRTIO_NET_F_GUEST_UFO)) > > @@ -2481,7 +2481,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > > virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) || > > virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) || > > virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))) { > > - NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO/CSUM, disable LRO/CSUM first"); > > + NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first"); > > return -EOPNOTSUPP; > > } > > > > @@ -2612,15 +2612,15 @@ static int virtnet_set_features(struct net_device *dev, > > u64 offloads; > > int err; > > > > - if ((dev->features ^ features) & NETIF_F_LRO) { > > + if ((dev->features ^ features) & NETIF_F_GRO_HW) { > > if (vi->xdp_enabled) > > return -EBUSY; > > > > - if (features & NETIF_F_LRO) > > + if (features & NETIF_F_GRO_HW) > > offloads = vi->guest_offloads_capable; > > else > > offloads = vi->guest_offloads_capable & > > - ~GUEST_OFFLOAD_LRO_MASK; > > + ~GUEST_OFFLOAD_GRO_HW_MASK; > > > > err = virtnet_set_guest_offloads(vi, offloads); > > if (err) > > @@ -3100,9 +3100,9 @@ static int virtnet_probe(struct virtio_device *vdev) > > 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_LRO; > > + dev->features |= NETIF_F_GRO_HW; > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) > > - dev->hw_features |= NETIF_F_LRO; > > + dev->hw_features |= NETIF_F_GRO_HW; > > > > dev->vlan_features = dev->features; > > > > -- > > I applied this patch, recompiled the kernel, and tested it. > The warning messages are gone. Network speed is normal. > I can now enable forwarding, and nothing bad happens. > So far, so good. > > Thank you.OK so that's Tested-by: ivan <ivan at prestigetransportation.com> It is still weird that without the patch networking dies. What happens if you apply the patch then try to disable GRO using ethtool? -- MST