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

On Wed, Dec 14, 2016 at 09:01:27AM -0800, John Fastabend wrote:

On 16-12-14 05:31 AM, Michael S. Tsirkin wrote:

quoted

On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:

quoted

On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:

quoted

On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:

quoted

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 <redacted>
---

[...]

quoted

quoted

quoted

quoted

 
 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