| Message ID | 20210817020338.6400-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 126 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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@redhat.com> Proposed rewritten commit log: === [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 <ivan@prestigetransportation.com> Tested-by: Ivan <ivan@prestigetransportation.com> Signed-off-by: Jason Wang <jasowang@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; > > -- > 2.25.1
On Tue, Aug 17, 2021 at 2:46 PM Michael S. Tsirkin <mst@redhat.com> 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@redhat.com> > > > > 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 <ivan@prestigetransportation.com> > Tested-by: Ivan <ivan@prestigetransportation.com> > Signed-off-by: Jason Wang <jasowang@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; > > > > -- > > 2.25.1 >
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;
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. 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 that to the spec and then we can catch the bad configuration and setup. Fixes: a02e8964eaf92 ("virtio-net: ethtool configurable LRO") Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/net/virtio_net.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)