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