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

Parav Pandit
2025-01-26T06:24:12Z

> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Wednesday, January 22, 2025 12:10 AM
>
> On 1/20/25 3:19 PM, Parav Pandit wrote:
> >
> >> 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@n
> > vidia.com/T/#u
> >
> > Do we need to open a new voting request for it? yes?
>
> I'm sorry for the latency.
>
> Please be aware that VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO and
> VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM values are reported a 2nd
> time in the "Offloads State Configuration" paragraph. I guess even such chunk
> will need patching.
>
Offloads state is only 64-bit in size.
So I kept it as is to reuse the reserved feature bits range.
This way there wont be any conflict.

Posted the v1 https://lore.kernel.org/virtio-comment/20250126062058.13695-1-parav@nvidia.com/T/#u