Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-15 Thread Michael S. Tsirkin 
On Wed, Dec 14, 2016 at 09:01:27AM -0800, John Fastabend wrote:
> On 16-12-14 05:31 AM, Michael S. Tsirkin wrote:
> > On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:
> >> On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
>  This adds support for dynamically setting the LRO feature flag. The
>  message to control guest features in the backend uses the
>  CTRL_GUEST_OFFLOADS msg type.
> 
>  Signed-off-by: John Fastabend 
>  ---
> 
> [...]
> 
>   
>   static void virtnet_config_changed_work(struct work_struct *work)
>  @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device 
>  *vdev)
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
>   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->hw_features |= NETIF_F_LRO;
> >>>
> >>> So the issue is I think that the virtio "LRO" isn't really
> >>> LRO, it's typically just GRO forwarded to guests.
> >>> So these are easily re-split along MTU boundaries,
> >>> which makes it ok to forward these across bridges.
> >>>
> >>> It's not nice that we don't document this in the spec,
> >>> but it's the reality and people rely on this.
> >>>
> >>> For now, how about doing a custom thing and just disable/enable
> >>> it as XDP is attached/detached?
> >>
> >> The annoying part about doing this is ethtool will say that it is fixed
> >> yet it will be changed by seemingly unrelated operation. I'm not sure I
> >> like the idea to start automatically configuring the link via xdp_set.
> > 
> > I really don't like the idea of dropping performance
> > by a factor of 3 for people bridging two virtio net
> > interfaces.
> > 
> > So how about a simple approach for now, just disable
> > XDP if GUEST_TSO is enabled?
> > 
> > We can discuss better approaches in next version.
> > 
> 
> So the proposal is to add a check in XDP setup so that
> 
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO{4|6})
>   return -ENOPSUPP;
> 
> Or whatever is the most appropriate return code? Then we can
> disable TSO via qemu-system with guest_tso4=off,guest_tso6=off for
> XDP use cases.

Right. It's a start.

> Sounds like a reasonable start to me. I'll make the change should this
> go through DaveMs net-next tree or do you want it on virtio tree? Either
> is fine with me.
> 
> Thanks,
> John

I think I'll merge it because I'm tweaking RX processing too,
and this will likely conflict.

-- 
MST

Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-14 Thread John Fastabend 
On 16-12-14 05:31 AM, Michael S. Tsirkin wrote:
> On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:
>> On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
>>> On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
 This adds support for dynamically setting the LRO feature flag. The
 message to control guest features in the backend uses the
 CTRL_GUEST_OFFLOADS msg type.

 Signed-off-by: John Fastabend 
 ---

[...]

  
  static void virtnet_config_changed_work(struct work_struct *work)
 @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
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->hw_features |= NETIF_F_LRO;
>>>
>>> So the issue is I think that the virtio "LRO" isn't really
>>> LRO, it's typically just GRO forwarded to guests.
>>> So these are easily re-split along MTU boundaries,
>>> which makes it ok to forward these across bridges.
>>>
>>> It's not nice that we don't document this in the spec,
>>> but it's the reality and people rely on this.
>>>
>>> For now, how about doing a custom thing and just disable/enable
>>> it as XDP is attached/detached?
>>
>> The annoying part about doing this is ethtool will say that it is fixed
>> yet it will be changed by seemingly unrelated operation. I'm not sure I
>> like the idea to start automatically configuring the link via xdp_set.
> 
> I really don't like the idea of dropping performance
> by a factor of 3 for people bridging two virtio net
> interfaces.
> 
> So how about a simple approach for now, just disable
> XDP if GUEST_TSO is enabled?
> 
> We can discuss better approaches in next version.
> 

So the proposal is to add a check in XDP setup so that

  if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO{4|6})
return -ENOPSUPP;

Or whatever is the most appropriate return code? Then we can
disable TSO via qemu-system with guest_tso4=off,guest_tso6=off for
XDP use cases.

Sounds like a reasonable start to me. I'll make the change should this
go through DaveMs net-next tree or do you want it on virtio tree? Either
is fine with me.

