Re: [net-next 0/6] move CAN skb headroom content to skb extensions

[Florian Westphal <fw@strlen.de>]

Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > When the first extensions were added all of them could be enabled
> > at same time, but I think that has changed.
> 
> IMO we do not need to 'union' extensions as long as automatic enum 
> calculation does it job with the enabled Kconfig options.
>
> My only concern would be distribution kernels that have an all-yes 
> config policy ;-)

Well, thats the norm, no?  Allmodconfig.
So we have two issues:
1. Turn active_extensions in sk_buff into u16
2. Growing memory usage of the skb_ext blob.

No need to add this 'union' now of course, but I think its something
that should be kept in mind: originally, all extensions could be
turned on for the same skb, but it looks like we now have mutually
exclusive ones.

> With my patch set the enum would now look like this:
> 
> #ifdef CONFIG_SKB_EXTENSIONS
> enum skb_ext_id {
> #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>          SKB_EXT_BRIDGE_NF,
> #endif
> #ifdef CONFIG_XFRM
>          SKB_EXT_SEC_PATH,
> #endif
> #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
>          TC_SKB_EXT,
> #endif
> #if IS_ENABLED(CONFIG_MPTCP)
>          SKB_EXT_MPTCP,
> #endif
> #if IS_ENABLED(CONFIG_MCTP_FLOWS)
>          SKB_EXT_MCTP,
> #endif
> #if IS_ENABLED(CONFIG_INET_PSP)
>          SKB_EXT_PSP,
> #endif
> #if IS_ENABLED(CONFIG_CAN)
>          SKB_EXT_CAN,
> #endif
>          SKB_EXT_NUM, /* must be last */
> };
> 
> => SKB_EXT_NUM is then 7
> 
> When we (correctly) add another extension, SKB_EXT_NUM would become 8 
> which is still fine IMO. But then the BUILD_BUG_ON check in 
> skb_extensions_init() would need the below fix, right?
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 648c20e19038..609851d70173 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5156,11 +5156,11 @@ static __always_inline unsigned int 
> skb_ext_total_length(void)
>          return l;
>   }
> 
>   static void skb_extensions_init(void)
>   {
> -       BUILD_BUG_ON(SKB_EXT_NUM >= 8);
> +       BUILD_BUG_ON(SKB_EXT_NUM > 8);

True, the last valid extension id can't be 8, but
SKB_EXT_NUM could be.

> Should I send a proper patch?

You can, otoh I think we should have consensus on what
to do when the 8th extension is added, do we just s/u8/u16' and
eat the growing memory cost for the skb_ext growth, or do we go
to a drawing board and figure out how to best merge mutually exclusive
extensions to save space?