[PATCH net-next 0/5] net: skbuff: hide some bitfield members

[PATCH net-next 0/5] net: skbuff: hide some bitfield members

[Jakub Kicinski]

There is a number of protocol or subsystem specific fields
in struct sk_buff which are only accessed by one subsystem.
We can wrap them in ifdefs with minimal code impact.

This gives us a better chance to save a 2B and a 4B holes
resulting with the following savings (assuming a lucky
kernel config):

-	/* size: 232, cachelines: 4, members: 28 */
-	/* sum members: 227, holes: 1, sum holes: 4 */
-	/* sum bitfield members: 8 bits (1 bytes) */
+	/* size: 224, cachelines: 4, members: 28 */
 	/* forced alignments: 2 */
-	/* last cacheline: 40 bytes */
+	/* last cacheline: 32 bytes */

I think that the changes shouldn't be too controversial.
The only one I'm not 100% sure of is the SCTP one,
12 extra LoC for one bit.. But it did fit squarely
in the "this bit has only one user" category.

Jakub Kicinski (5):
  net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set
  net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP not set
  net: skbuff: move alloc_cpu into a potential hole
  net: skbuff: push nf_trace down the bitfield
  net: skbuff: hide nf_trace and ipvs_property

 include/linux/skbuff.h | 38 ++++++++++++++++++++++++++++++++++----
 include/net/sock.h     |  2 +-
 net/core/dev.c         |  3 +--
 net/core/skbuff.c      |  2 ++
 net/sched/act_csum.c   |  3 +--
 net/socket.c           |  2 ++
 6 files changed, 41 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH net-next 1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set

[Jakub Kicinski]

Datacenter kernel builds will very likely not include WIRELESS,
so let them shave 2 bits off the skb by hiding the wifi fields.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: johannes@sipsolutions.net
---
 include/linux/skbuff.h | 11 +++++++++++
 include/net/sock.h     |  2 +-
 net/core/skbuff.c      |  2 ++
 net/socket.c           |  2 ++
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 494a23a976b0..7160101edc8a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -953,8 +953,10 @@ struct sk_buff {
 
 	__u8			l4_hash:1;
 	__u8			sw_hash:1;
+#ifdef CONFIG_WIRELESS
 	__u8			wifi_acked_valid:1;
 	__u8			wifi_acked:1;
+#endif
 	__u8			no_fcs:1;
 	/* Indicates the inner headers are valid in the skbuff. */
 	__u8			encapsulation:1;
@@ -1187,6 +1189,15 @@ static inline unsigned int skb_napi_id(const struct sk_buff *skb)
 #endif
 }
 
