| Message ID | 20220615032426.17214-1-liew.s.piaw@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 49ae83fc4fd027a4b4cf56bd2c94c7814ec4baff |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: don't check skb_count twice | expand |
Hello: This patch was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Wed, 15 Jun 2022 11:24:26 +0800 you wrote: > NAPI cache skb_count is being checked twice without condition. Change to > checking the second time only if the first check is run. > > Signed-off-by: Sieng Piaw Liew <liew.s.piaw@gmail.com> > --- > net/core/skbuff.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Here is the summary with links: - net: don't check skb_count twice https://git.kernel.org/netdev/net-next/c/49ae83fc4fd0 You are awesome, thank you!
From: Sieng Piaw Liew <liew.s.piaw@gmail.com> Date: Wed, 15 Jun 2022 11:24:26 +0800 > NAPI cache skb_count is being checked twice without condition. Change to > checking the second time only if the first check is run. > > Signed-off-by: Sieng Piaw Liew <liew.s.piaw@gmail.com> > --- > net/core/skbuff.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 5b3559cb1d82..c426adff6d96 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -172,13 +172,14 @@ static struct sk_buff *napi_skb_cache_get(void) > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); > struct sk_buff *skb; > > - if (unlikely(!nc->skb_count)) > + if (unlikely(!nc->skb_count)) { > nc->skb_count = kmem_cache_alloc_bulk(skbuff_head_cache, > GFP_ATOMIC, > NAPI_SKB_CACHE_BULK, > nc->skb_cache); > - if (unlikely(!nc->skb_count)) > - return NULL; > + if (unlikely(!nc->skb_count)) > + return NULL; > + } I was sure the compilers are able to see that if the condition is false first time, it will be the second as well. Just curious, have you consulted objdump/objdiff to look whether anything changed? Also, please use scripts/get_maintainers.pl or at least git blame and add the original authors to Ccs next time, so that they won't miss your changes and will be able to review them in time. E.g. I noticed this patch only when it did hit the net-next tree already, as I don't monitor LKML 24/7 (but I do that with my mailbox). > > skb = nc->skb_cache[--nc->skb_count]; > kasan_unpoison_object_data(skbuff_head_cache, skb); > -- > 2.17.1 Thanks, Olek
On Wed, Jun 15, 2022 at 05:35:25PM +0200, Alexander Lobakin wrote: > From: Sieng Piaw Liew <liew.s.piaw@gmail.com> > Date: Wed, 15 Jun 2022 11:24:26 +0800 > > > NAPI cache skb_count is being checked twice without condition. Change to > > checking the second time only if the first check is run. > > > > Signed-off-by: Sieng Piaw Liew <liew.s.piaw@gmail.com> > > --- > > net/core/skbuff.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 5b3559cb1d82..c426adff6d96 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -172,13 +172,14 @@ static struct sk_buff *napi_skb_cache_get(void) > > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); > > struct sk_buff *skb; > > > > - if (unlikely(!nc->skb_count)) > > + if (unlikely(!nc->skb_count)) { > > nc->skb_count = kmem_cache_alloc_bulk(skbuff_head_cache, > > GFP_ATOMIC, > > NAPI_SKB_CACHE_BULK, > > nc->skb_cache); > > - if (unlikely(!nc->skb_count)) > > - return NULL; > > + if (unlikely(!nc->skb_count)) > > + return NULL; > > + } > > I was sure the compilers are able to see that if the condition is > false first time, it will be the second as well. Just curious, have > you consulted objdump/objdiff to look whether anything changed? I'm a total noob at this. Thanks for the guidance. Here is the diff I just generated: < before patch > after patch 619,620c619,620 < 14: 24630000 addiu v1,v1,0 < 18: 00021080 sll v0,v0,0x2 --- > 14: 00021080 sll v0,v0,0x2 > 18: 24630000 addiu v1,v1,0 626,635c626,635 < 30: 8e030010 lw v1,16(s0) < 34: 1060000b beqz v1,64 <napi_skb_cache_get+0x64> < 38: 3c020000 lui v0,0x0 < 3c: 24620003 addiu v0,v1,3 < 40: 2463ffff addiu v1,v1,-1 < 44: ae030010 sw v1,16(s0) < 48: 8fbf0014 lw ra,20(sp) < 4c: 00021080 sll v0,v0,0x2 < 50: 02028021 addu s0,s0,v0 < 54: 8e020004 lw v0,4(s0) --- > 30: 8e020010 lw v0,16(s0) > 34: 1040000b beqz v0,64 <napi_skb_cache_get+0x64> > 38: 26070014 addiu a3,s0,20 > 3c: 24430003 addiu v1,v0,3 > 40: 00031880 sll v1,v1,0x2 > 44: 2442ffff addiu v0,v0,-1 > 48: ae020010 sw v0,16(s0) > 4c: 02038021 addu s0,s0,v1 > 50: 8e020004 lw v0,4(s0) > 54: 8fbf0014 lw ra,20(sp) 639,640c639,640 < 64: 8c440000 lw a0,0(v0) < 68: 26070014 addiu a3,s0,20 --- > 64: 3c020000 lui v0,0x0 > 68: 8c440000 lw a0,0(v0) 644c644 < 78: 00401825 move v1,v0 --- > 78: 1440fff0 bnez v0,3c <napi_skb_cache_get+0x3c> 646c646 < 80: 1460ffee bnez v1,3c <napi_skb_cache_get+0x3c> --- > 80: 1000fff4 b 54 <napi_skb_cache_get+0x54> 648,651d647 < 88: 8fbf0014 lw ra,20(sp) < 8c: 8fb00010 lw s0,16(sp) < 90: 03e00008 jr ra < 94: 27bd0018 addiu sp,sp,24 1736c1732 < 244: 24050ae8 li a1,2792 --- > 244: 24050ae9 li a1,2793 ...(More similar li instruction diffs) I think there are slightly more instructions before patch. > > Also, please use scripts/get_maintainers.pl or at least git blame > and add the original authors to Ccs next time, so that they won't > miss your changes and will be able to review them in time. E.g. I > noticed this patch only when it did hit the net-next tree already, > as I don't monitor LKML 24/7 (but I do that with my mailbox). > Thanks for the tip. > > > > skb = nc->skb_cache[--nc->skb_count]; > > kasan_unpoison_object_data(skbuff_head_cache, skb); > > -- > > 2.17.1 > > Thanks, > Olek
From: Sieng Piaw Liew <liew.s.piaw@gmail.com> Date: Thu, 16 Jun 2022 10:04:53 +0800 > On Wed, Jun 15, 2022 at 05:35:25PM +0200, Alexander Lobakin wrote: > > From: Sieng Piaw Liew <liew.s.piaw@gmail.com> > > Date: Wed, 15 Jun 2022 11:24:26 +0800 > > > > > NAPI cache skb_count is being checked twice without condition. Change to > > > checking the second time only if the first check is run. > > > > > > Signed-off-by: Sieng Piaw Liew <liew.s.piaw@gmail.com> > > > --- > > > net/core/skbuff.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > > index 5b3559cb1d82..c426adff6d96 100644 > > > --- a/net/core/skbuff.c > > > +++ b/net/core/skbuff.c > > > @@ -172,13 +172,14 @@ static struct sk_buff *napi_skb_cache_get(void) > > > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); > > > struct sk_buff *skb; > > > > > > - if (unlikely(!nc->skb_count)) > > > + if (unlikely(!nc->skb_count)) { > > > nc->skb_count = kmem_cache_alloc_bulk(skbuff_head_cache, > > > GFP_ATOMIC, > > > NAPI_SKB_CACHE_BULK, > > > nc->skb_cache); > > > - if (unlikely(!nc->skb_count)) > > > - return NULL; > > > + if (unlikely(!nc->skb_count)) > > > + return NULL; > > > + } > > > > I was sure the compilers are able to see that if the condition is > > false first time, it will be the second as well. Just curious, have > > you consulted objdump/objdiff to look whether anything changed? > > I'm a total noob at this. Thanks for the guidance. > Here is the diff I just generated: > > < before patch > > after patch > > 619,620c619,620 > < 14: 24630000 addiu v1,v1,0 > < 18: 00021080 sll v0,v0,0x2 > --- > > 14: 00021080 sll v0,v0,0x2 > > 18: 24630000 addiu v1,v1,0 > 626,635c626,635 > < 30: 8e030010 lw v1,16(s0) > < 34: 1060000b beqz v1,64 <napi_skb_cache_get+0x64> > < 38: 3c020000 lui v0,0x0 > < 3c: 24620003 addiu v0,v1,3 > < 40: 2463ffff addiu v1,v1,-1 > < 44: ae030010 sw v1,16(s0) > < 48: 8fbf0014 lw ra,20(sp) > < 4c: 00021080 sll v0,v0,0x2 > < 50: 02028021 addu s0,s0,v0 > < 54: 8e020004 lw v0,4(s0) > --- > > 30: 8e020010 lw v0,16(s0) > > 34: 1040000b beqz v0,64 <napi_skb_cache_get+0x64> > > 38: 26070014 addiu a3,s0,20 > > 3c: 24430003 addiu v1,v0,3 > > 40: 00031880 sll v1,v1,0x2 > > 44: 2442ffff addiu v0,v0,-1 > > 48: ae020010 sw v0,16(s0) > > 4c: 02038021 addu s0,s0,v1 > > 50: 8e020004 lw v0,4(s0) > > 54: 8fbf0014 lw ra,20(sp) > 639,640c639,640 > < 64: 8c440000 lw a0,0(v0) > < 68: 26070014 addiu a3,s0,20 > --- > > 64: 3c020000 lui v0,0x0 > > 68: 8c440000 lw a0,0(v0) > 644c644 > < 78: 00401825 move v1,v0 > --- > > 78: 1440fff0 bnez v0,3c <napi_skb_cache_get+0x3c> > 646c646 > < 80: 1460ffee bnez v1,3c <napi_skb_cache_get+0x3c> > --- > > 80: 1000fff4 b 54 <napi_skb_cache_get+0x54> > 648,651d647 > < 88: 8fbf0014 lw ra,20(sp) > < 8c: 8fb00010 lw s0,16(sp) > < 90: 03e00008 jr ra > < 94: 27bd0018 addiu sp,sp,24 > 1736c1732 > < 244: 24050ae8 li a1,2792 > --- > > 244: 24050ae9 li a1,2793 > > ...(More similar li instruction diffs) > I think there are slightly more instructions before patch. Ok, thank you! Then it makes sense. I'll recheck my recent code whether I did it that way again somewhere :D > > > > > Also, please use scripts/get_maintainers.pl or at least git blame > > and add the original authors to Ccs next time, so that they won't > > miss your changes and will be able to review them in time. E.g. I > > noticed this patch only when it did hit the net-next tree already, > > as I don't monitor LKML 24/7 (but I do that with my mailbox). > > > > Thanks for the tip. > > > > > > > skb = nc->skb_cache[--nc->skb_count]; > > > kasan_unpoison_object_data(skbuff_head_cache, skb); > > > -- > > > 2.17.1 > > > > Thanks, > > Olek Thanks, Olek
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 5b3559cb1d82..c426adff6d96 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -172,13 +172,14 @@ static struct sk_buff *napi_skb_cache_get(void) struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); struct sk_buff *skb; - if (unlikely(!nc->skb_count)) + if (unlikely(!nc->skb_count)) { nc->skb_count = kmem_cache_alloc_bulk(skbuff_head_cache, GFP_ATOMIC, NAPI_SKB_CACHE_BULK, nc->skb_cache); - if (unlikely(!nc->skb_count)) - return NULL; + if (unlikely(!nc->skb_count)) + return NULL; + } skb = nc->skb_cache[--nc->skb_count]; kasan_unpoison_object_data(skbuff_head_cache, skb);
NAPI cache skb_count is being checked twice without condition. Change to checking the second time only if the first check is run. Signed-off-by: Sieng Piaw Liew <liew.s.piaw@gmail.com> --- net/core/skbuff.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)