[PATCH net-next] virtio-net: ethtool configurable LRO

[PATCH net-next] virtio-net: ethtool configurable LRO

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Virtio-net devices negotiate LRO support with the host.
Display the initially negotiated state with ethtool -k.

Also allow configuring it with ethtool -K, reusing the existing
virtnet_set_guest_offloads helper that disables LRO for XDP.

Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
distinguish between the two. In the unlikely legacy case where the
host only supports one of the two, maintain the current state.

RTNL is held while calling virtnet_set_features, same as on the path
from virtnet_xdp_set.

Tested:
  Verified that large-receive-offload is "off [fixed]" when not
  negotiated during initial probe and "on" otherwise.

  Measured packet size when scp-ing a large file to the guest with
  nstat (IpExtInOctets / IpInDelivers). Verified that packet size
  exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ea672145f6a66..9890ef30ef32e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2479,6 +2479,22 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
 	return 0;
 }
 
+static int virtnet_set_features(struct net_device *dev,
+				netdev_features_t features)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	int ret = 0;
+
+	if ((dev->features ^ features) & NETIF_F_LRO) {
+		if (features & NETIF_F_LRO)
+			ret = virtnet_restore_guest_offloads(vi);
+		else
+			ret = virtnet_clear_guest_offloads(vi);
+	}
+
+	return ret;
+}
+
 static const struct net_device_ops virtnet_netdev = {
 	.ndo_open            = virtnet_open,
 	.ndo_stop   	     = virtnet_close,
@@ -2493,6 +2509,7 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_xdp_xmit		= virtnet_xdp_xmit,
 	.ndo_features_check	= passthru_features_check,
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
+	.ndo_set_features	= virtnet_set_features,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -2942,6 +2959,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->hw_features |= NETIF_F_TSO6;
 		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
 			dev->hw_features |= NETIF_F_TSO_ECN;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) &&
+		    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) {
+			dev->hw_features |= NETIF_F_LRO;
+			dev->features |= NETIF_F_LRO;
+		}
 
 		dev->features |= NETIF_F_GSO_ROBUST;
 
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Willem de Bruijn]

On Wed, Dec 19, 2018 at 5:32 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> Virtio-net devices negotiate LRO support with the host.
> Display the initially negotiated state with ethtool -k.
>
> Also allow configuring it with ethtool -K, reusing the existing
> virtnet_set_guest_offloads helper that disables LRO for XDP.
>
> Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> distinguish between the two. In the unlikely legacy case where the
> host only supports one of the two, maintain the current state.
>
> RTNL is held while calling virtnet_set_features, same as on the path
> from virtnet_xdp_set.
>
> Tested:
>   Verified that large-receive-offload is "off [fixed]" when not
>   negotiated during initial probe and "on" otherwise.
>
>   Measured packet size when scp-ing a large file to the guest with
>   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
>   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Two comments for the review:

1. Current drivers show LRO as disabled even if it is negotiated at
probe. That is arguably a bug. In which case I can split this patch into
- a net patch to advertise the initially negotiated value through dev->feature
- a net-next patch to configure it with ndo_set_features.

2. virtnet_clear_guest_offloads disables all guest_offloads, including
VIRTIO_NET_F_GUEST_CSUM. That seems undesirable? The feature is a
prerequisite for LRO, but not the other way around. This applies
equally to the xdp and ethtool paths, so would be another  separate
net patch.

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 05:44:44PM -0500, Willem de Bruijn wrote:
> On Wed, Dec 19, 2018 at 5:32 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Virtio-net devices negotiate LRO support with the host.
> > Display the initially negotiated state with ethtool -k.
> >
> > Also allow configuring it with ethtool -K, reusing the existing
> > virtnet_set_guest_offloads helper that disables LRO for XDP.
> >
> > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > distinguish between the two. In the unlikely legacy case where the
> > host only supports one of the two, maintain the current state.
> >
> > RTNL is held while calling virtnet_set_features, same as on the path
> > from virtnet_xdp_set.
> >
> > Tested:
> >   Verified that large-receive-offload is "off [fixed]" when not
> >   negotiated during initial probe and "on" otherwise.
> >
> >   Measured packet size when scp-ing a large file to the guest with
> >   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
> >   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> >
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> Two comments for the review:
> 
> 1. Current drivers show LRO as disabled even if it is negotiated at
> probe. That is arguably a bug. In which case I can split this patch into
> - a net patch to advertise the initially negotiated value through dev->feature
> - a net-next patch to configure it with ndo_set_features.
> 
> 2. virtnet_clear_guest_offloads disables all guest_offloads, including
> VIRTIO_NET_F_GUEST_CSUM. That seems undesirable? The feature is a
> prerequisite for LRO, but not the other way around. This applies
> equally to the xdp and ethtool paths, so would be another  separate
> net patch.

