Re: [PATCH v9 1/2] virtio-net: define UDP tunnel segmentation offload feature

[Paolo Abeni <pabeni@redhat.com>]

On 10/11/24 04:08, Jason Wang wrote:
> I wonder why not just introduce new fields for the inner, like what
> has beed done in the linux sk_buff implementation? I know I propose
> this before, but now I think it can help to reduce the complexity and
> more aligned with the for non GSO tunneling.
> 
> ...
>          union {
>                  __be16          inner_protocol;
>                  __u8            inner_ipproto;
>          };
> ...
>          __u16                   inner_transport_header;
>          __u16                   inner_network_header;
>          __u16                   inner_mac_header;
> 
>          __be16                  protocol;
>          __u16                   transport_header;
>          __u16                   network_header;
> ...

As the discussion touches many different points, please allow me 
wrap-up, in order of increasing conflictuality:

1) there are a few points clarification requests which are actually not 
needed, because the asked text is already there in the spec.

2) the DATA_VALID handling proposed in v9 is bad, let me scratch it for now.

3) you feel the handling of csum_start/csum_offset is confusing and 
would prefer adding new fields for the inner headers.

Let's try to set this most critical point first. Adding new fields for 
the inner headers has several dis-advantages:

- we will need to add more new fields WRT to the current proposal. Using 
csum_start/csum_offset for the outer we will need:

   inner_nh
   inner_th
   inner_offset

instead of just outer_th, inner_nh

- AFAICS, due to align reason, the above will require an additional 
<unused> field.

- the implementation will not be able to reuse the existing code, 
leading to duplication and/or hacks.

- will discard most of the discussions on this topics done in the past 
several revisions

I understand that one of the argument in favor of using new fields for 
the inner headers is to align with the current linux kernel handling of 
tunnel related fields. Please note that the existence of skb->inner_* 
fields is quite blamed, I suggest avoiding taking them as a reference.

AFAICS the main argument against the layout I proposed is the 
inconsistent usage of csum_start that will refer to the inner header for 
GSO over udp-tunnel packets and to the outer header in when the device 
does not expose the GSO over UDP tunnel support.

I argue that dubbing such behavior as inconsistent is a matter of 
interpretation: in both case the csum_start field refers to the 
innermost header that the device is able to parse.

Finally, the specification change to describe the new field usage I 
propose is quite minimal and almost don't touch the existing definition: 
it's difficult for alternative specification being simpler - especially 
when such alternative specification will need more new fields to be 
described.

Cheers,

Paolo