thr3ads.net - Virtualization - [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F

If this information is useful, please help other people find it:
Share via:

Jason Wang

2021-Aug-17 02:03 UTC

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

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

Michael S. Tsirkin

2021-Aug-17 06:45 UTC

head link

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

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 at 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 at redhat.com>
Reported-by: Ivan <ivan at prestigetransportation.com>
Tested-by: Ivan <ivan at prestigetransportation.com>
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;
>  
> -- 
> 2.25.1

Virtualization - Aug 2021 - [PATCH net] virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO

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

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