RE: [PATCH v11 3/4] virtio-net: define UDP tunnel segmentation offload feature

Parav Pandit
2025-01-20T14:19:21Z

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, January 20, 2025 5:50 PM
>
> On Mon, Jan 20 2025, Parav Pandit <parav@nvidia.com> wrote:
>
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Monday, January 20, 2025 2:34 PM
> >>
> >> On Sun, Jan 19 2025, Parav Pandit <parav@nvidia.com> wrote:
> >>
> >> > Hi Paolo, Michael,
> >> >
> >> >> From: Paolo Abeni <pabeni@redhat.com>
> >> >> Sent: Wednesday, November 27, 2024 3:07 PM
> >> >>
> >> >>
> >> >> +\item[VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO (46)] Driver can
> receive
> >> >> GSO
> >> >> +packets
> >> >> + carried by a UDP tunnel.
> >> >> +
> >> >> +\item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO (48)] Device can
> receive
> >> >> GSO
> >> >> +packets
> >> >> + carried by a UDP tunnel.
> >> >
> >> > Feature bits 42 to 49 are not applicable to device specific type.
> >> > They are
> >> reserved.
> >> > Please see the note.
> >> >
> >> > Current spec description is:
> >> >
> >> > 0 to 23, and 50 to 127 Feature bits for the specific device type
> >> > 24 to 41 Feature bits reserved for extensions to the queue and
> >> > feature negotiation mechanisms, see 6
> >> > 42 to 49, and 128 and above Feature bits reserved for future extensions.
> >> >
> >> > So 2 bits of these patch and of patch 4, 46 to 49 to be shifted to
> >> > bits 65 to
> >> 68.
> >> >
> >> > Or an alternative would be to amend,
> >> >
> >> > Bits 46 to 127 for specific device type, And 42 to 45 and 128
> >> > reserved for future extensions.
> >> >
> >> > Michael,
> >> > Please let me know so that I can have the follow up patch to this
> >> > one as
> >> voting is started.
> >> > And the fix is required for it.
> >> >
> >> > Is there any historic reason for 42-49 as reserved, or it can be
> >> > used as
> >> proposed in this patch?
> >> > If it was only kept as elastic buffer for device generic, and
> >> > device type to
> >> grow it is probably ok.
> >> > Using 46 to 49 has small advantage to reuse in the offloads bit as below.
> >> > Please confirm.
> >>
> >> See the reasoning given in 88895f56e642 ("Reserve more feature bits
> >> for device type usage") -- the idea was to push out the point in time
> >> when generic feature bits will have to be in the bit area beyond 63.
> >> Given that we only used one of those generic bits in the meantime, I
> >> assume it will still be some time before we run out of bits, even if
> >> we let the net driver grab some bits there. Still, I'd probably prefer that the
> net device used different bits.
> >> [And issues like this are easy to miss during review unfortunately.]
> >>
> > I made assumption that one would have picked the device specific rage.
> > I missed to detect this early enough, my bad. :(
> >
> > Given that we are only left with 4 bits in range 42 to 45, at generic level, it
> seems wise for device-type to consume feature bits in its defined range.
> > For virtio-net, bits 65 and higher are preferred.
> >
> >> However, if the device was to use different bits, the clean solution
> >> would be to withdraw, respin, and revote. If we only changed the
> >> reserved ranges, we'd be fine with a change on top, so that would be
> >> less hassle in the short team. If we decide that we can live with the
> >> smaller feature bit space below 63, I'd be fine with that solution.
> >>
> >
> > I am fine either way to merge this and fix immediately for using bits 65 and
> higher.
> > My preference is to use bits 65 and higher.
> >
> > Or as you suggested to respin and revote right away to forward with bit 65.
> >
> > Please let me know.
>
> Personally, I'd prefer to handle this in an "atomic" manner, i.e. making sure
> that no version ever has the wrong bit numbers... maybe if you did a fixup
> patch that changes the numbers, and commit these patches + the fixup all at
> once? Less churn than doing the respin and revote dance.
>
Here it is.
https://lore.kernel.org/virtio-comment/20250120141052.877355-1-parav@nvidia.com/T/#u

Do we need to open a new voting request for it? yes?

> >
> >> >
> >> >
> >> >> @@ -1862,6 +2068,7 @@ \subsubsection{Control
> >> >> Virtqueue}\label{sec:Device Types / Network Device / Devi
> >> >> #define VIRTIO_NET_F_GUEST_TSO6 8
> >> >> #define VIRTIO_NET_F_GUEST_ECN 9
> >> >> #define VIRTIO_NET_F_GUEST_UFO 10
> >> >> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 46
> >> >> #define VIRTIO_NET_F_GUEST_USO4 54
> >> >> #define VIRTIO_NET_F_GUEST_USO6 55