From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mat Martineau Subject: Re: [PATCH net-next 02/13] sk_buff: add skb extension infrastructure Date: Wed, 12 Dec 2018 08:59:50 -0800 (PST) Message-ID: References: <20181210145006.19098-1-fw@strlen.de> <20181210145006.19098-3-fw@strlen.de> <20181212001129.yz2pqukihzc3a6w4@breakpoint.cc> <20181212115941.z7zwckkfs72khtev@breakpoint.cc> Mime-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="0-1255709774-1544633115=:24938" Cc: netdev@vger.kernel.org To: Florian Westphal Return-path: Received: from mga09.intel.com ([134.134.136.24]:19903 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727681AbeLLQ7v (ORCPT ); Wed, 12 Dec 2018 11:59:51 -0500 In-Reply-To: <20181212115941.z7zwckkfs72khtev@breakpoint.cc> Content-ID: Sender: netdev-owner@vger.kernel.org List-ID: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1255709774-1544633115=:24938 Content-Type: text/plain; CHARSET=ISO-8859-15; format=flowed Content-Transfer-Encoding: 8BIT Content-ID: On Wed, 12 Dec 2018, Florian Westphal wrote: > Florian Westphal wrote: >> Mat Martineau 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. --0-1255709774-1544633115=:24938--