On Wed, 12 Dec 2018, Florian Westphal wrote:
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.
Thanks for looking at this. You're definitely right, there will be cases
where some CONFIG_SKB_EXTENSION users will be enabled and not others, and
that needs to be handled cleanly.
If you have a better idea, let me know.
I think the best thing is to keep the skb_ext_* functions you have, and
CONFIG_SKB_EXTENSION-dependent features like XFRM/BRIDGE_NF/etc can define
alternate inline functions or macros that call skb_ext_exist() if they
benefit from that.
--
Mat Martineau
Intel OTC
