| Message ID | 20230414160105.172125-5-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: skbuff: hide some bitfield members | expand |
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>
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>
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.
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.
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;
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(-)