| Message ID | 20201116031715.7891-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 9606 this patch: 9606 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 48 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 9975 this patch: 9975 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Hi Randy, On 16/11/2020 04:17, Randy Dunlap wrote: > The previous Kconfig patch led to some other build errors as > reported by the 0day bot and my own overnight build testing. > > These are all in <linux/skbuff.h> when KCOV is enabled but > SKB_EXTENSIONS is not enabled, so fix those by combining those conditions > in the header file. > > Also, add stubs for skb_ext_add() and skb_ext_find() to reduce the > amount of ifdef-ery. (Jakub) It makes sense, good idea! Thank you for the new version! > --- linux-next-20201113.orig/include/linux/skbuff.h > +++ linux-next-20201113/include/linux/skbuff.h > @@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s > #endif > } > > -#ifdef CONFIG_SKB_EXTENSIONS > enum skb_ext_id { > #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > SKB_EXT_BRIDGE_NF, > @@ -4151,12 +4150,11 @@ enum skb_ext_id { > #if IS_ENABLED(CONFIG_MPTCP) > SKB_EXT_MPTCP, > #endif > -#if IS_ENABLED(CONFIG_KCOV) > SKB_EXT_KCOV_HANDLE, > -#endif I don't think we should remove this #ifdef: the number of extensions are currently limited to 8, we might not want to always have KCOV there even if we don't want it. I think adding items in this enum only when needed was the intension of Florian (+cc) when creating these SKB extensions. Also, this will increase a tiny bit some structures, see "struct skb_ext()". But apart from that, I think we are fine, even if we add new extensions in the future after this kcov one. So if we think it is better to remove these #ifdef here, we should be OK. But if we prefer not to do that, we should then not add stubs for skb_ext_{add,find}() and keep the ones for skb_[gs]et_kcov_handle(). Cheers, Matt
Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > --- linux-next-20201113.orig/include/linux/skbuff.h > > +++ linux-next-20201113/include/linux/skbuff.h > > @@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s > > #endif > > } > > -#ifdef CONFIG_SKB_EXTENSIONS > > enum skb_ext_id { > > #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > > SKB_EXT_BRIDGE_NF, > > @@ -4151,12 +4150,11 @@ enum skb_ext_id { > > #if IS_ENABLED(CONFIG_MPTCP) > > SKB_EXT_MPTCP, > > #endif > > -#if IS_ENABLED(CONFIG_KCOV) > > SKB_EXT_KCOV_HANDLE, > > -#endif > > I don't think we should remove this #ifdef: the number of extensions are > currently limited to 8, we might not want to always have KCOV there even if > we don't want it. I think adding items in this enum only when needed was the > intension of Florian (+cc) when creating these SKB extensions. > Also, this will increase a tiny bit some structures, see "struct skb_ext()". Yes, I would also prefer to retrain the ifdef. Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives a compile error if the extension is not enabled. > So if we think it is better to remove these #ifdef here, we should be OK. > But if we prefer not to do that, we should then not add stubs for > skb_ext_{add,find}() and keep the ones for skb_[gs]et_kcov_handle(). Yes, exactly, I did not add these stubs because I could not figure out a case where an empty skb_ext_{add,find} would be wanted. If your code calls skb_ext_add() but no skb extensions exist you forgot a SELECT/DEPENDS SKB_EXTENSIONS in Kconfig & compiler error would tell you that.
On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote: > > > @@ -4151,12 +4150,11 @@ enum skb_ext_id { > > > #if IS_ENABLED(CONFIG_MPTCP) > > > SKB_EXT_MPTCP, > > > #endif > > > -#if IS_ENABLED(CONFIG_KCOV) > > > SKB_EXT_KCOV_HANDLE, > > > -#endif > > > > I don't think we should remove this #ifdef: the number of extensions are > > currently limited to 8, we might not want to always have KCOV there even if > > we don't want it. I think adding items in this enum only when needed was the > > intension of Florian (+cc) when creating these SKB extensions. > > Also, this will increase a tiny bit some structures, see "struct skb_ext()". > > Yes, I would also prefer to retrain the ifdef. > > Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives > a compile error if the extension is not enabled. Oh well, sorry for taking you down the wrong path Randy!
On 11/16/20 7:30 AM, Jakub Kicinski wrote: > On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote: >>>> @@ -4151,12 +4150,11 @@ enum skb_ext_id { >>>> #if IS_ENABLED(CONFIG_MPTCP) >>>> SKB_EXT_MPTCP, >>>> #endif >>>> -#if IS_ENABLED(CONFIG_KCOV) >>>> SKB_EXT_KCOV_HANDLE, >>>> -#endif >>> >>> I don't think we should remove this #ifdef: the number of extensions are >>> currently limited to 8, we might not want to always have KCOV there even if >>> we don't want it. I think adding items in this enum only when needed was the >>> intension of Florian (+cc) when creating these SKB extensions. >>> Also, this will increase a tiny bit some structures, see "struct skb_ext()". >> >> Yes, I would also prefer to retrain the ifdef. >> >> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives >> a compile error if the extension is not enabled. > > Oh well, sorry for taking you down the wrong path Randy! No problem. So we are back to v2, right? Do I need to resend that one? thanks.
Randy Dunlap <rdunlap@infradead.org> wrote: > On 11/16/20 7:30 AM, Jakub Kicinski wrote: > > On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote: > >>>> @@ -4151,12 +4150,11 @@ enum skb_ext_id { > >>>> #if IS_ENABLED(CONFIG_MPTCP) > >>>> SKB_EXT_MPTCP, > >>>> #endif > >>>> -#if IS_ENABLED(CONFIG_KCOV) > >>>> SKB_EXT_KCOV_HANDLE, > >>>> -#endif > >>> > >>> I don't think we should remove this #ifdef: the number of extensions are > >>> currently limited to 8, we might not want to always have KCOV there even if > >>> we don't want it. I think adding items in this enum only when needed was the > >>> intension of Florian (+cc) when creating these SKB extensions. > >>> Also, this will increase a tiny bit some structures, see "struct skb_ext()". > >> > >> Yes, I would also prefer to retrain the ifdef. > >> > >> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives > >> a compile error if the extension is not enabled. > > > > Oh well, sorry for taking you down the wrong path Randy! > > No problem. > So we are back to v2, right? Yes, you can still drop the line >> +#if IS_ENABLED(CONFIG_KCOV) && IS_ENABLED(CONFIG_SKB_EXTENSIONS) for enum skb_ext_id (alreadyt under SKB_EXTENSIONS). Other than that v2 looks good to me. Thanks!
--- linux-next-20201113.orig/include/linux/skbuff.h +++ linux-next-20201113/include/linux/skbuff.h @@ -4137,7 +4137,6 @@ static inline void skb_set_nfct(struct s #endif } -#ifdef CONFIG_SKB_EXTENSIONS enum skb_ext_id { #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) SKB_EXT_BRIDGE_NF, @@ -4151,12 +4150,11 @@ enum skb_ext_id { #if IS_ENABLED(CONFIG_MPTCP) SKB_EXT_MPTCP, #endif -#if IS_ENABLED(CONFIG_KCOV) SKB_EXT_KCOV_HANDLE, -#endif SKB_EXT_NUM, /* must be last */ }; +#ifdef CONFIG_SKB_EXTENSIONS /** * struct skb_ext - sk_buff extensions * @refcnt: 1 on allocation, deallocated on 0 @@ -4252,6 +4250,10 @@ static inline void skb_ext_del(struct sk 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) {} static inline bool skb_has_extensions(struct sk_buff *skb) { return false; } +static inline void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id) +{ return NULL; } +static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id) +{ return NULL; } #endif /* CONFIG_SKB_EXTENSIONS */ static inline void nf_reset_ct(struct sk_buff *skb) @@ -4608,7 +4610,6 @@ static inline void skb_reset_redirect(st #endif } -#ifdef CONFIG_KCOV static inline void skb_set_kcov_handle(struct sk_buff *skb, const u64 kcov_handle) { @@ -4632,11 +4633,6 @@ static inline u64 skb_get_kcov_handle(st return kcov_handle ? *kcov_handle : 0; } -#else -static inline void skb_set_kcov_handle(struct sk_buff *skb, - const u64 kcov_handle) { } -static inline u64 skb_get_kcov_handle(struct sk_buff *skb) { return 0; } -#endif /* CONFIG_KCOV */ #endif /* __KERNEL__ */ #endif /* _LINUX_SKBUFF_H */
The previous Kconfig patch led to some other build errors as reported by the 0day bot and my own overnight build testing. These are all in <linux/skbuff.h> when KCOV is enabled but SKB_EXTENSIONS is not enabled, so fix those by combining those conditions in the header file. Also, add stubs for skb_ext_add() and skb_ext_find() to reduce the amount of ifdef-ery. (Jakub) Fixes: 6370cc3bbd8a ("net: add kcov handle to skb extensions") Fixes: 85ce50d337d1 ("net: kcov: don't select SKB_EXTENSIONS when there is no NET") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: kernel test robot <lkp@intel.com> Cc: Aleksandr Nogikh <nogikh@google.com> Cc: Willem de Bruijn <willemb@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: linux-next@vger.kernel.org Cc: netdev@vger.kernel.org Cc: Matthieu Baerts <matthieu.baerts@tessares.net> --- v4: The enum for SKB_EXT_KCOV_HANDLE needs to be exposed unconditionally because it is used in skb_get/set_kcov_handle(), which are always present since v3. v3: (as suggested by Jakub Kicinski <kuba@kernel.org>) add stubs for skb_ext_add() and skb_ext_find() to reduce the ifdef-ery v2: (as suggested by Matthieu Baerts <matthieu.baerts@tessares.net>) drop an extraneous space in a comment; use CONFIG_SKB_EXTENSIONS instead of CONFIG_NET; include/linux/skbuff.h | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)