Thanks,
John

Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-14 Thread Michael S. Tsirkin 
On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:
> On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
> >> This adds support for dynamically setting the LRO feature flag. The
> >> message to control guest features in the backend uses the
> >> CTRL_GUEST_OFFLOADS msg type.
> >>
> >> Signed-off-by: John Fastabend 
> >> ---
> >>  drivers/net/virtio_net.c |   40 +++-
> >>  1 file changed, 39 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index a21d93a..a5c47b1 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1419,6 +1419,36 @@ static void virtnet_init_settings(struct net_device 
> >> *dev)
> >>.set_settings = virtnet_set_settings,
> >>  };
> >>  
> >> +static int virtnet_set_features(struct net_device *netdev,
> >> +  netdev_features_t features)
> >> +{
> >> +  struct virtnet_info *vi = netdev_priv(netdev);
> >> +  struct virtio_device *vdev = vi->vdev;
> >> +  struct scatterlist sg;
> >> +  u64 offloads = 0;
> >> +
> >> +  if (features & NETIF_F_LRO)
> >> +  offloads |= (1 << VIRTIO_NET_F_GUEST_TSO4) |
> >> +  (1 << VIRTIO_NET_F_GUEST_TSO6);
> >> +
> >> +  if (features & NETIF_F_RXCSUM)
> >> +  offloads |= (1 << VIRTIO_NET_F_GUEST_CSUM);
> >> +
> >> +  if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> >> +  sg_init_one(, , sizeof(uint64_t));
> >> +  if (!virtnet_send_command(vi,
> >> +VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> >> +VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
> >> +)) {
> > 
> > Hmm I just realised that this will slow down setups that bridge
> > virtio net interfaces since bridge calls this if provided.
> > See below.
> 
> 
> Really? What code is trying to turn off GRO via the GUEST_OFFLOADS LRO
> command. My qemu/Linux setup has a set of tap/vhost devices attached to
> a bridge and all of them have LRO enabled even with this patch series.
> 
> I must missing a setup handler somewhere?
> 
> > 
> >> +  dev_warn(>dev,
> >> +   "Failed to set guest offloads by virtnet 
> >> command.\n");
> >> +  return -EINVAL;
> >> +  }
> >> +  }
> > 
> > Hmm if VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is off, this fails
> > silently. It might actually be a good idea to avoid
> > breaking setups.
> > 
> >> +
> >> +  return 0;
> >> +}
> >> +
> >>  static const struct net_device_ops virtnet_netdev = {
> >>.ndo_open= virtnet_open,
> >>.ndo_stop= virtnet_close,
> >> @@ -1435,6 +1465,7 @@ static void virtnet_init_settings(struct net_device 
> >> *dev)
> >>  #ifdef CONFIG_NET_RX_BUSY_POLL
> >>.ndo_busy_poll  = virtnet_busy_poll,
> >>  #endif
> >> +  .ndo_set_features   = virtnet_set_features,
> >>  };
> >>  
> >>  static void virtnet_config_changed_work(struct work_struct *work)
> >> @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> >>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->hw_features |= NETIF_F_LRO;
> > 
> > So the issue is I think that the virtio "LRO" isn't really
> > LRO, it's typically just GRO forwarded to guests.
> > So these are easily re-split along MTU boundaries,
> > which makes it ok to forward these across bridges.
> > 
> > It's not nice that we don't document this in the spec,
> > but it's the reality and people rely on this.
> > 
> > For now, how about doing a custom thing and just disable/enable
> > it as XDP is attached/detached?
> 
> The annoying part about doing this is ethtool will say that it is fixed
> yet it will be changed by seemingly unrelated operation. I'm not sure I
> like the idea to start automatically configuring the link via xdp_set.

I really don't like the idea of dropping performance
by a factor of 3 for people bridging two virtio net
interfaces.

So how about a simple approach for now, just disable
XDP if GUEST_TSO is enabled?

We can discuss better approaches in next version.


