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

On Tue, Aug 17, 2021 at 2:46 PM Michael S. Tsirkin [off-list ref] wrote:

Patch is good. Suggest some tweaks to the commit log.

On Tue, Aug 17, 2021 at 10:03:38AM +0800, Jason Wang wrote:

quoted

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"?

quoted

administrator to enable and disable those features via ethtool.

This may lead several issues:

may lead->lead to

quoted

- 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

quoted

  dev_disable_lro()

.. when turning off LRO or when enabling forwarding bridging etc.

quoted

- For the device that have the control guest offloads, the guest

have the -> supports

quoted

  offloads were disabled in the case of bridge etc

etc -> forwarding etc

quoted

which may slow down

were -> are

may slow -> slows

quoted

  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

quoted

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.

quoted

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.

quoted

Fixes: a02e8964eaf92 ("virtio-net: ethtool configurable LRO")
Signed-off-by: Jason Wang <redacted>

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@redhat.com>
Reported-by: Ivan <redacted>
Tested-by: Ivan <redacted>
Signed-off-by: Jason Wang <redacted>

===

quoted

---
 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