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

[PATCH net-next v2 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.

v2:
 - use IS_ENABLED() for nf_trace
v1: https://lore.kernel.org/all/20230414160105.172125-1-kuba@kernel.org/

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 v2 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
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 a823ec3aa326..513f03b23a73 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 ef81452759be..768f9d04911f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5189,6 +5189,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;
@@ -5214,6 +5215,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

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

[Johannes Berg]

On Mon, 2023-04-17 at 08:53 -0700, 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.

:-)

You could make this be CONFIG_MAC80211 if you wanted to, but I don't
mind CONFIG_WIRELESS and honestly they're probably pretty much
equivalent because there aren't many non-mac80211 drivers.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

johannes

[PATCH net-next v2 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
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 513f03b23a73..98d6b48f4dcf 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 */
@@ -5060,7 +5062,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 8aea68275172..3fc4dba71f9d 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 v2 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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
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 98d6b48f4dcf..2595b2cfba0d 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 v2 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.

Acked-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
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 2595b2cfba0d..3ae9e8868afa 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 v2 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.

Acked-by: Florian Westphal <fw@strlen.de>
Acked-by: Simon Horman <horms@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - use IS_ENABLED()
v1: https://lore.kernel.org/all/20230414160105.172125-1-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 3ae9e8868afa..5f120bbab9da 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) || IS_ENABLED(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 v2 0/5] net: skbuff: hide some bitfield members

[Jakub Kicinski]

On Mon, 17 Apr 2023 08:53:45 -0700 Jakub Kicinski 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):
> 
> -	/* 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.

Missed Simon's tag, sorry about that: 
https://lore.kernel.org/all/ZD03APYJqdhflYNJ@kernel.org/

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

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 17 Apr 2023 08:53:45 -0700 you 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):
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set
    https://git.kernel.org/netdev/net-next/c/eb6fba7555a8
  - [net-next,v2,2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP not set
    https://git.kernel.org/netdev/net-next/c/c24831a13ba2
  - [net-next,v2,3/5] net: skbuff: move alloc_cpu into a potential hole
    https://git.kernel.org/netdev/net-next/c/4398f3f6d138
  - [net-next,v2,4/5] net: skbuff: push nf_trace down the bitfield
    https://git.kernel.org/netdev/net-next/c/4c60d04c2888
  - [net-next,v2,5/5] net: skbuff: hide nf_trace and ipvs_property
    https://git.kernel.org/netdev/net-next/c/48d80c394d3d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html