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

Paolo Abeni
2025-01-21T18:40:13Z

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@nvidia.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.

/P