Re: [PATCH net-next v2] virtio-net: enable NETIF_F_GRO_HW only if GRO-related offloads are supported

On Mon, Mar 16, 2026 at 01:57:22PM +0000, Zhud wrote:

 > On Mon, Mar 16, 2026 at 12:57:00PM +0000, Zhud wrote:

quoted

quoted

quoted

On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:

quoted

quoted

Thanks! Yes something to improve:

On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:

quoted

Although VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is negotiated, which
indicates the device supports dynamic control of guest
offloads, it does not necessarily mean the device supports
specific hardware GRO

features.

quoted

quoted

quoted

If none of the features defined in GUEST_OFFLOAD_GRO_HW_MASK
(such as TSO4, TSO6, or UFO) are present in
vi->guest_offloads_capable, the device effectively lacks the hardware

capability to perform GRO.

quoted

quoted

quoted

quoted

So what is the user-visible problem this is trying to address?

A key concern is that once a user enables NETIF_F_GRO_HW via
ethtool, they might manually disable software GRO (ethtool -K eth0
gro off) assuming the hardware is now handling the aggregation.

Thanks!
Sorry could you be even more specific please?
Is this a theoretical concern or did some users encounter this?
Note that NETIF_F_GRO_HW is best effort anyway: e.g.
it can apply only to TCPv6 and v4 will still need software.

This might not be the best example, but I want to draw an analogy to
show how this hardware offload capability can be misleading. For
instance, if I enable GRO_HW expecting to see lower CPU usage when
receiving packets, but it doesn't happen, that would be very confusing.

It still can happen if hardware does not offload the specific traffic, yes?

Yes, of course, but there's still a difference between "best-effort" and "no-effort." Right?

I am not saying this does not improve the user experience.
But let us set the expectations correctly.

What this does (I think):

	When a virtio device does not have either GUEST_TSO6 or
	GUEST_TSO4 offloads, this means it can't really do
	hardware GRO.

	however, the driver will set NETIF_F_GRO_HW whenever
	the device allows control over offload support - even
	if the offloads that can be controlled have nothing
	to do with GRO.

	As a result, in such a setup, rx-gro-hw reported for the device
	is too optimistic.  Improve the situation by masking off
	NETIF_F_GRO_HW.


	Out of abundance of caution, this does not change the
	current behaviour for hardware with just v6 or just v4 GRO:
	current interfaces do not allow
	distinguishing between v6/v4 GRO, so we can't expose
	them to userspace precisely.

Also:

Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")

are you sure it's right?

quoted

quoted

quoted

quoted

Secondly, while we haven't encountered a specific hardware failure
yet, enabling a hardware offload feature that the DPU does not
physically support introduces the risk of undefined hardware
behavior

This would be a major concern but I don't get it - how would one trigger this?
It seems that guest_offloads_capable only includes offloads actually supported.

You're absolutely right. Upon rechecking the code,
virtnet_set_features already ensures that only bits within

vi->guest_offloads_capable are sent to the device.

quoted

Thank you for pointing that out.

quoted

quoted

quoted

quoted

So, making NETIF_F_GRO_HW conditional on these feature bits
ensures the stack does not enable an unsupported hardware
offload

configuration.

quoted

quoted

I guess the assumption is that without this, something enables
such a config? Which stack is this and what happens then?

Sorry for the confusion, let me clarify the intent.
The 'stack' here refers to the ethtool interface and the netset (ioctl/netlink)

path.

quoted

quoted

A bit more detail about the specific set of commands that leads to
confusion in the commit log would be helpful.

quoted

Thanks!

quoted

quoted

quoted

Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
Signed-off-by: Di Zhu <redacted>

judging by this, has something to do with LRO?

quoted

---
/* v2 */
  -make the modified logic clearer
---
 drivers/net/virtio_net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c

b/drivers/net/virtio_net.c index
72d6a9c6a5a2..b233c99925e9 100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -6781,8 +6781,6 @@ static int virtnet_probe(struct virtio_device

*vdev)

quoted

quoted

quoted

quoted

quoted

 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
 		dev->features |= NETIF_F_GRO_HW;
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
-		dev->hw_features |= NETIF_F_GRO_HW;

 	dev->vlan_features = dev->features;
 	dev->xdp_features = NETDEV_XDP_ACT_BASIC |

NETDEV_XDP_ACT_REDIRECT |

quoted

@@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device

*vdev)

quoted

quoted

quoted

quoted

quoted

 	}
 	vi->guest_offloads_capable = vi->guest_offloads;

+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)

&&

quoted

quoted

quoted

quoted

quoted

+	    (vi->guest_offloads_capable &

GUEST_OFFLOAD_GRO_HW_MASK))

quoted

quoted

quoted

quoted

quoted

+		dev->hw_features |= NETIF_F_GRO_HW;
+
 	rtnl_unlock();

 	err = virtnet_cpu_notif_add(vi);
--
2.34.1