Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32

[Alexander Lobakin]

From: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
Date: Mon, 22 May 2023 13:41:43 +0200

> 
> 
> On 19/05/2023 18.35, Alexander Lobakin wrote:
>> From: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
>> Date: Tue, 16 May 2023 17:35:27 +0200

[...]

> Not talking about your changes (in this patch).
> 
> I'm realizing that SKBs using metadata area will have a performance hit
> due to accessing another cacheline (the meta_len in skb_shared_info).
> 
> IIRC Daniel complained about this performance hit (in the past), I guess
> this explains it.  IIRC Cilium changed to use percpu variables/datastore
> to workaround this.

Why should we compare metadata of skbs on GRO anyway? I was disabling it
the old hints series (conditionally, if driver asks), moreover...
...if metadata contains full checksum, GRO will be broken completely due
to this comparison (or any other frame-unique fields. VLAN tags and
hashes are okay).

> 
> 
>> The whole xdp_metalen_invalid() gets expanded into:
>>
>>     return (metalen % 4) || metalen > 255;
>>
>> at compile-time. All those typeof shenanigans are only to not open-code
>> meta_len's type/size/max.
>>
>>>
>>> But only use for SKBs that gets created from xdp with metadata, right?
>>>
> 
> Normal netstack processing actually access this skb_shinfo->meta_len in
> gro_list_prepare().  As the caller dev_gro_receive() later access other
> memory in skb_shared_info, then the GRO code path already takes this hit
> to begin with.

You access skb_shinfo() often even before running XDP program, for
example, when a frame is multi-buffer. Plus HW timestamps are also
there, and so on.

> 
> --Jesper
> 

Thanks,
Olek