Florian Westphal <f...@strlen.de> wrote:
> Mat Martineau <mathew.j.martin...@linux.intel.com> wrote:
> > > +#ifdef CONFIG_SKB_EXTENSIONS
> > > +enum skb_ext_id {
> > > +#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> > > + SKB_EXT_BRIDGE_NF,
> > > +#endif
> > > + SKB_EXT_NUM, /* must be last */
> > > +};
> >
> > Could enum skb_ext_id always be defined, with none of the SKB_EXT_* values
> > conditionally excluded?
>
> Yes, this is certainly possible.
Did this today, did not like the result (see below).
> > In combination with some alternate function
> > definitions below (see later comments) I think this could reduce the need
> > for CONFIG_SKB_EXTENSIONS preprocessor conditionals throughout the net code.
>
> Perhaps. I'll give this a shot.
>
> > > +static inline void skb_ext_del(struct sk_buff *skb, int unused) {}
> > > +static inline void __skb_ext_copy(struct sk_buff *d, const struct
> > > sk_buff *s) {}
> > > +static inline void skb_ext_copy(struct sk_buff *dst, const struct
> > > sk_buff *s) {}
> >
> > For the !CONFIG_SKB_EXTENSIONS case, an alternate definition of
> > skb_ext_exist() that always returns false would be useful to reduce the need
> > for preprocessor conditionals. A similar skb_ext_find() that always returns
> > NULL might also be helpful.
>
> I'll do this and double-check that gcc removes the branches accordingly.
Problem is that the alternate skb_ext_exist() becomes bloated, and
instead gets the #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)/XFRM, etc.
junk.
Otherwise, gcc can't remove the branch in case e.g. CONFIG_XFRM=y
BRIDGE_NF=n.
With current approach, SKB_EXT_BRIDGE_NF id is hidden, i.e. all
calls to skb_ext_add/exists/del(, SKB_EXT_BRIDGE_NF) result in a build
error unless they appear in bridge-netfilter-only sections of the code
(this is intentional).
So, no extra code is generated.
The empty stubs are there only in case no subsystem requires the
extension infrastructure, and in that case, there should be no calls
to skb_ext_exist() in the first place.
Gist is that CONFIG_SKB_EXTENSIONS=y can still mean that all the
'remove bridge' or 'add secpath' etc. calls are not needed, but thats
something the compiler can't know unless called function explicitly
considers this in the inlined section.
Unfortunately, the existing 'id not defined in enum' seems the best
approach to ensure that.
If you have a better idea, let me know.
