Re: [PATCH v8 0/2] virtio-net: define UDP tunnel offload

[Paolo Abeni <pabeni@redhat.com>]

On 10/1/24 15:09, Willem de Bruijn wrote:
> On Mon, Sep 30, 2024 at 3:36 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 9/9/24 10:05, Paolo Abeni wrote:
>>> On 9/9/24 05:16, Jason Wang wrote:
>>>> On Wed, Sep 4, 2024 at 11:50 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>>>
>>>>> UDP tunnel usage is ubiquitous in container deployment, and the ability
>>>>> to offload UDP encapsulated GSO traffic impacts greatly the performances
>>>>> and the CPU utilization of such use cases.
>>>>>
>>>>> This series introduces separate features to handle both UDP tunnel
>>>>> segmentation offload (patch 1) and UDP tunnel outer checksum offload
>>>>> (patch 2).
>>>>
>>>> Looks good to me.
>>>>
>>>> And I wonder if there's a link to the prototype code so I can have a
>>>> double check.
>>>
>>> I still have to refresh the code from first implementation - that was
>>> the base for the first draft here some time ago.
>>>
>>> In the next 2 weeks I'll be traveling and/or AFK, I hope I can share the
>>> code a little after that.
>>
>> A very early PoC is available here:
>>
>> https://github.com/pabeni/linux-devel/commits/virtio_tunnel_gso/
>>
>> It's very lightly tested vs a patched user-space:
>>
>> https://github.com/pabeni/qemu/commits/virtio_tunnel/
>>
>> "very lightly" is actually a strong understatement; the thing survived a
>> single iperf-over-udp-tunnel run, and that is. However I hope it should
>> suffice as showcase.
>>
>> In the tun/user-space interface I used a separate ioctl to configure the
>> tunnel offload instead of re-using the existing ioctl for plain GSO,
>> because AFAICS the tun driver is currently unaware of the complete
>> virtio net hdr layout (e.g. ignores rx hash and VIRTIO_NET_F_MRG_RXBUF)
>> and it is just notified of the total hdr struct size.
>>
>> With UDP tunnel offload enabled, the tun driver need to know the tunnel
>> fields offset inside the virtio net hdr. Piggybacking such info inside
>> the the current ioctl(TUNSETOFFLOAD) data (a single long int) is
>> feasible but looks hackish. Instead I preferred implementing a separate
>> ioctl(TUNSETTNLOFFLOAD) receiving as input from user-space a struct
>> containing the mentioned offset and the "csum offload enabled" flag
>>
>> Any comments/suggestion WRT such interface more than welcome!
>>
>> The PoC demonstrated that the pending spec changes need some fixlet:
>>
>> - I wrote the patch on top of the current virtio-spec master branch, I
>> need to rebase on top of virtio-1.4, I guess.
>>
>> - I underlooked to the VIRTIO_NET_HDR_F_DATA_VALID flag. The
>> specification change allows GSO over UDP tunnel offload with DATA_VALID,
>> but GSO over UDP tunnel needs the inner transport protocol information.
>> This point looks more difficult and potentially controversial.
>>
>> With the proposed change, when DATA_VALID is set, the implementation
>> will have to determine the inner transport header location via parsing,
>> as currently is doing for plain GSO. Note that the PoC is currently
>> bugged WRT this scenario.
>>
>> I'm wondering if we could revisit the spec change requiring that both
>> the driver and the device, for GSO over UDP tunnel packets only, will
>> fill the csum_start field when either the NEEDS_CSUM or the DATA_VALID
>> bits are set.
> 
> This SGTM.
> 
>> Equivalently, for GSO over UDP tunnel packets only, the
>> driver and the device, to specify CHECKSUM_UNNECESSARY packets, could
>> set both the NEEDS_CSUM and DATA_VALID, with the latter flag 'taking
>> precedence' WRT the actual checksum type.
> 
> Why do both need to be set to signal CHECKSUM_UNNECESSARY, rather
> than only DATA_VALID?

I think it's mostly a matter of preferences, and 'allowed delta' WRT to 
the existing offload.

AFAICS, technically the plain GSO spec don't mandate that NEEDS_CSUM and 
DATA_VALID as mutually exclusive. Instead the spec require the 
NEEDS_CSUM flag to use csum_* fields: the first option would create a 
more significant difference WRT plain GSO.

 From an implementation PoV, the 2nd option (both NEEDS_CSUM and 
NEEDS_CSUM) would allow for simpler integration/reuse of the existing 
code: the UDP tunnel GSO 'to_skb()' helper could directly call the plain 
GSO one and eventually later just set CHECKSUM_UNNECESSARY as needed.

Thanks for the feedback,

Paolo