> 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 <[email protected]>
> > > > > > >
> > > > > > > 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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