> > 
> >> +  }
> >> +
> >>dev->vlan_features = dev->features;
> >>  
> >>/* MTU range: 68 - 65535 */
> >> @@ -2057,7 +2094,8 @@ static int virtnet_restore(struct virtio_device 
> >> *vdev)
> >>VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> -  VIRTIO_NET_F_MTU
> >> +  VIRTIO_NET_F_MTU, \
> >> +  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>  
> >>  static unsigned int features[] = {

Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-08 Thread Michael S. Tsirkin 
On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:
> On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
> >> This adds support for dynamically setting the LRO feature flag. The
> >> message to control guest features in the backend uses the
> >> CTRL_GUEST_OFFLOADS msg type.
> >>
> >> Signed-off-by: John Fastabend 
> >> ---
> >>  drivers/net/virtio_net.c |   40 +++-
> >>  1 file changed, 39 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index a21d93a..a5c47b1 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1419,6 +1419,36 @@ static void virtnet_init_settings(struct net_device 
> >> *dev)
> >>.set_settings = virtnet_set_settings,
> >>  };
> >>  
> >> +static int virtnet_set_features(struct net_device *netdev,
> >> +  netdev_features_t features)
> >> +{
> >> +  struct virtnet_info *vi = netdev_priv(netdev);
> >> +  struct virtio_device *vdev = vi->vdev;
> >> +  struct scatterlist sg;
> >> +  u64 offloads = 0;
> >> +
> >> +  if (features & NETIF_F_LRO)
> >> +  offloads |= (1 << VIRTIO_NET_F_GUEST_TSO4) |
> >> +  (1 << VIRTIO_NET_F_GUEST_TSO6);
> >> +
> >> +  if (features & NETIF_F_RXCSUM)
> >> +  offloads |= (1 << VIRTIO_NET_F_GUEST_CSUM);
> >> +
> >> +  if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> >> +  sg_init_one(, , sizeof(uint64_t));
> >> +  if (!virtnet_send_command(vi,
> >> +VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> >> +VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
> >> +)) {
> > 
> > Hmm I just realised that this will slow down setups that bridge
> > virtio net interfaces since bridge calls this if provided.
> > See below.
> 
> 
> Really? What code is trying to turn off GRO via the GUEST_OFFLOADS LRO
> command. My qemu/Linux setup has a set of tap/vhost devices attached to
> a bridge and all of them have LRO enabled even with this patch series.
> 
> I must missing a setup handler somewhere?

Turning off LRO, not GRO.

> > 
> >> +  dev_warn(>dev,
> >> +   "Failed to set guest offloads by virtnet 
> >> command.\n");
> >> +  return -EINVAL;
> >> +  }
> >> +  }
> > 
> > Hmm if VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is off, this fails
> > silently. It might actually be a good idea to avoid
> > breaking setups.
> > 
> >> +
> >> +  return 0;
> >> +}
> >> +
> >>  static const struct net_device_ops virtnet_netdev = {
> >>.ndo_open= virtnet_open,
> >>.ndo_stop= virtnet_close,
> >> @@ -1435,6 +1465,7 @@ static void virtnet_init_settings(struct net_device 
> >> *dev)
> >>  #ifdef CONFIG_NET_RX_BUSY_POLL
> >>.ndo_busy_poll  = virtnet_busy_poll,
> >>  #endif
> >> +  .ndo_set_features   = virtnet_set_features,
> >>  };
> >>  
> >>  static void virtnet_config_changed_work(struct work_struct *work)
> >> @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> >>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->hw_features |= NETIF_F_LRO;
> > 
> > So the issue is I think that the virtio "LRO" isn't really
> > LRO, it's typically just GRO forwarded to guests.
> > So these are easily re-split along MTU boundaries,
> > which makes it ok to forward these across bridges.
> > 
> > It's not nice that we don't document this in the spec,
> > but it's the reality and people rely on this.
> > 
> > For now, how about doing a custom thing and just disable/enable
> > it as XDP is attached/detached?
> 
> The annoying part about doing this is ethtool will say that it is fixed
> yet it will be changed by seemingly unrelated operation.

ATM ethtool does not tell you LRO is enabled, since
again, this isn't exactly LRO. It's a safe variant that
supports e.g. bridging.

> I'm not sure I
> like the idea to start automatically configuring the link via xdp_set.

That's what I thought up off-hand. Let me think about better
interfaces over the weekend.

