* [PATCH net-next] net: shrink napi_skb_cache_put() @ 2025-10-15 23:38 Eric Dumazet 2025-10-16 0:14 ` Kuniyuki Iwashima 2025-10-16 10:20 ` Alexander Lobakin 0 siblings, 2 replies; 10+ messages in thread From: Eric Dumazet @ 2025-10-15 23:38 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet, Eric Dumazet, Alexander Lobakin Following loop in napi_skb_cache_put() is unrolled by the compiler even if CONFIG_KASAN is not enabled: for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) kasan_mempool_unpoison_object(nc->skb_cache[i], kmem_cache_size(net_hotdata.skbuff_cache)); We have 32 times this sequence, for a total of 384 bytes. 48 8b 3d 00 00 00 00 net_hotdata.skbuff_cache,%rdi e8 00 00 00 00 call kmem_cache_size This is because kmem_cache_size() is an extern function, and kasan_unpoison_object_data() is an inline function. Cache kmem_cache_size() result in a temporary variable, and make the loop conditional to CONFIG_KASAN. After this patch, napi_skb_cache_put() is inlined in its callers. Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Alexander Lobakin <aleksander.lobakin@intel.com> --- net/core/skbuff.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..5a8b48b201843f94b5fdaab3241801f642fbd1f0 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1426,10 +1426,13 @@ static void napi_skb_cache_put(struct sk_buff *skb) nc->skb_cache[nc->skb_count++] = skb; if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { - for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) - kasan_mempool_unpoison_object(nc->skb_cache[i], - kmem_cache_size(net_hotdata.skbuff_cache)); + if (IS_ENABLED(CONFIG_KASAN)) { + u32 size = kmem_cache_size(net_hotdata.skbuff_cache); + for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) + kasan_mempool_unpoison_object(nc->skb_cache[i], + size); + } kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, nc->skb_cache + NAPI_SKB_CACHE_HALF); nc->skb_count = NAPI_SKB_CACHE_HALF; -- 2.51.0.869.ge66316f041-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-15 23:38 [PATCH net-next] net: shrink napi_skb_cache_put() Eric Dumazet @ 2025-10-16 0:14 ` Kuniyuki Iwashima 2025-10-16 10:20 ` Alexander Lobakin 1 sibling, 0 replies; 10+ messages in thread From: Kuniyuki Iwashima @ 2025-10-16 0:14 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, eric.dumazet, Alexander Lobakin On Wed, Oct 15, 2025 at 4:38 PM Eric Dumazet <edumazet@google.com> wrote: > > Following loop in napi_skb_cache_put() is unrolled by the compiler > even if CONFIG_KASAN is not enabled: > > for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > kasan_mempool_unpoison_object(nc->skb_cache[i], > kmem_cache_size(net_hotdata.skbuff_cache)); > > We have 32 times this sequence, for a total of 384 bytes. > > 48 8b 3d 00 00 00 00 net_hotdata.skbuff_cache,%rdi > e8 00 00 00 00 call kmem_cache_size > > This is because kmem_cache_size() is an extern function, > and kasan_unpoison_object_data() is an inline function. > > Cache kmem_cache_size() result in a temporary variable, and > make the loop conditional to CONFIG_KASAN. > > After this patch, napi_skb_cache_put() is inlined in its callers. > > Signed-off-by: Eric Dumazet <edumazet@google.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com> Oh exactly, it's interesting that the compiler cannot optimise out unused retval even though kmem_cache_size() is a single load :o $ grep KASAN .config CONFIG_HAVE_ARCH_KASAN=y CONFIG_HAVE_ARCH_KASAN_VMALLOC=y CONFIG_CC_HAS_KASAN_GENERIC=y CONFIG_CC_HAS_KASAN_SW_TAGS=y # CONFIG_KASAN is not set $ objdump --disassemble=napi_skb_cache_put vmlinux ... ffffffff81e9fe10 <napi_skb_cache_put>: ... ffffffff81e9fe40: 48 8b 3d 89 bd d6 00 mov 0xd6bd89(%rip),%rdi # ffffffff82c0bbd0 <net_hotdata+0x150> ffffffff81e9fe47: e8 a4 79 60 ff call ffffffff814a77f0 <kmem_cache_size> ffffffff81e9fe4c: 83 eb 01 sub $0x1,%ebx ffffffff81e9fe4f: 75 ef jne ffffffff81e9fe40 <napi_skb_cache_put+0x30> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-15 23:38 [PATCH net-next] net: shrink napi_skb_cache_put() Eric Dumazet 2025-10-16 0:14 ` Kuniyuki Iwashima @ 2025-10-16 10:20 ` Alexander Lobakin 2025-10-16 10:31 ` Eric Dumazet 1 sibling, 1 reply; 10+ messages in thread From: Alexander Lobakin @ 2025-10-16 10:20 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet From: Eric Dumazet <edumazet@google.com> Date: Wed, 15 Oct 2025 23:38:01 +0000 > Following loop in napi_skb_cache_put() is unrolled by the compiler > even if CONFIG_KASAN is not enabled: > > for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > kasan_mempool_unpoison_object(nc->skb_cache[i], > kmem_cache_size(net_hotdata.skbuff_cache)); > > We have 32 times this sequence, for a total of 384 bytes. > > 48 8b 3d 00 00 00 00 net_hotdata.skbuff_cache,%rdi > e8 00 00 00 00 call kmem_cache_size > > This is because kmem_cache_size() is an extern function, > and kasan_unpoison_object_data() is an inline function. > > Cache kmem_cache_size() result in a temporary variable, and > make the loop conditional to CONFIG_KASAN. > > After this patch, napi_skb_cache_put() is inlined in its callers. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > net/core/skbuff.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..5a8b48b201843f94b5fdaab3241801f642fbd1f0 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1426,10 +1426,13 @@ static void napi_skb_cache_put(struct sk_buff *skb) > nc->skb_cache[nc->skb_count++] = skb; > > if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { > - for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > - kasan_mempool_unpoison_object(nc->skb_cache[i], > - kmem_cache_size(net_hotdata.skbuff_cache)); > + if (IS_ENABLED(CONFIG_KASAN)) { > + u32 size = kmem_cache_size(net_hotdata.skbuff_cache); > > + for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > + kasan_mempool_unpoison_object(nc->skb_cache[i], > + size); > + } Very interesting; back when implementing napi_skb_cache*() family and someone (most likely Jakub) asked me to add KASAN-related checks here, I was comparing the object code and stopped on the current variant, as without KASAN, the entire loop got optimized away (but only when kmem_cache_size() is *not* a temporary variable). Or does this patch addresses KASAN-enabled kernels? Either way, if this patch really optimizes things: Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com> > kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, > nc->skb_cache + NAPI_SKB_CACHE_HALF); > nc->skb_count = NAPI_SKB_CACHE_HALF; Thanks, Olek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 10:20 ` Alexander Lobakin @ 2025-10-16 10:31 ` Eric Dumazet 2025-10-16 11:07 ` Alexander Lobakin 0 siblings, 1 reply; 10+ messages in thread From: Eric Dumazet @ 2025-10-16 10:31 UTC (permalink / raw) To: Alexander Lobakin Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet On Thu, Oct 16, 2025 at 3:20 AM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 15 Oct 2025 23:38:01 +0000 > > > Following loop in napi_skb_cache_put() is unrolled by the compiler > > even if CONFIG_KASAN is not enabled: > > > > for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > > kasan_mempool_unpoison_object(nc->skb_cache[i], > > kmem_cache_size(net_hotdata.skbuff_cache)); > > > > We have 32 times this sequence, for a total of 384 bytes. > > > > 48 8b 3d 00 00 00 00 net_hotdata.skbuff_cache,%rdi > > e8 00 00 00 00 call kmem_cache_size > > > > This is because kmem_cache_size() is an extern function, > > and kasan_unpoison_object_data() is an inline function. > > > > Cache kmem_cache_size() result in a temporary variable, and > > make the loop conditional to CONFIG_KASAN. > > > > After this patch, napi_skb_cache_put() is inlined in its callers. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Alexander Lobakin <aleksander.lobakin@intel.com> > > --- > > net/core/skbuff.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..5a8b48b201843f94b5fdaab3241801f642fbd1f0 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -1426,10 +1426,13 @@ static void napi_skb_cache_put(struct sk_buff *skb) > > nc->skb_cache[nc->skb_count++] = skb; > > > > if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { > > - for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > > - kasan_mempool_unpoison_object(nc->skb_cache[i], > > - kmem_cache_size(net_hotdata.skbuff_cache)); > > + if (IS_ENABLED(CONFIG_KASAN)) { > > + u32 size = kmem_cache_size(net_hotdata.skbuff_cache); > > > > + for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > > + kasan_mempool_unpoison_object(nc->skb_cache[i], > > + size); > > + } > > Very interesting; back when implementing napi_skb_cache*() family and > someone (most likely Jakub) asked me to add KASAN-related checks here, > I was comparing the object code and stopped on the current variant, as > without KASAN, the entire loop got optimized away (but only when > kmem_cache_size() is *not* a temporary variable). > > Or does this patch addresses KASAN-enabled kernels? Either way, if this > patch really optimizes things: No, this is when CONFIG_KASAN is _not_ enabled. (I have not checked when it is enabled, I do not care about the cost of KASAN as long as it is not too expensive) Compiler does not know anything about kmem_cache_size() It could contain some memory cloberring, memory freeing, some kind of destructive action. So it has to call it 32 times. And reload net_hotdata.skbuff_cache 32 times, because the value could have been changed by kmem_cache_size() (if kmem_cache_size() wanted to) Not sure if kmem_cache_size() could be inlined. Its use has been discouraged so I guess nobody cared. > > Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com> > > > kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, > > nc->skb_cache + NAPI_SKB_CACHE_HALF); > > nc->skb_count = NAPI_SKB_CACHE_HALF; > > Thanks, > Olek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 10:31 ` Eric Dumazet @ 2025-10-16 11:07 ` Alexander Lobakin 2025-10-16 12:56 ` Eric Dumazet 0 siblings, 1 reply; 10+ messages in thread From: Alexander Lobakin @ 2025-10-16 11:07 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet From: Eric Dumazet <edumazet@google.com> Date: Thu, 16 Oct 2025 03:31:37 -0700 > On Thu, Oct 16, 2025 at 3:20 AM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: >> >> From: Eric Dumazet <edumazet@google.com> >> Date: Wed, 15 Oct 2025 23:38:01 +0000 >> >>> Following loop in napi_skb_cache_put() is unrolled by the compiler >>> even if CONFIG_KASAN is not enabled: >>> >>> for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) >>> kasan_mempool_unpoison_object(nc->skb_cache[i], >>> kmem_cache_size(net_hotdata.skbuff_cache)); >>> >>> We have 32 times this sequence, for a total of 384 bytes. >>> >>> 48 8b 3d 00 00 00 00 net_hotdata.skbuff_cache,%rdi >>> e8 00 00 00 00 call kmem_cache_size >>> >>> This is because kmem_cache_size() is an extern function, >>> and kasan_unpoison_object_data() is an inline function. >>> >>> Cache kmem_cache_size() result in a temporary variable, and >>> make the loop conditional to CONFIG_KASAN. >>> >>> After this patch, napi_skb_cache_put() is inlined in its callers. >>> >>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>> Cc: Alexander Lobakin <aleksander.lobakin@intel.com> >>> --- >>> net/core/skbuff.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..5a8b48b201843f94b5fdaab3241801f642fbd1f0 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -1426,10 +1426,13 @@ static void napi_skb_cache_put(struct sk_buff *skb) >>> nc->skb_cache[nc->skb_count++] = skb; >>> >>> if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { >>> - for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) >>> - kasan_mempool_unpoison_object(nc->skb_cache[i], >>> - kmem_cache_size(net_hotdata.skbuff_cache)); >>> + if (IS_ENABLED(CONFIG_KASAN)) { >>> + u32 size = kmem_cache_size(net_hotdata.skbuff_cache); >>> >>> + for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) >>> + kasan_mempool_unpoison_object(nc->skb_cache[i], >>> + size); >>> + } >> >> Very interesting; back when implementing napi_skb_cache*() family and >> someone (most likely Jakub) asked me to add KASAN-related checks here, >> I was comparing the object code and stopped on the current variant, as >> without KASAN, the entire loop got optimized away (but only when >> kmem_cache_size() is *not* a temporary variable). >> >> Or does this patch addresses KASAN-enabled kernels? Either way, if this >> patch really optimizes things: > > No, this is when CONFIG_KASAN is _not_ enabled. > > (I have not checked when it is enabled, I do not care about the cost > of KASAN as long as it is not too expensive) > > Compiler does not know anything about kmem_cache_size() > It could contain some memory cloberring, memory freeing, some kind of > destructive action. > > So it has to call it 32 times. > > And reload net_hotdata.skbuff_cache 32 times, because the value could > have been changed > by kmem_cache_size() (if kmem_cache_size() wanted to) > > Not sure if kmem_cache_size() could be inlined. BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? > > Its use has been discouraged so I guess nobody cared. > >> >> Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com> >> >>> kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, >>> nc->skb_cache + NAPI_SKB_CACHE_HALF); >>> nc->skb_count = NAPI_SKB_CACHE_HALF; Thanks, Olek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 11:07 ` Alexander Lobakin @ 2025-10-16 12:56 ` Eric Dumazet 2025-10-16 13:29 ` Eric Dumazet 0 siblings, 1 reply; 10+ messages in thread From: Eric Dumazet @ 2025-10-16 12:56 UTC (permalink / raw) To: Alexander Lobakin Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet On Thu, Oct 16, 2025 at 4:08 AM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? Probably, like other calls to napi_skb_cache_put(() No loop there, so I guess there is no big deal. I was looking at napi_skb_cache_put() because there is a lack of NUMA awareness, and was curious to experiment with some strategies there. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 12:56 ` Eric Dumazet @ 2025-10-16 13:29 ` Eric Dumazet 2025-10-16 13:36 ` Eric Dumazet 0 siblings, 1 reply; 10+ messages in thread From: Eric Dumazet @ 2025-10-16 13:29 UTC (permalink / raw) To: Alexander Lobakin Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet On Thu, Oct 16, 2025 at 5:56 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Oct 16, 2025 at 4:08 AM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: > > > > From: Eric Dumazet <edumazet@google.com> > > > > BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? > > Probably, like other calls to napi_skb_cache_put(() > > No loop there, so I guess there is no big deal. > > I was looking at napi_skb_cache_put() because there is a lack of NUMA awareness, > and was curious to experiment with some strategies there. If we cache kmem_cache_size() in net_hotdata, the compiler is able to eliminate dead code for CONFIG_KASAN=n Maybe this looks better ? diff --git a/include/net/hotdata.h b/include/net/hotdata.h index 1aca9db99320f942b06b7d412d428a3045e87e60..f643e6a4647cc5e694a7044797f01a1107db46a9 100644 --- a/include/net/hotdata.h +++ b/include/net/hotdata.h @@ -33,9 +33,10 @@ struct net_hotdata { struct kmem_cache *skbuff_cache; struct kmem_cache *skbuff_fclone_cache; struct kmem_cache *skb_small_head_cache; + u32 skbuff_cache_size; #ifdef CONFIG_RPS - struct rps_sock_flow_table __rcu *rps_sock_flow_table; u32 rps_cpu_mask; + struct rps_sock_flow_table __rcu *rps_sock_flow_table; #endif struct skb_defer_node __percpu *skb_defer_nodes; int gro_normal_batch; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index c9e33f26852b63e930e33a406c19cc02f1821746..62b1acca55c7fd3e1fb7614cb0c625206db0ab3f 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -365,7 +365,7 @@ static struct sk_buff *napi_skb_cache_get(void) skb = nc->skb_cache[--nc->skb_count]; local_unlock_nested_bh(&napi_alloc_cache.bh_lock); - kasan_mempool_unpoison_object(skb, kmem_cache_size(net_hotdata.skbuff_cache)); + kasan_mempool_unpoison_object(skb, net_hotdata.skbuff_cache_size); return skb; } @@ -1504,7 +1504,7 @@ static void napi_skb_cache_put(struct sk_buff *skb) if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) kasan_mempool_unpoison_object(nc->skb_cache[i], - kmem_cache_size(net_hotdata.skbuff_cache)); + net_hotdata.skbuff_cache_size); kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, nc->skb_cache + NAPI_SKB_CACHE_HALF); @@ -5164,6 +5164,7 @@ void __init skb_init(void) offsetof(struct sk_buff, cb), sizeof_field(struct sk_buff, cb), NULL); + net_hotdata.skbuff_cache_size = kmem_cache_size(net_hotdata.skbuff_cache); net_hotdata.skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache", sizeof(struct sk_buff_fclones), 0, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 13:29 ` Eric Dumazet @ 2025-10-16 13:36 ` Eric Dumazet 2025-10-16 15:24 ` Alexander Lobakin 0 siblings, 1 reply; 10+ messages in thread From: Eric Dumazet @ 2025-10-16 13:36 UTC (permalink / raw) To: Alexander Lobakin Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet On Thu, Oct 16, 2025 at 6:29 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Oct 16, 2025 at 5:56 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, Oct 16, 2025 at 4:08 AM Alexander Lobakin > > <aleksander.lobakin@intel.com> wrote: > > > > > > From: Eric Dumazet <edumazet@google.com> > > > > > > BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? > > > > Probably, like other calls to napi_skb_cache_put(() > > > > No loop there, so I guess there is no big deal. > > > > I was looking at napi_skb_cache_put() because there is a lack of NUMA awareness, > > and was curious to experiment with some strategies there. > > If we cache kmem_cache_size() in net_hotdata, the compiler is able to > eliminate dead code > for CONFIG_KASAN=n > > Maybe this looks better ? No need to put this in net_hotdata, I was distracted by a 4byte hole there, we can keep this hole for something hot later. diff --git a/net/core/skbuff.c b/net/core/skbuff.c index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..f3b9356bebc06548a055355c5d1eb04c480f813f 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -274,6 +274,8 @@ void *__netdev_alloc_frag_align(unsigned int fragsz, unsigned int align_mask) } EXPORT_SYMBOL(__netdev_alloc_frag_align); +u32 skbuff_cache_size __read_mostly; + static struct sk_buff *napi_skb_cache_get(void) { struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); @@ -293,7 +295,7 @@ static struct sk_buff *napi_skb_cache_get(void) skb = nc->skb_cache[--nc->skb_count]; local_unlock_nested_bh(&napi_alloc_cache.bh_lock); - kasan_mempool_unpoison_object(skb, kmem_cache_size(net_hotdata.skbuff_cache)); + kasan_mempool_unpoison_object(skb, skbuff_cache_size); return skb; } @@ -1428,7 +1430,7 @@ static void napi_skb_cache_put(struct sk_buff *skb) if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) kasan_mempool_unpoison_object(nc->skb_cache[i], - kmem_cache_size(net_hotdata.skbuff_cache)); + skbuff_cache_size); kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF, nc->skb_cache + NAPI_SKB_CACHE_HALF); @@ -5116,6 +5118,8 @@ void __init skb_init(void) offsetof(struct sk_buff, cb), sizeof_field(struct sk_buff, cb), NULL); + skbuff_cache_size = kmem_cache_size(net_hotdata.skbuff_cache); + net_hotdata.skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache", sizeof(struct sk_buff_fclones), 0, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 13:36 ` Eric Dumazet @ 2025-10-16 15:24 ` Alexander Lobakin 2025-10-16 15:27 ` Eric Dumazet 0 siblings, 1 reply; 10+ messages in thread From: Alexander Lobakin @ 2025-10-16 15:24 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet From: Eric Dumazet <edumazet@google.com> Date: Thu, 16 Oct 2025 06:36:55 -0700 > On Thu, Oct 16, 2025 at 6:29 AM Eric Dumazet <edumazet@google.com> wrote: >> >> On Thu, Oct 16, 2025 at 5:56 AM Eric Dumazet <edumazet@google.com> wrote: >>> >>> On Thu, Oct 16, 2025 at 4:08 AM Alexander Lobakin >>> <aleksander.lobakin@intel.com> wrote: >>>> >>>> From: Eric Dumazet <edumazet@google.com> >>>> >>>> BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? >>> >>> Probably, like other calls to napi_skb_cache_put(() >>> >>> No loop there, so I guess there is no big deal. >>> >>> I was looking at napi_skb_cache_put() because there is a lack of NUMA awareness, >>> and was curious to experiment with some strategies there. >> >> If we cache kmem_cache_size() in net_hotdata, the compiler is able to >> eliminate dead code >> for CONFIG_KASAN=n >> >> Maybe this looks better ? > > No need to put this in net_hotdata, I was distracted by a 4byte hole > there, we can keep this hole for something hot later. Yeah this looks good! It's not "hot" anyway, so let it lay freestanding. > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..f3b9356bebc06548a055355c5d1eb04c480f813f > 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -274,6 +274,8 @@ void *__netdev_alloc_frag_align(unsigned int > fragsz, unsigned int align_mask) > } > EXPORT_SYMBOL(__netdev_alloc_frag_align); > > +u32 skbuff_cache_size __read_mostly; ...but probably `static`? > + > static struct sk_buff *napi_skb_cache_get(void) > { > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); > @@ -293,7 +295,7 @@ static struct sk_buff *napi_skb_cache_get(void) > > skb = nc->skb_cache[--nc->skb_count]; > local_unlock_nested_bh(&napi_alloc_cache.bh_lock); > - kasan_mempool_unpoison_object(skb, > kmem_cache_size(net_hotdata.skbuff_cache)); > + kasan_mempool_unpoison_object(skb, skbuff_cache_size); > > return skb; > } > @@ -1428,7 +1430,7 @@ static void napi_skb_cache_put(struct sk_buff *skb) > if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { > for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > kasan_mempool_unpoison_object(nc->skb_cache[i], > - > kmem_cache_size(net_hotdata.skbuff_cache)); > + skbuff_cache_size); > > kmem_cache_free_bulk(net_hotdata.skbuff_cache, > NAPI_SKB_CACHE_HALF, > nc->skb_cache + NAPI_SKB_CACHE_HALF); > @@ -5116,6 +5118,8 @@ void __init skb_init(void) > offsetof(struct sk_buff, cb), > sizeof_field(struct sk_buff, cb), > NULL); > + skbuff_cache_size = kmem_cache_size(net_hotdata.skbuff_cache); > + > net_hotdata.skbuff_fclone_cache = > kmem_cache_create("skbuff_fclone_cache", > sizeof(struct sk_buff_fclones), > 0, Thanks, Olek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: shrink napi_skb_cache_put() 2025-10-16 15:24 ` Alexander Lobakin @ 2025-10-16 15:27 ` Eric Dumazet 0 siblings, 0 replies; 10+ messages in thread From: Eric Dumazet @ 2025-10-16 15:27 UTC (permalink / raw) To: Alexander Lobakin Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, netdev, eric.dumazet On Thu, Oct 16, 2025 at 8:24 AM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Thu, 16 Oct 2025 06:36:55 -0700 > > > On Thu, Oct 16, 2025 at 6:29 AM Eric Dumazet <edumazet@google.com> wrote: > >> > >> On Thu, Oct 16, 2025 at 5:56 AM Eric Dumazet <edumazet@google.com> wrote: > >>> > >>> On Thu, Oct 16, 2025 at 4:08 AM Alexander Lobakin > >>> <aleksander.lobakin@intel.com> wrote: > >>>> > >>>> From: Eric Dumazet <edumazet@google.com> > >>>> > >>>> BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way? > >>> > >>> Probably, like other calls to napi_skb_cache_put(() > >>> > >>> No loop there, so I guess there is no big deal. > >>> > >>> I was looking at napi_skb_cache_put() because there is a lack of NUMA awareness, > >>> and was curious to experiment with some strategies there. > >> > >> If we cache kmem_cache_size() in net_hotdata, the compiler is able to > >> eliminate dead code > >> for CONFIG_KASAN=n > >> > >> Maybe this looks better ? > > > > No need to put this in net_hotdata, I was distracted by a 4byte hole > > there, we can keep this hole for something hot later. > > Yeah this looks good! It's not "hot" anyway, so let it lay freestanding. > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..f3b9356bebc06548a055355c5d1eb04c480f813f > > 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -274,6 +274,8 @@ void *__netdev_alloc_frag_align(unsigned int > > fragsz, unsigned int align_mask) > > } > > EXPORT_SYMBOL(__netdev_alloc_frag_align); > > > > +u32 skbuff_cache_size __read_mostly; > > ...but probably `static`? Sure, I will add the static. > > > + > > static struct sk_buff *napi_skb_cache_get(void) > > { > > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); > > @@ -293,7 +295,7 @@ static struct sk_buff *napi_skb_cache_get(void) > > > > skb = nc->skb_cache[--nc->skb_count]; > > local_unlock_nested_bh(&napi_alloc_cache.bh_lock); > > - kasan_mempool_unpoison_object(skb, > > kmem_cache_size(net_hotdata.skbuff_cache)); > > + kasan_mempool_unpoison_object(skb, skbuff_cache_size); > > > > return skb; > > } > > @@ -1428,7 +1430,7 @@ static void napi_skb_cache_put(struct sk_buff *skb) > > if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) { > > for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++) > > kasan_mempool_unpoison_object(nc->skb_cache[i], > > - > > kmem_cache_size(net_hotdata.skbuff_cache)); > > + skbuff_cache_size); > > > > kmem_cache_free_bulk(net_hotdata.skbuff_cache, > > NAPI_SKB_CACHE_HALF, > > nc->skb_cache + NAPI_SKB_CACHE_HALF); > > @@ -5116,6 +5118,8 @@ void __init skb_init(void) > > offsetof(struct sk_buff, cb), > > sizeof_field(struct sk_buff, cb), > > NULL); > > + skbuff_cache_size = kmem_cache_size(net_hotdata.skbuff_cache); > > + > > net_hotdata.skbuff_fclone_cache = > > kmem_cache_create("skbuff_fclone_cache", > > sizeof(struct sk_buff_fclones), > > 0, > > Thanks, > Olek ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-10-16 15:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-15 23:38 [PATCH net-next] net: shrink napi_skb_cache_put() Eric Dumazet 2025-10-16 0:14 ` Kuniyuki Iwashima 2025-10-16 10:20 ` Alexander Lobakin 2025-10-16 10:31 ` Eric Dumazet 2025-10-16 11:07 ` Alexander Lobakin 2025-10-16 12:56 ` Eric Dumazet 2025-10-16 13:29 ` Eric Dumazet 2025-10-16 13:36 ` Eric Dumazet 2025-10-16 15:24 ` Alexander Lobakin 2025-10-16 15:27 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).