Makes sense I guess.

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Willem de Bruijn]

On Wed, Dec 19, 2018 at 5:44 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Dec 19, 2018 at 5:32 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Virtio-net devices negotiate LRO support with the host.
> > Display the initially negotiated state with ethtool -k.
> >
> > Also allow configuring it with ethtool -K, reusing the existing
> > virtnet_set_guest_offloads helper that disables LRO for XDP.
> >
> > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > distinguish between the two. In the unlikely legacy case where the
> > host only supports one of the two, maintain the current state.
> >
> > RTNL is held while calling virtnet_set_features, same as on the path
> > from virtnet_xdp_set.
> >
> > Tested:
> >   Verified that large-receive-offload is "off [fixed]" when not
> >   negotiated during initial probe and "on" otherwise.
> >
> >   Measured packet size when scp-ing a large file to the guest with
> >   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
> >   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> >
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> Two comments for the review:
>
> 1. Current drivers show LRO as disabled even if it is negotiated at
> probe. That is arguably a bug. In which case I can split this patch into
> - a net patch to advertise the initially negotiated value through dev->feature
> - a net-next patch to configure it with ndo_set_features.
>
> 2. virtnet_clear_guest_offloads disables all guest_offloads, including
> VIRTIO_NET_F_GUEST_CSUM. That seems undesirable? The feature is a
> prerequisite for LRO, but not the other way around. This applies
> equally to the xdp and ethtool paths, so would be another  separate
> net patch.

I was wrong. This was a deliberate choice. See below. Will leave as is.

The other change is not really important enough on its, so I'll just
send a v2 of this patch.

One additional change, show LRO as enabled if it is active for any protocol
(as opposed to only for all protocols), so that we can truly disable it in all
cases.

"
commit e59ff2c49ae16e1d179de679aca81405829aee6c
Author: Jason Wang <jasowang@redhat.com>
Date:   Thu Nov 22 14:36:30 2018 +0800

    virtio-net: disable guest csum during XDP set

    We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means we
    can receive partial csumed packets with metadata kept in the
    vnet_hdr. This may have several side effects:

    - It could be overridden by header adjustment, thus is might be not
      correct after XDP processing.
    - There's no way to pass such metadata information through
      XDP_REDIRECT to another driver.
    - XDP does not support checksum offload right now.

    So simply disable guest csum if possible in this the case of XDP.
"

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Michael S. Tsirkin]

On Thu, Dec 20, 2018 at 01:30:15PM -0500, Willem de Bruijn wrote:
> On Wed, Dec 19, 2018 at 5:44 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, Dec 19, 2018 at 5:32 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb@google.com>
> > >
> > > Virtio-net devices negotiate LRO support with the host.
> > > Display the initially negotiated state with ethtool -k.
> > >
> > > Also allow configuring it with ethtool -K, reusing the existing
> > > virtnet_set_guest_offloads helper that disables LRO for XDP.
> > >
> > > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > > distinguish between the two. In the unlikely legacy case where the
> > > host only supports one of the two, maintain the current state.
> > >
> > > RTNL is held while calling virtnet_set_features, same as on the path
> > > from virtnet_xdp_set.
> > >
> > > Tested:
> > >   Verified that large-receive-offload is "off [fixed]" when not
> > >   negotiated during initial probe and "on" otherwise.
> > >
> > >   Measured packet size when scp-ing a large file to the guest with
> > >   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
> > >   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> > >
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > Two comments for the review:
> >
> > 1. Current drivers show LRO as disabled even if it is negotiated at
> > probe. That is arguably a bug. In which case I can split this patch into
> > - a net patch to advertise the initially negotiated value through dev->feature
> > - a net-next patch to configure it with ndo_set_features.
> >
> > 2. virtnet_clear_guest_offloads disables all guest_offloads, including
> > VIRTIO_NET_F_GUEST_CSUM. That seems undesirable? The feature is a
> > prerequisite for LRO, but not the other way around. This applies
> > equally to the xdp and ethtool paths, so would be another  separate
> > net patch.
> 
> I was wrong. This was a deliberate choice. See below. Will leave as is.
> 
> The other change is not really important enough on its, so I'll just
> send a v2 of this patch.
> 
> One additional change, show LRO as enabled if it is active for any protocol
> (as opposed to only for all protocols), so that we can truly disable it in all
> cases.

