Re: [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 17, 2021 at 2:46 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> Patch is good. Suggest some tweaks to the commit log.
>
> On Tue, Aug 17, 2021 at 10:03:38AM +0800, Jason Wang wrote:
> > Commit a02e8964eaf92 ("virtio-net: ethtool configurable LRO") tries to
> > advertise LRO on behalf of the guest offloading features and allow the
>
> tries to advertise -> advertises
>
> that part actually works.
>
> allow -> allows
>
> on behalf of features is really weird. maybe "maps"?
>
> > administrator to enable and disable those features via ethtool.
> >
> > This may lead several issues:
>
> may lead->lead to
>
> >
> > - For the device that doesn't support control guest offloads, the
> >   "LRO" can't be disabled so we will get a warn in the
>
> warn -> warning
>
> >   dev_disable_lro()
>
> .. when turning off LRO or when enabling forwarding bridging etc.
>
> > - For the device that have the control guest offloads, the guest
>
> have the -> supports
>
> >   offloads were disabled in the case of bridge etc
>
> etc -> forwarding etc
>
> > which may slow down
>
> were -> are
>
> may slow -> slows
>
> >   the traffic.
> >
> > Fixing this by using NETIF_F_GRO_HW instead. Though the spec does not
> > guaranteed to be re-segmented as original explicitly now, we can add
>
> guaranteed -> guarantee
>
> > that to the spec
>
> I would add:
>
> Further, we never advertised LRO historically before a02e8964eaf92
> ("virtio-net: ethtool configurable LRO") and so bridged/forwarded
> configs effectively relied on virtio receive offloads being GRO.
>
>
>
>
> > and then we can catch the bad configuration and
> > setup.
>
> Don't know what does this part mean. How would we catch it?
> With a new flag? Let's say so.
>
> >
> > Fixes: a02e8964eaf92 ("virtio-net: ethtool configurable LRO")
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>
>
>
> Proposed rewritten commit log:

Agree.

Post a new version.

Thanks

>
> ===
> [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO
>
> Commit a02e8964eaf92 ("virtio-net: ethtool configurable LRO")
> maps LRO to virtio guest offloading features and allows the
> administrator to enable and disable those features via ethtool.
>
> This leads to several issues:
>
>
> - For a device that doesn't support control guest offloads, the "LRO"
>   can't be disabled triggering WARN in dev_disable_lro() when turning
>   off LRO or when enabling forwarding bridging etc.
>
> - For a device that supports control guest offloads, the guest
>   offloads are disabled in cases of bridging, forwarding etc
>   slowing down the traffic.
>
> Fix this by using NETIF_F_GRO_HW instead. Though the spec does not
> guarantee packets to be re-segmented as the original ones,
> we can add that to the spec, possibly with a flag for devices to
> differentiate between GRO and LRO.
>
> Further, we never advertised LRO historically before a02e8964eaf92
> ("virtio-net: ethtool configurable LRO") and so bridged/forwarded
> configs effectively always relied on virtio receive offloads behaving
> like GRO - thus even if this breaks any configs it is at least not
> a regression.
>
> Fixes: a02e8964eaf92 ("virtio-net: ethtool configurable LRO")
> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Reported-by: Ivan <ivan@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> Tested-by: Ivan <ivan@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>
> ===
>
>
> > ---
> >  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;
> >
> > --
> > 2.25.1
>

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux