* [PATCH net-next 1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
@ 2023-04-14 16:01 ` Jakub Kicinski
2023-04-14 17:46 ` Florian Fainelli
2023-04-14 16:01 ` [PATCH net-next 2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP " Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 16:01 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, Jakub Kicinski, johannes
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP not set
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
2023-04-14 16:01 ` [PATCH net-next 1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set Jakub Kicinski
@ 2023-04-14 16:01 ` Jakub Kicinski
2023-04-14 17:47 ` Florian Fainelli
2023-04-14 16:01 ` [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 16:01 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
2023-04-14 16:01 ` [PATCH net-next 1/5] net: skbuff: hide wifi_acked when CONFIG_WIRELESS not set Jakub Kicinski
2023-04-14 16:01 ` [PATCH net-next 2/5] net: skbuff: hide csum_not_inet when CONFIG_IP_SCTP " Jakub Kicinski
@ 2023-04-14 16:01 ` Jakub Kicinski
2023-04-14 17:51 ` Florian Fainelli
2023-04-14 16:01 ` [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 16:01 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole
2023-04-14 16:01 ` [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole Jakub Kicinski
@ 2023-04-14 17:51 ` Florian Fainelli
0 siblings, 0 replies; 22+ messages in thread
From: Florian Fainelli @ 2023-04-14 17:51 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
` (2 preceding siblings ...)
2023-04-14 16:01 ` [PATCH net-next 3/5] net: skbuff: move alloc_cpu into a potential hole Jakub Kicinski
@ 2023-04-14 16:01 ` Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
` (2 more replies)
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
2023-04-15 11:53 ` [PATCH net-next 0/5] net: skbuff: hide some bitfield members Eric Dumazet
5 siblings, 3 replies; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 16:01 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, Jakub Kicinski, pablo, fw
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield
2023-04-14 16:01 ` [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield Jakub Kicinski
@ 2023-04-14 17:50 ` Florian Fainelli
2023-04-14 21:06 ` Florian Westphal
2023-04-15 8:31 ` Pablo Neira Ayuso
2 siblings, 0 replies; 22+ messages in thread
From: Florian Fainelli @ 2023-04-14 17:50 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni, pablo, fw
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield
2023-04-14 16:01 ` [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
@ 2023-04-14 21:06 ` Florian Westphal
2023-04-15 8:31 ` Pablo Neira Ayuso
2 siblings, 0 replies; 22+ messages in thread
From: Florian Westphal @ 2023-04-14 21:06 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, pablo, fw
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield
2023-04-14 16:01 ` [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
2023-04-14 21:06 ` Florian Westphal
@ 2023-04-15 8:31 ` Pablo Neira Ayuso
2023-04-17 4:12 ` Jakub Kicinski
2 siblings, 1 reply; 22+ messages in thread
From: Pablo Neira Ayuso @ 2023-04-15 8:31 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, fw
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield
2023-04-15 8:31 ` Pablo Neira Ayuso
@ 2023-04-17 4:12 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-17 4:12 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: davem, netdev, edumazet, pabeni, fw
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
` (3 preceding siblings ...)
2023-04-14 16:01 ` [PATCH net-next 4/5] net: skbuff: push nf_trace down the bitfield Jakub Kicinski
@ 2023-04-14 16:01 ` Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
` (4 more replies)
2023-04-15 11:53 ` [PATCH net-next 0/5] net: skbuff: hide some bitfield members Eric Dumazet
5 siblings, 5 replies; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 16:01 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, Jakub Kicinski, pablo, fw
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
@ 2023-04-14 17:50 ` Florian Fainelli
2023-04-14 21:09 ` Florian Westphal
` (3 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: Florian Fainelli @ 2023-04-14 17:50 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni, pablo, fw
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
@ 2023-04-14 21:09 ` Florian Westphal
2023-04-14 22:07 ` Jakub Kicinski
2023-04-15 0:32 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Florian Westphal @ 2023-04-14 21:09 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, pablo, fw
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 21:09 ` Florian Westphal
@ 2023-04-14 22:07 ` Jakub Kicinski
2023-04-14 23:11 ` Florian Westphal
0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-14 22:07 UTC (permalink / raw)
To: Florian Westphal, Florian Fainelli; +Cc: davem, netdev, edumazet, pabeni, pablo
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? 🤔️
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 22:07 ` Jakub Kicinski
@ 2023-04-14 23:11 ` Florian Westphal
2023-04-15 0:44 ` Jakub Kicinski
0 siblings, 1 reply; 22+ messages in thread
From: Florian Westphal @ 2023-04-14 23:11 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Florian Fainelli, davem, netdev, edumazet,
pabeni, pablo
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 23:11 ` Florian Westphal
@ 2023-04-15 0:44 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2023-04-15 0:44 UTC (permalink / raw)
To: Florian Westphal; +Cc: Florian Fainelli, davem, netdev, edumazet, pabeni, pablo
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
2023-04-14 17:50 ` Florian Fainelli
2023-04-14 21:09 ` Florian Westphal
@ 2023-04-15 0:32 ` kernel test robot
2023-04-15 0:43 ` kernel test robot
2023-04-17 12:09 ` Simon Horman
4 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-04-15 0:32 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: oe-kbuild-all, netdev, edumazet, pabeni, Jakub Kicinski, pablo, fw
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
` (2 preceding siblings ...)
2023-04-15 0:32 ` kernel test robot
@ 2023-04-15 0:43 ` kernel test robot
2023-04-17 12:09 ` Simon Horman
4 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-04-15 0:43 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: llvm, oe-kbuild-all, netdev, edumazet, pabeni, Jakub Kicinski, pablo, fw
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
` (3 preceding siblings ...)
2023-04-15 0:43 ` kernel test robot
@ 2023-04-17 12:09 ` Simon Horman
4 siblings, 0 replies; 22+ messages in thread
From: Simon Horman @ 2023-04-17 12:09 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, edumazet, pabeni, pablo, fw
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 0/5] net: skbuff: hide some bitfield members
2023-04-14 16:01 [PATCH net-next 0/5] net: skbuff: hide some bitfield members Jakub Kicinski
` (4 preceding siblings ...)
2023-04-14 16:01 ` [PATCH net-next 5/5] net: skbuff: hide nf_trace and ipvs_property Jakub Kicinski
@ 2023-04-15 11:53 ` Eric Dumazet
5 siblings, 0 replies; 22+ messages in thread
From: Eric Dumazet @ 2023-04-15 11:53 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, pabeni
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>
^ permalink raw reply [flat|nested] 22+ messages in thread