On 28.01.26 14:18, Florian Westphal wrote:

Oliver Hartkopp [off-list ref] wrote:

quoted

quoted

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.

Ok. But then I don't see any real pressure to do the extension right 
now. There doesn't seem so much changes adding new skb extensions users. 
  And we still have a free slot even if all users would have been 
enabled (which is not the case due to the mutually exclusive options).

quoted

   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.

quoted

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?

The good thing is that skb extensions are very efficient. Which leads to 
the bad thing that we only can detect the problems at build time with

BUILD_BUG_ON(SKB_EXT_NUM > 8);
BUILD_BUG_ON(skb_ext_total_length() > 255);

For (SKB_EXT_NUM > 8) the upgrade of active_extensions to u16 should 
simply make it. Probably with some #ifdef magic only.

But thinking about BUILD_BUG_ON(skb_ext_total_length() > 255):

Shouldn't this be SKB_EXT_CHUNKSIZEOF(struct skb_ext) + sum of the skb 
ext user data which can be up to 255 *  SKB_EXT_ALIGN_VALUE (= 2040) ???

The offset calculation with the multiplication of SKB_EXT_ALIGN_VALUE 
(== 8) is the real magic in skb_ext_get_ptr(). therefore it should be 
something like

BUILD_BUG_ON(skb_ext_total_length() - SKB_EXT_CHUNKSIZEOF(struct 
skb_ext) > 255 *  SKB_EXT_ALIGN_VALUE)

right?

We could create a new BUILD_BUG_ON with all SKB_EXT_CHUNKSIZEOF'ed skb 
ext user data sizeof(structs) being < 255 * SKB_EXT_ALIGN_VALUE

IMO there's already plenty of space.

Best regards,
Oliver