+static inline bool skb_wifi_acked_valid(const struct sk_buff *skb)
+{
+#ifdef CONFIG_WIRELESS
+	return skb->wifi_acked_valid;
+#else
+	return 0;
+#endif
+}
+
 /**
  * skb_unref - decrement the skb's reference count
  * @skb: buffer
diff --git a/include/net/sock.h b/include/net/sock.h
index 5edf0038867c..8b7ed7167243 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2697,7 +2697,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
 	else
 		sock_write_timestamp(sk, kt);
 
-	if (sock_flag(sk, SOCK_WIFI_STATUS) && skb->wifi_acked_valid)
+	if (sock_flag(sk, SOCK_WIFI_STATUS) && skb_wifi_acked_valid(skb))
 		__sock_recv_wifi_status(msg, sk, skb);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 78238a13dbcf..856926d2837e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5187,6 +5187,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 }
 EXPORT_SYMBOL_GPL(skb_tstamp_tx);
 
+#ifdef CONFIG_WIRELESS
 void skb_complete_wifi_ack(struct sk_buff *skb, bool acked)
 {
 	struct sock *sk = skb->sk;
@@ -5212,6 +5213,7 @@ void skb_complete_wifi_ack(struct sk_buff *skb, bool acked)
 		kfree_skb(skb);
 }
 EXPORT_SYMBOL_GPL(skb_complete_wifi_ack);
+#endif /* CONFIG_WIRELESS */
 
 /**
  * skb_partial_csum_set - set up and verify partial csum values for packet
diff --git a/net/socket.c b/net/socket.c
index 73e493da4589..a7b4b37d86df 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -957,6 +957,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 }
 EXPORT_SYMBOL_GPL(__sock_recv_timestamp);
 
+#ifdef CONFIG_WIRELESS
 void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
 	struct sk_buff *skb)
 {
@@ -972,6 +973,7 @@ void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
 	put_cmsg(msg, SOL_SOCKET, SCM_WIFI_STATUS, sizeof(ack), &ack);
 }
 EXPORT_SYMBOL_GPL(__sock_recv_wifi_status);
+#endif
 
 static inline void sock_recv_drops(struct msghdr *msg, struct sock *sk,
 				   struct sk_buff *skb)
-- 
2.39.2

[PATCH net-next 2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP not set

[Jakub Kicinski]

SCTP is not universally deployed, allow hiding its bit
from the skb.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/skbuff.h | 14 ++++++++++++++
 net/core/dev.c         |  3 +--
 net/sched/act_csum.c   |  3 +--
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7160101edc8a..45c3044e8123 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -983,7 +983,9 @@ struct sk_buff {
 	__u8			decrypted:1;
 #endif
 	__u8			slow_gro:1;
+#if IS_ENABLED(CONFIG_IP_SCTP)
 	__u8			csum_not_inet:1;
+#endif
 
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */
@@ -5054,7 +5056,19 @@ static inline void skb_reset_redirect(struct sk_buff *skb)
 
 static inline bool skb_csum_is_sctp(struct sk_buff *skb)
 {
+#if IS_ENABLED(CONFIG_IP_SCTP)
 	return skb->csum_not_inet;
+#else
+	return 0;
+#endif
+}
+
+static inline void skb_reset_csum_not_inet(struct sk_buff *skb)
+{
+	skb->ip_summed = CHECKSUM_NONE;
+#if IS_ENABLED(CONFIG_IP_SCTP)
+	skb->csum_not_inet = 0;
+#endif
 }
 
 static inline void skb_set_kcov_handle(struct sk_buff *skb,
diff --git a/net/core/dev.c b/net/core/dev.c
index c7f13742b56c..177e91819850 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3315,8 +3315,7 @@ int skb_crc32c_csum_help(struct sk_buff *skb)
 						  skb->len - start, ~(__u32)0,
 						  crc32c_csum_stub));
 	*(__le32 *)(skb->data + offset) = crc32c_csum;
-	skb->ip_summed = CHECKSUM_NONE;
-	skb->csum_not_inet = 0;
+	skb_reset_csum_not_inet(skb);
 out:
 	return ret;
 }
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 95e9304024b7..8ed285023a40 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -376,8 +376,7 @@ static int tcf_csum_sctp(struct sk_buff *skb, unsigned int ihl,
 
 	sctph->checksum = sctp_compute_cksum(skb,
 					     skb_network_offset(skb) + ihl);
-	skb->ip_summed = CHECKSUM_NONE;
-	skb->csum_not_inet = 0;
+	skb_reset_csum_not_inet(skb);
 
 	return 1;
 }
-- 
2.39.2

[PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole

[Jakub Kicinski]

alloc_cpu is currently between 4 byte fields, so it's almost
guaranteed to create a 2B hole. It has a knock on effect of
creating a 4B hole after @end (and @end and @tail being in
different cachelines).

None of this matters hugely, but for kernel configs which
don't enable all the features there may well be a 2B hole
after the bitfield. Move alloc_cpu there.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/skbuff.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 45c3044e8123..fd6344aca94a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -991,6 +991,8 @@ struct sk_buff {
 	__u16			tc_index;	/* traffic control index */
 #endif
 
+	u16			alloc_cpu;
+
 	union {
 		__wsum		csum;
 		struct {
@@ -1014,7 +1016,6 @@ struct sk_buff {
 		unsigned int	sender_cpu;
 	};
 #endif
-	u16			alloc_cpu;
 #ifdef CONFIG_NETWORK_SECMARK
 	__u32		secmark;
 #endif
-- 
2.39.2

[PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield

[Jakub Kicinski]

nf_trace is a debug feature, AFAIU, and yet it sits oddly
high in the sk_buff bitfield. Move it down, pushing up
dst_pending_confirm and inner_protocol_type.

Next change will make nf_trace optional (under Kconfig)
and all optional fields should be placed after 2b fields
to avoid 2b fields straddling bytes.

dst_pending_confirm is L3, so it makes sense next to ignore_df.
inner_protocol_type goes up just to keep the balance.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: pablo@netfilter.org
CC: fw@strlen.de
---
 include/linux/skbuff.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index fd6344aca94a..543f7ae9f09f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -934,7 +934,7 @@ struct sk_buff {
 	/* public: */
 	__u8			pkt_type:3; /* see PKT_TYPE_MAX */
 	__u8			ignore_df:1;
-	__u8			nf_trace:1;
+	__u8			dst_pending_confirm:1;
 	__u8			ip_summed:2;
 	__u8			ooo_okay:1;
 
@@ -949,7 +949,7 @@ struct sk_buff {
 	__u8			remcsum_offload:1;
 	__u8			csum_complete_sw:1;
 	__u8			csum_level:2;
-	__u8			dst_pending_confirm:1;
+	__u8			inner_protocol_type:1;
 
 	__u8			l4_hash:1;
 	__u8			sw_hash:1;
@@ -967,7 +967,7 @@ struct sk_buff {
 #endif
 
 	__u8			ipvs_property:1;
-	__u8			inner_protocol_type:1;
+	__u8			nf_trace:1;
 #ifdef CONFIG_NET_SWITCHDEV
 	__u8			offload_fwd_mark:1;
 	__u8			offload_l3_fwd_mark:1;
-- 
2.39.2

[PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Jakub Kicinski]

Accesses to nf_trace and ipvs_property are already wrapped
by ifdefs where necessary. Don't allocate the bits for those
fields at all if possible.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: pablo@netfilter.org
CC: fw@strlen.de
---
 include/linux/skbuff.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 543f7ae9f09f..7b43d5a03613 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -966,8 +966,12 @@ struct sk_buff {
 	__u8			ndisc_nodetype:2;
 #endif
 
+#if IS_ENABLED(CONFIG_IP_VS)
 	__u8			ipvs_property:1;
+#endif
+#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
 	__u8			nf_trace:1;
+#endif
 #ifdef CONFIG_NET_SWITCHDEV
 	__u8			offload_fwd_mark:1;
 	__u8			offload_l3_fwd_mark:1;
-- 
2.39.2

Re: [PATCH net-next 1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set

[Florian Fainelli]

On 4/14/23 09:01, Jakub Kicinski wrote:
> Datacenter kernel builds will very likely not include WIRELESS,
> so let them shave 2 bits off the skb by hiding the wifi fields.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP not set

[Florian Fainelli]

On 4/14/23 09:01, Jakub Kicinski wrote:
> SCTP is not universally deployed, allow hiding its bit
> from the skb.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Florian Fainelli]

On 4/14/23 09:01, Jakub Kicinski wrote:
> Accesses to nf_trace and ipvs_property are already wrapped
> by ifdefs where necessary. Don't allocate the bits for those
> fields at all if possible.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: pablo@netfilter.org
> CC: fw@strlen.de
> ---
>   include/linux/skbuff.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 543f7ae9f09f..7b43d5a03613 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -966,8 +966,12 @@ struct sk_buff {
>   	__u8			ndisc_nodetype:2;
>   #endif
>   
> +#if IS_ENABLED(CONFIG_IP_VS)
>   	__u8			ipvs_property:1;
> +#endif
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)

Should that be IS_ENABLED(CONFIG_NT_TABLES) given it can be tristate?

>   	__u8			nf_trace:1;
> +#endif
>   #ifdef CONFIG_NET_SWITCHDEV
>   	__u8			offload_fwd_mark:1;
>   	__u8			offload_l3_fwd_mark:1;

-- 
Florian

Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield

[Florian Fainelli]

On 4/14/23 09:01, Jakub Kicinski wrote:
> nf_trace is a debug feature, AFAIU, and yet it sits oddly
> high in the sk_buff bitfield. Move it down, pushing up
> dst_pending_confirm and inner_protocol_type.
> 
> Next change will make nf_trace optional (under Kconfig)
> and all optional fields should be placed after 2b fields
> to avoid 2b fields straddling bytes.
> 
> dst_pending_confirm is L3, so it makes sense next to ignore_df.
> inner_protocol_type goes up just to keep the balance.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole

[Florian Fainelli]

On 4/14/23 09:01, Jakub Kicinski wrote:
> alloc_cpu is currently between 4 byte fields, so it's almost
> guaranteed to create a 2B hole. It has a knock on effect of
> creating a 4B hole after @end (and @end and @tail being in
> different cachelines).
> 
> None of this matters hugely, but for kernel configs which
> don't enable all the features there may well be a 2B hole
> after the bitfield. Move alloc_cpu there.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> nf_trace is a debug feature, AFAIU, and yet it sits oddly
> high in the sk_buff bitfield. Move it down, pushing up
> dst_pending_confirm and inner_protocol_type.

Yes, its a debug feature and can be moved.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> Accesses to nf_trace and ipvs_property are already wrapped
> by ifdefs where necessary. Don't allocate the bits for those
> fields at all if possible.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: pablo@netfilter.org
> CC: fw@strlen.de
> ---
>  include/linux/skbuff.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 543f7ae9f09f..7b43d5a03613 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -966,8 +966,12 @@ struct sk_buff {
>  	__u8			ndisc_nodetype:2;
>  #endif
>  
> +#if IS_ENABLED(CONFIG_IP_VS)
>  	__u8			ipvs_property:1;
> +#endif
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
>  	__u8			nf_trace:1;

As already pointed out nftables can be a module, other than that

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Jakub Kicinski]

On Fri, 14 Apr 2023 23:09:50 +0200 Florian Westphal wrote:
> > +#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
> >  	__u8			nf_trace:1;  
> 
> As already pointed out nftables can be a module, other than that

I copied it from:

static inline void nf_reset_trace(struct sk_buff *skb)
{
#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
	skb->nf_trace = 0;
#endif
}

I can't quite figure out why this would be intentional.
Do the existing conditions need to be fixed? 🤔️

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 14 Apr 2023 23:09:50 +0200 Florian Westphal wrote:
> > > +#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
> > >  	__u8			nf_trace:1;  
> > 
> > As already pointed out nftables can be a module, other than that
> 
> I copied it from:
> 
> static inline void nf_reset_trace(struct sk_buff *skb)
> {
> #if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
> 	skb->nf_trace = 0;
> #endif
> }
>
> I can't quite figure out why this would be intentional.
> Do the existing conditions need to be fixed?

Yes, this is not correct; needs to be "|| IS_ENABLED(CONFIG_NF_TABLES)".

Fixes: 478b360a47b7 ("netfilter: nf_tables: fix nf_trace always-on with XT_TRACE=n")

Let me know if you'd like to add the fix to v2 of your patchset
yourself, otherwise I'll send a fixup patch to netfilter-devel@ on
monday.

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[kernel test robot]

Hi Jakub,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Kicinski/net-skbuff-hide-wifi_acked-when-CONFIG_WIRELESS-not-set/20230415-000750
patch link:    https://lore.kernel.org/r/20230414160105.172125-6-kuba%40kernel.org
patch subject: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
config: ia64-randconfig-r031-20230409 (https://download.01.org/0day-ci/archive/20230415/202304150856.ZcdKTZna-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e2a92e33e41fe773b7c4a32a75db87340855387a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jakub-Kicinski/net-skbuff-hide-wifi_acked-when-CONFIG_WIRELESS-not-set/20230415-000750
        git checkout e2a92e33e41fe773b7c4a32a75db87340855387a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash net/netfilter/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304150856.ZcdKTZna-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/netfilter/nf_tables_core.c: In function 'nft_trace_packet':
>> net/netfilter/nf_tables_core.c:64:42: error: 'struct sk_buff' has no member named 'nf_trace'
      64 |                 info->nf_trace = pkt->skb->nf_trace;
         |                                          ^~
   net/netfilter/nf_tables_core.c: In function 'nft_trace_copy_nftrace':
   net/netfilter/nf_tables_core.c:75:50: error: 'struct sk_buff' has no member named 'nf_trace'
      75 |                         info->nf_trace = pkt->skb->nf_trace;
         |                                                  ^~
   net/netfilter/nf_tables_core.c: In function '__nft_trace_verdict':
   net/netfilter/nf_tables_core.c:132:56: error: 'struct sk_buff' has no member named 'nf_trace'
     132 |                         info->nf_trace = info->pkt->skb->nf_trace;
         |                                                        ^~
--
   net/netfilter/nf_tables_trace.c: In function 'nft_trace_init':
>> net/netfilter/nf_tables_trace.c:284:34: error: 'struct sk_buff' has no member named 'nf_trace'
     284 |         info->nf_trace = pkt->skb->nf_trace;
         |                                  ^~
--
   net/netfilter/nft_meta.c: In function 'nft_meta_set_eval':
>> net/netfilter/nft_meta.c:446:20: error: 'struct sk_buff' has no member named 'nf_trace'
     446 |                 skb->nf_trace = !!value8;
         |                    ^~


vim +64 net/netfilter/nf_tables_core.c

01ef16c2dd2e9a Patrick McHardy   2015-03-03  56  
399a14ec7993d6 Florian Westphal  2022-08-04  57  static inline void nft_trace_packet(const struct nft_pktinfo *pkt,
399a14ec7993d6 Florian Westphal  2022-08-04  58  				    struct nft_traceinfo *info,
01ef16c2dd2e9a Patrick McHardy   2015-03-03  59  				    const struct nft_chain *chain,
2c865a8a28a10e Pablo Neira Ayuso 2022-01-09  60  				    const struct nft_rule_dp *rule,
33d5a7b14bfd02 Florian Westphal  2015-11-28  61  				    enum nft_trace_types type)
01ef16c2dd2e9a Patrick McHardy   2015-03-03  62  {
e639f7ab079b52 Florian Westphal  2015-11-28  63  	if (static_branch_unlikely(&nft_trace_enabled)) {
e34b9ed96ce3b0 Florian Westphal  2022-06-22 @64  		info->nf_trace = pkt->skb->nf_trace;
33d5a7b14bfd02 Florian Westphal  2015-11-28  65  		info->rule = rule;
e65eebec9c67df Florian Westphal  2018-05-11  66  		__nft_trace_packet(info, chain, type);
33d5a7b14bfd02 Florian Westphal  2015-11-28  67  	}
01ef16c2dd2e9a Patrick McHardy   2015-03-03  68  }
01ef16c2dd2e9a Patrick McHardy   2015-03-03  69  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[kernel test robot]

Hi Jakub,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Kicinski/net-skbuff-hide-wifi_acked-when-CONFIG_WIRELESS-not-set/20230415-000750
patch link:    https://lore.kernel.org/r/20230414160105.172125-6-kuba%40kernel.org
patch subject: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
config: i386-randconfig-a005-20230410 (https://download.01.org/0day-ci/archive/20230415/202304150840.hkUoyFNG-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e2a92e33e41fe773b7c4a32a75db87340855387a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jakub-Kicinski/net-skbuff-hide-wifi_acked-when-CONFIG_WIRELESS-not-set/20230415-000750
        git checkout e2a92e33e41fe773b7c4a32a75db87340855387a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/netfilter/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304150840.hkUoyFNG-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/netfilter/nf_tables_core.c:64:30: error: no member named 'nf_trace' in 'struct sk_buff'
                   info->nf_trace = pkt->skb->nf_trace;
                                    ~~~~~~~~  ^
   net/netfilter/nf_tables_core.c:75:31: error: no member named 'nf_trace' in 'struct sk_buff'
                           info->nf_trace = pkt->skb->nf_trace;
                                            ~~~~~~~~  ^
   net/netfilter/nf_tables_core.c:132:37: error: no member named 'nf_trace' in 'struct sk_buff'
                           info->nf_trace = info->pkt->skb->nf_trace;
                                            ~~~~~~~~~~~~~~  ^
   3 errors generated.
--
>> net/netfilter/nf_tables_trace.c:284:29: error: no member named 'nf_trace' in 'struct sk_buff'
           info->nf_trace = pkt->skb->nf_trace;
                            ~~~~~~~~  ^
   1 error generated.
--
>> net/netfilter/nft_meta.c:446:8: error: no member named 'nf_trace' in 'struct sk_buff'
                   skb->nf_trace = !!value8;
                   ~~~  ^
   1 error generated.


vim +64 net/netfilter/nf_tables_core.c

01ef16c2dd2e9a Patrick McHardy   2015-03-03  56  
399a14ec7993d6 Florian Westphal  2022-08-04  57  static inline void nft_trace_packet(const struct nft_pktinfo *pkt,
399a14ec7993d6 Florian Westphal  2022-08-04  58  				    struct nft_traceinfo *info,
01ef16c2dd2e9a Patrick McHardy   2015-03-03  59  				    const struct nft_chain *chain,
2c865a8a28a10e Pablo Neira Ayuso 2022-01-09  60  				    const struct nft_rule_dp *rule,
33d5a7b14bfd02 Florian Westphal  2015-11-28  61  				    enum nft_trace_types type)
01ef16c2dd2e9a Patrick McHardy   2015-03-03  62  {
e639f7ab079b52 Florian Westphal  2015-11-28  63  	if (static_branch_unlikely(&nft_trace_enabled)) {
e34b9ed96ce3b0 Florian Westphal  2022-06-22 @64  		info->nf_trace = pkt->skb->nf_trace;
33d5a7b14bfd02 Florian Westphal  2015-11-28  65  		info->rule = rule;
e65eebec9c67df Florian Westphal  2018-05-11  66  		__nft_trace_packet(info, chain, type);
33d5a7b14bfd02 Florian Westphal  2015-11-28  67  	}
01ef16c2dd2e9a Patrick McHardy   2015-03-03  68  }
01ef16c2dd2e9a Patrick McHardy   2015-03-03  69  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Jakub Kicinski]

On Sat, 15 Apr 2023 01:11:39 +0200 Florian Westphal wrote:
> > I copied it from:
> > 
> > static inline void nf_reset_trace(struct sk_buff *skb)
> > {
> > #if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
> > 	skb->nf_trace = 0;
> > #endif
> > }
> >
> > I can't quite figure out why this would be intentional.
> > Do the existing conditions need to be fixed?  
> 
> Yes, this is not correct; needs to be "|| IS_ENABLED(CONFIG_NF_TABLES)".
> 
> Fixes: 478b360a47b7 ("netfilter: nf_tables: fix nf_trace always-on with XT_TRACE=n")
> 
> Let me know if you'd like to add the fix to v2 of your patchset
> yourself, otherwise I'll send a fixup patch to netfilter-devel@ on
> monday.

You can take the fix thru netfilter, I reckon. The new condition 
is wider so I should be able to proceed with v2 without waiting.

Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield

[Pablo Neira Ayuso]

On Fri, Apr 14, 2023 at 09:01:04AM -0700, Jakub Kicinski wrote:
> nf_trace is a debug feature, AFAIU, and yet it sits oddly
> high in the sk_buff bitfield. Move it down, pushing up
> dst_pending_confirm and inner_protocol_type.
> 
> Next change will make nf_trace optional (under Kconfig)
> and all optional fields should be placed after 2b fields
> to avoid 2b fields straddling bytes.
> 
> dst_pending_confirm is L3, so it makes sense next to ignore_df.
> inner_protocol_type goes up just to keep the balance.

Well, yes, this is indeed a debug feature.

But if only one single container enables debugging, this cache line
will be visited very often. The debugging infrastructure is guarded
under a static_key, which is global.

Re: [PATCH net-next 0/5] net: skbuff: hide some bitfield members

[Eric Dumazet]

On Fri, Apr 14, 2023 at 6:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> There is a number of protocol or subsystem specific fields
> in struct sk_buff which are only accessed by one subsystem.
> We can wrap them in ifdefs with minimal code impact.
>
> This gives us a better chance to save a 2B and a 4B holes
> resulting with the following savings (assuming a lucky
> kernel config):
>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield

[Jakub Kicinski]

On Sat, 15 Apr 2023 10:31:19 +0200 Pablo Neira Ayuso wrote:
> On Fri, Apr 14, 2023 at 09:01:04AM -0700, Jakub Kicinski wrote:
> > nf_trace is a debug feature, AFAIU, and yet it sits oddly
> > high in the sk_buff bitfield. Move it down, pushing up
> > dst_pending_confirm and inner_protocol_type.
> > 
> > Next change will make nf_trace optional (under Kconfig)
> > and all optional fields should be placed after 2b fields
> > to avoid 2b fields straddling bytes.
> > 
> > dst_pending_confirm is L3, so it makes sense next to ignore_df.
> > inner_protocol_type goes up just to keep the balance.  
> 
> Well, yes, this is indeed a debug feature.
> 
> But if only one single container enables debugging, this cache line
> will be visited very often. The debugging infrastructure is guarded
> under a static_key, which is global.

I wasn't thinking about cacheline placement, really, although you're
right, under some custom configs it may indeed push the bit from the
second to the third cache line.

The problem is that I can't make the bit optional if it sits this far
up in the bitfield because (as mentioned) 2 bit fields start to
straddle bytes. And that leads to holes.

WiFi is a bit lucky because it has 2 bits and largest fields are also 
2b so it can't cause straddling when Kconfig'ed out.

Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property

[Simon Horman]

On Fri, Apr 14, 2023 at 09:01:05AM -0700, Jakub Kicinski wrote:
> Accesses to nf_trace and ipvs_property are already wrapped
> by ifdefs where necessary. Don't allocate the bits for those
> fields at all if possible.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

FWIIW, I'm fine with this, modulo the module handling
discussed elsewhere in this thread.

Acked-by: Simon Horman <horms@kernel.org>

> ---
> CC: pablo@netfilter.org
> CC: fw@strlen.de
> ---
>  include/linux/skbuff.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 543f7ae9f09f..7b43d5a03613 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -966,8 +966,12 @@ struct sk_buff {
>  	__u8			ndisc_nodetype:2;
>  #endif
>  
> +#if IS_ENABLED(CONFIG_IP_VS)
>  	__u8			ipvs_property:1;
> +#endif
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
>  	__u8			nf_trace:1;
> +#endif
>  #ifdef CONFIG_NET_SWITCHDEV
>  	__u8			offload_fwd_mark:1;
>  	__u8			offload_l3_fwd_mark:1;
> -- 
> 2.39.2
>