[xdp-hints] Re: [PATCH bpf-next v2 15/20] net, xdp: allow metadata > 32

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Larysa Zaremba <larysa.zaremba@intel.com>
Date: Thu, 6 Jul 2023 16:51:22 +0200

> On Mon, Jul 03, 2023 at 02:06:46PM -0700, John Fastabend wrote:
>> Larysa Zaremba wrote:
>>> From: Aleksander Lobakin <aleksander.lobakin@intel.com>
>>>
>>> When using XDP hints, metadata sometimes has to be much bigger
>>> than 32 bytes. Relax the restriction, allow metadata larger than 32 bytes
>>> and make __skb_metadata_differs() work with bigger lengths.
>>>
>>> Now size of metadata is only limited by the fact it is stored as u8
>>> in skb_shared_info, so maximum possible value is 255. Other important
>>> conditions, such as having enough space for xdp_frame building, are already
>>> checked in bpf_xdp_adjust_meta().
>>>
>>> The requirement of having its length aligned to 4 bytes is still
>>> valid.
>>>
>>> Signed-off-by: Aleksander Lobakin <aleksander.lobakin@intel.com>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>> ---
>>>  include/linux/skbuff.h | 13 ++++++++-----
>>>  include/net/xdp.h      |  7 ++++++-
>>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 91ed66952580..cd49cdd71019 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -4209,10 +4209,13 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
>>>  {
>>>  	const void *a = skb_metadata_end(skb_a);
>>>  	const void *b = skb_metadata_end(skb_b);
>>> -	/* Using more efficient varaiant than plain call to memcmp(). */
>>> -#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>>
>> Why are we removing the ifdef here? Its adding a runtime 'if' when its not
>> necessary. I would keep the ifdef and simply add the default case
>> in the switch.
> 
> Seems like Alex has missed your message, but we discussed this with him before, 
> so I know the answer: Compiler will 100% convert it into a compile-time 'if' and 
> this looks nicer than preprocessor condition.

Sorry, I'm not always able to follow all the threads =\

As Larysa said, it's not a runtime `if`. Both conditions are always
known at compilation time.
And this looks a bit less ugly than with ifdefs to me :D

Thanks,
Olek