That makes sense.

> "
> commit e59ff2c49ae16e1d179de679aca81405829aee6c
> Author: Jason Wang <jasowang@redhat.com>
> Date:   Thu Nov 22 14:36:30 2018 +0800
> 
>     virtio-net: disable guest csum during XDP set
> 
>     We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means we
>     can receive partial csumed packets with metadata kept in the
>     vnet_hdr. This may have several side effects:
> 
>     - It could be overridden by header adjustment, thus is might be not
>       correct after XDP processing.
>     - There's no way to pass such metadata information through
>       XDP_REDIRECT to another driver.
>     - XDP does not support checksum offload right now.
> 
>     So simply disable guest csum if possible in this the case of XDP.
> "

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 05:32:27PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Virtio-net devices negotiate LRO support with the host.
> Display the initially negotiated state with ethtool -k.
> 
> Also allow configuring it with ethtool -K, reusing the existing
> virtnet_set_guest_offloads helper that disables LRO for XDP.
> 
> Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> distinguish between the two. In the unlikely legacy case where the
> host only supports one of the two, maintain the current state.
> 
> RTNL is held while calling virtnet_set_features, same as on the path
> from virtnet_xdp_set.
> 
> Tested:
>   Verified that large-receive-offload is "off [fixed]" when not
>   negotiated during initial probe and "on" otherwise.
> 
>   Measured packet size when scp-ing a large file to the guest with
>   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
>   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>

So I see a bug here:

	virtnet_send_command calls:
		/* Caller should know better */
		BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));

works fine for XDP because that checks VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
in virtnet_xdp_set.


> ---
>  drivers/net/virtio_net.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ea672145f6a66..9890ef30ef32e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2479,6 +2479,22 @@ static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
>  	return 0;
>  }
>  
> +static int virtnet_set_features(struct net_device *dev,
> +				netdev_features_t features)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	int ret = 0;
> +
> +	if ((dev->features ^ features) & NETIF_F_LRO) {
> +		if (features & NETIF_F_LRO)
> +			ret = virtnet_restore_guest_offloads(vi);
> +		else
> +			ret = virtnet_clear_guest_offloads(vi);
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct net_device_ops virtnet_netdev = {
>  	.ndo_open            = virtnet_open,
>  	.ndo_stop   	     = virtnet_close,
> @@ -2493,6 +2509,7 @@ static const struct net_device_ops virtnet_netdev = {
>  	.ndo_xdp_xmit		= virtnet_xdp_xmit,
>  	.ndo_features_check	= passthru_features_check,
>  	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
> +	.ndo_set_features	= virtnet_set_features,
>  };
>  
>  static void virtnet_config_changed_work(struct work_struct *work)
> @@ -2942,6 +2959,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			dev->hw_features |= NETIF_F_TSO6;
>  		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
>  			dev->hw_features |= NETIF_F_TSO_ECN;
> +		if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) &&
> +		    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) {
> +			dev->hw_features |= NETIF_F_LRO;
> +			dev->features |= NETIF_F_LRO;
> +		}
>  
>  		dev->features |= NETIF_F_GSO_ROBUST;
>  
> -- 
> 2.20.1.415.g653613c723-goog

Re: [PATCH net-next] virtio-net: ethtool configurable LRO

[Willem de Bruijn]

On Wed, Dec 19, 2018 at 6:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Dec 19, 2018 at 05:32:27PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Virtio-net devices negotiate LRO support with the host.
> > Display the initially negotiated state with ethtool -k.
> >
> > Also allow configuring it with ethtool -K, reusing the existing
> > virtnet_set_guest_offloads helper that disables LRO for XDP.
> >
> > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > distinguish between the two. In the unlikely legacy case where the
> > host only supports one of the two, maintain the current state.
> >
> > RTNL is held while calling virtnet_set_features, same as on the path
> > from virtnet_xdp_set.
> >
> > Tested:
> >   Verified that large-receive-offload is "off [fixed]" when not
> >   negotiated during initial probe and "on" otherwise.
> >
> >   Measured packet size when scp-ing a large file to the guest with
> >   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
> >   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> >
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> So I see a bug here:
>
>         virtnet_send_command calls:
>                 /* Caller should know better */
>                 BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>
> works fine for XDP because that checks VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> in virtnet_xdp_set.

Oh, indeed. Of course I need to check that flag before advertising as
configurable. Thanks!