> > 
> >> +  }
> >> +
> >>dev->vlan_features = dev->features;
> >>  
> >>/* MTU range: 68 - 65535 */
> >> @@ -2057,7 +2094,8 @@ static int virtnet_restore(struct virtio_device 
> >> *vdev)
> >>VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> -  VIRTIO_NET_F_MTU
> >> +  VIRTIO_NET_F_MTU, \
> >> +  VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>  
> >>  static unsigned int features[] = {
>

Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-08 Thread John Fastabend 
On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
>> This adds support for dynamically setting the LRO feature flag. The
>> message to control guest features in the backend uses the
>> CTRL_GUEST_OFFLOADS msg type.
>>
>> Signed-off-by: John Fastabend 
>> ---
>>  drivers/net/virtio_net.c |   40 +++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index a21d93a..a5c47b1 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1419,6 +1419,36 @@ static void virtnet_init_settings(struct net_device 
>> *dev)
>>  .set_settings = virtnet_set_settings,
>>  };
>>  
>> +static int virtnet_set_features(struct net_device *netdev,
>> +netdev_features_t features)
>> +{
>> +struct virtnet_info *vi = netdev_priv(netdev);
>> +struct virtio_device *vdev = vi->vdev;
>> +struct scatterlist sg;
>> +u64 offloads = 0;
>> +
>> +if (features & NETIF_F_LRO)
>> +offloads |= (1 << VIRTIO_NET_F_GUEST_TSO4) |
>> +(1 << VIRTIO_NET_F_GUEST_TSO6);
>> +
>> +if (features & NETIF_F_RXCSUM)
>> +offloads |= (1 << VIRTIO_NET_F_GUEST_CSUM);
>> +
>> +if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
>> +sg_init_one(, , sizeof(uint64_t));
>> +if (!virtnet_send_command(vi,
>> +  VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> +  VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> +  )) {
> 
> Hmm I just realised that this will slow down setups that bridge
> virtio net interfaces since bridge calls this if provided.
> See below.


Really? What code is trying to turn off GRO via the GUEST_OFFLOADS LRO
command. My qemu/Linux setup has a set of tap/vhost devices attached to
a bridge and all of them have LRO enabled even with this patch series.

I must missing a setup handler somewhere?

> 
>> +dev_warn(>dev,
>> + "Failed to set guest offloads by virtnet 
>> command.\n");
>> +return -EINVAL;
>> +}
>> +}
> 
> Hmm if VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is off, this fails
> silently. It might actually be a good idea to avoid
> breaking setups.
> 
>> +
>> +return 0;
>> +}
>> +
>>  static const struct net_device_ops virtnet_netdev = {
>>  .ndo_open= virtnet_open,
>>  .ndo_stop= virtnet_close,
>> @@ -1435,6 +1465,7 @@ static void virtnet_init_settings(struct net_device 
>> *dev)
>>  #ifdef CONFIG_NET_RX_BUSY_POLL
>>  .ndo_busy_poll  = virtnet_busy_poll,
>>  #endif
>> +.ndo_set_features   = virtnet_set_features,
>>  };
>>  
>>  static void virtnet_config_changed_work(struct work_struct *work)
>> @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>>  if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
>>  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->hw_features |= NETIF_F_LRO;
> 
> So the issue is I think that the virtio "LRO" isn't really
> LRO, it's typically just GRO forwarded to guests.
> So these are easily re-split along MTU boundaries,
> which makes it ok to forward these across bridges.
> 
> It's not nice that we don't document this in the spec,
> but it's the reality and people rely on this.
> 
> For now, how about doing a custom thing and just disable/enable
> it as XDP is attached/detached?

The annoying part about doing this is ethtool will say that it is fixed
yet it will be changed by seemingly unrelated operation. I'm not sure I
like the idea to start automatically configuring the link via xdp_set.

> 
>> +}
>> +
>>  dev->vlan_features = dev->features;
>>  
>>  /* MTU range: 68 - 65535 */
>> @@ -2057,7 +2094,8 @@ static int virtnet_restore(struct virtio_device *vdev)
>>  VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>  VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>  VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> -VIRTIO_NET_F_MTU
>> +VIRTIO_NET_F_MTU, \
>> +VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>  
>>  static unsigned int features[] = {
>>  VIRTNET_FEATURES,

Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-08 Thread Michael S. Tsirkin 
On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
> This adds support for dynamically setting the LRO feature flag. The
> message to control guest features in the backend uses the
> CTRL_GUEST_OFFLOADS msg type.
> 
> Signed-off-by: John Fastabend 
> ---
>  drivers/net/virtio_net.c |   40 +++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a21d93a..a5c47b1 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1419,6 +1419,36 @@ static void virtnet_init_settings(struct net_device 
> *dev)
>   .set_settings = virtnet_set_settings,
>  };
>  
> +static int virtnet_set_features(struct net_device *netdev,
> + netdev_features_t features)
> +{
> + struct virtnet_info *vi = netdev_priv(netdev);
> + struct virtio_device *vdev = vi->vdev;
> + struct scatterlist sg;
> + u64 offloads = 0;
> +
> + if (features & NETIF_F_LRO)
> + offloads |= (1 << VIRTIO_NET_F_GUEST_TSO4) |
> + (1 << VIRTIO_NET_F_GUEST_TSO6);
> +
> + if (features & NETIF_F_RXCSUM)
> + offloads |= (1 << VIRTIO_NET_F_GUEST_CSUM);
> +
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> + sg_init_one(, , sizeof(uint64_t));
> + if (!virtnet_send_command(vi,
> +   VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> +   VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
> +   )) {

Hmm I just realised that this will slow down setups that bridge
virtio net interfaces since bridge calls this if provided.
See below.

> + dev_warn(>dev,
> +  "Failed to set guest offloads by virtnet 
> command.\n");
> + return -EINVAL;
> + }
> + }

Hmm if VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is off, this fails
silently. It might actually be a good idea to avoid
breaking setups.

> +
> + return 0;
> +}
> +
>  static const struct net_device_ops virtnet_netdev = {
>   .ndo_open= virtnet_open,
>   .ndo_stop= virtnet_close,
> @@ -1435,6 +1465,7 @@ static void virtnet_init_settings(struct net_device 
> *dev)
>  #ifdef CONFIG_NET_RX_BUSY_POLL
>   .ndo_busy_poll  = virtnet_busy_poll,
>  #endif
> + .ndo_set_features   = virtnet_set_features,
>  };
>  
>  static void virtnet_config_changed_work(struct work_struct *work)
> @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
>   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->hw_features |= NETIF_F_LRO;

