Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: alexander.h.duyck@intel.com,
	Virtio-Dev <virtio-dev@lists.oasis-open.org>,
	kubakici@wp.pl, sridhar.samudrala@intel.com,
	jesse.brandeburg@intel.com, gavi@nvidia.com,
	virtualization <virtualization@lists.linux-foundation.org>,
	"Hemminger, Stephen" <stephen@networkplumber.org>,
	loseweigh@gmail.com, davem <davem@davemloft.net>,
	Gavin Li <gavinl@nvidia.com>
Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets
Date: Tue, 9 Aug 2022 05:22:13 -0400	[thread overview]
Message-ID: <20220809051849-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <CACGkMEsVG_62yJ5nGmp5ufF_xiEJk2TezQz-QFftF8ezKYLB8A@mail.gmail.com>

On Tue, Aug 09, 2022 at 03:44:22PM +0800, Jason Wang wrote:
> > @@ -3571,8 +3581,10 @@ 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) ||
> >           virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
> > -         virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
> > +         virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO)) {
> >               vi->big_packets = true;
> > +             vi->gso_is_supported = true;
> >
> > Please do the same for virtnet_clear_guest_offloads(), and
> > correspondingly virtnet_restore_guest_offloads() as well. Not sure why
> > virtnet_clear_guest_offloads() or the caller doesn't unset big_packet on
> > successful return, seems like a bug to me.
> 
> It is fine as long as
> 
> 1) we don't implement ethtool API for changing guest offloads
> 2) big mode XDP is not enabled
> 
> So that code works only for XDP but we forbid big packets in the case
> of XDP right now.
> 
> Thanks

To put it another way, changing big_packets after probe requires a bunch
of work as current code assumes this flag never changes.
Adding a TODO to handle dynamic offload config is fine but
I don't think it should block this.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization