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

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

[Di Zhu]

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.

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.

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

Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
Signed-off-by: Di Zhu <zhud@hygon.cn>
---
/* 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)
 	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 |
@@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	}
 	vi->guest_offloads_capable = vi->guest_offloads;
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
+	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
+		dev->hw_features |= NETIF_F_GRO_HW;
+
 	rtnl_unlock();
 
 	err = virtnet_cpu_notif_add(vi);
-- 
2.34.1

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

[Michael S. Tsirkin]

Thanks! Yes something to improve:

On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> 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.
> 
> 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.

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

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

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



> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Signed-off-by: Di Zhu <zhud@hygon.cn>

judging by this, has something to do with LRO?

> ---
> /* 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)
>  	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 |
> @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	}
>  	vi->guest_offloads_capable = vi->guest_offloads;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> +		dev->hw_features |= NETIF_F_GRO_HW;
> +
>  	rtnl_unlock();
>  
>  	err = virtnet_cpu_notif_add(vi);
> -- 
> 2.34.1
> 

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

[Zhud]

> Thanks! Yes something to improve:
> 
> On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > 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.
> >
> > 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.
> 
> 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.

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

> >
> > So, making NETIF_F_GRO_HW conditional on these feature bits ensures
> > the stack does not enable an unsupported hardware offload configuration.
> 
> 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. 

> 
> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > Signed-off-by: Di Zhu <zhud@hygon.cn>
> 
> judging by this, has something to do with LRO?
> 
> > ---
> > /* 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)
> >  	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 |
> > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> >  	}
> >  	vi->guest_offloads_capable = vi->guest_offloads;
> >
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> > +		dev->hw_features |= NETIF_F_GRO_HW;
> > +
> >  	rtnl_unlock();
> >
> >  	err = virtnet_cpu_notif_add(vi);
> > --
> > 2.34.1
> >
> 

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

[Michael S. Tsirkin]

On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > Thanks! Yes something to improve:
> > 
> > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > 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.
> > >
> > > 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.
> > 
> > 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.

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

> > >
> > > So, making NETIF_F_GRO_HW conditional on these feature bits ensures
> > > the stack does not enable an unsupported hardware offload configuration.
> > 
> > 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. 


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

> > 
> > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > 
> > judging by this, has something to do with LRO?
> > 
> > > ---
> > > /* 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)
> > >  	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 |
> > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> > >  	}
> > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > >
> > > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > +
> > >  	rtnl_unlock();
> > >
> > >  	err = virtnet_cpu_notif_add(vi);
> > > --
> > > 2.34.1
> > >
> > 
> 

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

[Zhud]

> On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > > Thanks! Yes something to improve:
> > >
> > > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > > 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.
> > > >
> > > > 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.
> > >
> > > 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.

> > 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.
Thank you for pointing that out.

> > > >
> > > > So, making NETIF_F_GRO_HW conditional on these feature bits
> > > > ensures the stack does not enable an unsupported hardware offload
> configuration.
> > >
> > > 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.
> 
> 
> A bit more detail about the specific set of commands that leads to confusion in the
> commit log would be helpful.

> Thanks!
> 
> > >
> > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > >
> > > judging by this, has something to do with LRO?
> > >
> > > > ---
> > > > /* 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)
> > > >  	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 |
> > > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > >  	}
> > > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > > >
> > > > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> > > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > > +
> > > >  	rtnl_unlock();
> > > >
> > > >  	err = virtnet_cpu_notif_add(vi);
> > > > --
> > > > 2.34.1
> > > >
> > >
> >
> 

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

[Michael S. Tsirkin]

On Mon, Mar 16, 2026 at 12:57:00PM +0000, Zhud wrote:
> > On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > > > Thanks! Yes something to improve:
> > > >
> > > > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > > > 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.
> > > > >
> > > > > 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.
> > > >
> > > > 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?

> > > 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.
> Thank you for pointing that out.
> 
> > > > >
> > > > > So, making NETIF_F_GRO_HW conditional on these feature bits
> > > > > ensures the stack does not enable an unsupported hardware offload
> > configuration.
> > > >
> > > > 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.
> > 
> > 
> > A bit more detail about the specific set of commands that leads to confusion in the
> > commit log would be helpful.
> 
> > Thanks!
> > 
> > > >
> > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > > >
> > > > judging by this, has something to do with LRO?
> > > >
> > > > > ---
> > > > > /* 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)
> > > > >  	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 |
> > > > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > > >  	}
> > > > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > > > >
> > > > > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > > > > +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> > > > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > > > +
> > > > >  	rtnl_unlock();
> > > > >
> > > > >  	err = virtnet_cpu_notif_add(vi);
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > >
> > 
> 

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

[Zhud]

 > On Mon, Mar 16, 2026 at 12:57:00PM +0000, Zhud wrote:
> > > On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > > > > Thanks! Yes something to improve:
> > > > >
> > > > > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > > > > 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.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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?

> 
> > > > 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.
> > Thank you for pointing that out.
> >
> > > > > >
> > > > > > So, making NETIF_F_GRO_HW conditional on these feature bits
> > > > > > ensures the stack does not enable an unsupported hardware
> > > > > > offload
> > > configuration.
> > > > >
> > > > > 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.
> > >
> > >
> > > A bit more detail about the specific set of commands that leads to
> > > confusion in the commit log would be helpful.
> >
> > > Thanks!
> > >
> > > > >
> > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > > > >
> > > > > judging by this, has something to do with LRO?
> > > > >
> > > > > > ---
> > > > > > /* 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)
> > > > > >  	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 |
> > > > > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device
> *vdev)
> > > > > >  	}
> > > > > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > > > > >
> > > > > > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
> &&
> > > > > > +	    (vi->guest_offloads_capable &
> GUEST_OFFLOAD_GRO_HW_MASK))
> > > > > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > > > > +
> > > > > >  	rtnl_unlock();
> > > > > >
> > > > > >  	err = virtnet_cpu_notif_add(vi);
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
> > > >
> > >
> >
> 

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

[Michael S. Tsirkin]

On Mon, Mar 16, 2026 at 01:57:22PM +0000, Zhud wrote:
>  > On Mon, Mar 16, 2026 at 12:57:00PM +0000, Zhud wrote:
> > > > On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > > > > > Thanks! Yes something to improve:
> > > > > >
> > > > > > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > 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?


> > 
> > > > > 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.
> > > Thank you for pointing that out.
> > >
> > > > > > >
> > > > > > > So, making NETIF_F_GRO_HW conditional on these feature bits
> > > > > > > ensures the stack does not enable an unsupported hardware
> > > > > > > offload
> > > > configuration.
> > > > > >
> > > > > > 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.
> > > >
> > > >
> > > > A bit more detail about the specific set of commands that leads to
> > > > confusion in the commit log would be helpful.
> > >
> > > > Thanks!
> > > >
> > > > > >
> > > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > > > > >
> > > > > > judging by this, has something to do with LRO?
> > > > > >
> > > > > > > ---
> > > > > > > /* 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)
> > > > > > >  	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 |
> > > > > > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device
> > *vdev)
> > > > > > >  	}
> > > > > > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > > > > > >
> > > > > > > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
> > &&
> > > > > > > +	    (vi->guest_offloads_capable &
> > GUEST_OFFLOAD_GRO_HW_MASK))
> > > > > > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > > > > > +
> > > > > > >  	rtnl_unlock();
> > > > > > >
> > > > > > >  	err = virtnet_cpu_notif_add(vi);
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > 
> 

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

[Zhud]

> On Mon, Mar 16, 2026 at 01:57:22PM +0000, Zhud wrote:
> >  > On Mon, Mar 16, 2026 at 12:57:00PM +0000, Zhud wrote:
> > > > > On Mon, Mar 16, 2026 at 10:18:04AM +0000, Zhud wrote:
> > > > > > > Thanks! Yes something to improve:
> > > > > > >
> > > > > > > On Mon, Mar 16, 2026 at 03:21:52PM +0800, Di Zhu wrote:
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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
> > > > > > > > vi->the hardware
> > > capability to perform GRO.
> > > > > > >
> > > > > > > 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.


	Thank you for the much clearer explanation of the problem. 
	It perfectly captures the intent. I will use this description for the v3 patch


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

	Yes, exactly. That is why I used GUEST_OFFLOAD_GRO_HW_MASK to
	maintain the current behavior.

> Also:
> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> 
> are you sure it's right?

	Fixes: dbcf24d15388 ("virtio-net: use NETIF_F_GRO_HW instead of NETIF_F_LRO") 
	Maybe the more accurate target. I will update the Fixes tag, thanks.

> 
> > >
> > > > > > 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.
> > > > Thank you for pointing that out.
> > > >
> > > > > > > >
> > > > > > > > So, making NETIF_F_GRO_HW conditional on these feature
> > > > > > > > bits ensures the stack does not enable an unsupported
> > > > > > > > hardware offload
> > > > > configuration.
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > >
> > > > > A bit more detail about the specific set of commands that leads
> > > > > to confusion in the commit log would be helpful.
> > > >
> > > > > Thanks!
> > > > >
> > > > > > >
> > > > > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable
> > > > > > > > LRO")
> > > > > > > > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > > > > > >
> > > > > > > judging by this, has something to do with LRO?
> > > > > > >
> > > > > > > > ---
> > > > > > > > /* 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)
> > > > > > > >  	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 |
> > > > > > > > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct
> > > > > > > > virtio_device
> > > *vdev)
> > > > > > > >  	}
> > > > > > > >  	vi->guest_offloads_capable = vi->guest_offloads;
> > > > > > > >
> > > > > > > > +	if (virtio_has_feature(vdev,
> > > > > > > > +VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
> > > &&
> > > > > > > > +	    (vi->guest_offloads_capable &
> > > GUEST_OFFLOAD_GRO_HW_MASK))
> > > > > > > > +		dev->hw_features |= NETIF_F_GRO_HW;
> > > > > > > > +
> > > > > > > >  	rtnl_unlock();
> > > > > > > >
> > > > > > > >  	err = virtnet_cpu_notif_add(vi);
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>