So the issue is I think that the virtio "LRO" isn't really
LRO, it's typically just GRO forwarded to guests.
So these are easily re-split along MTU boundaries,
which makes it ok to forward these across bridges.

It's not nice that we don't document this in the spec,
but it's the reality and people rely on this.

For now, how about doing a custom thing and just disable/enable
it as XDP is attached/detached?

> + }
> +
>   dev->vlan_features = dev->features;
>  
>   /* MTU range: 68 - 65535 */
> @@ -2057,7 +2094,8 @@ static int virtnet_restore(struct virtio_device *vdev)
>   VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>   VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>   VIRTIO_NET_F_CTRL_MAC_ADDR, \
> - VIRTIO_NET_F_MTU
> + VIRTIO_NET_F_MTU, \
> + VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>  
>  static unsigned int features[] = {
>   VIRTNET_FEATURES,

[net-next PATCH v5 1/6] net: virtio dynamically disable/enable LRO

2016-12-07 Thread John Fastabend 
This adds support for dynamically setting the LRO feature flag. The
message to control guest features in the backend uses the
CTRL_GUEST_OFFLOADS msg type.

Signed-off-by: John Fastabend 
---
 drivers/net/virtio_net.c |   40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a21d93a..a5c47b1 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1419,6 +1419,36 @@ static void virtnet_init_settings(struct net_device *dev)
.set_settings = virtnet_set_settings,
 };
 
+static int virtnet_set_features(struct net_device *netdev,
+   netdev_features_t features)
+{
+   struct virtnet_info *vi = netdev_priv(netdev);
+   struct virtio_device *vdev = vi->vdev;
+   struct scatterlist sg;
+   u64 offloads = 0;
+
+   if (features & NETIF_F_LRO)
+   offloads |= (1 << VIRTIO_NET_F_GUEST_TSO4) |
+   (1 << VIRTIO_NET_F_GUEST_TSO6);
+
+   if (features & NETIF_F_RXCSUM)
+   offloads |= (1 << VIRTIO_NET_F_GUEST_CSUM);
+
+   if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
+   sg_init_one(, , sizeof(uint64_t));
+   if (!virtnet_send_command(vi,
+ VIRTIO_NET_CTRL_GUEST_OFFLOADS,
+ VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
+ )) {
+   dev_warn(>dev,
+"Failed to set guest offloads by virtnet 
command.\n");
+   return -EINVAL;
+   }
+   }
+
+   return 0;
+}
+
 static const struct net_device_ops virtnet_netdev = {
.ndo_open= virtnet_open,
.ndo_stop= virtnet_close,
@@ -1435,6 +1465,7 @@ static void virtnet_init_settings(struct net_device *dev)
 #ifdef CONFIG_NET_RX_BUSY_POLL
.ndo_busy_poll  = virtnet_busy_poll,
 #endif
+   .ndo_set_features   = virtnet_set_features,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
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->hw_features |= NETIF_F_LRO;
+   }
+
dev->vlan_features = dev->features;
 
/* MTU range: 68 - 65535 */
@@ -2057,7 +2094,8 @@ static int virtnet_restore(struct virtio_device *vdev)
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
-   VIRTIO_NET_F_MTU
+   VIRTIO_NET_F_MTU, \
+   VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
 
 static unsigned int features[] = {
VIRTNET_FEATURES,