* [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO
@ 2021-08-17 2:03 Jason Wang
2021-08-17 6:45 ` Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Jason Wang @ 2021-08-17 2:03 UTC (permalink / raw)
To: mst, jasowang, davem, kuba
Cc: willemb, virtualization, netdev, linux-kernel, ivan,
xiangxia.m.yue
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO
2021-08-17 2:03 [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO Jason Wang
@ 2021-08-17 6:45 ` Michael S. Tsirkin
2021-08-17 8:16 ` Jason Wang
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2021-08-17 6:45 UTC (permalink / raw)
To: Jason Wang
Cc: davem, kuba, willemb, virtualization, netdev, linux-kernel, ivan,
xiangxia.m.yue
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO
2021-08-17 6:45 ` Michael S. Tsirkin
@ 2021-08-17 8:16 ` Jason Wang
0 siblings, 0 replies; 3+ messages in thread
From: Jason Wang @ 2021-08-17 8:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: davem, Jakub Kicinski, Willem de Bruijn, virtualization, netdev,
linux-kernel, Ivan, Tonghao Zhang
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-17 8:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-17 2:03 [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO Jason Wang
2021-08-17 6:45 ` Michael S. Tsirkin
2021-08-17 8:16 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).