* [PATCH] net: don't check skb_count twice
@ 2022-06-15 3:24 Sieng Piaw Liew
2022-06-15 12:00 ` patchwork-bot+netdevbpf
2022-06-15 15:35 ` Alexander Lobakin
0 siblings, 2 replies; 5+ messages in thread
From: Sieng Piaw Liew @ 2022-06-15 3:24 UTC (permalink / raw)
To: davem, edumazet, kuba, netdev, linux-kernel; +Cc: Sieng Piaw Liew
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;
+ }
skb = nc->skb_cache[--nc->skb_count];
kasan_unpoison_object_data(skbuff_head_cache, skb);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: don't check skb_count twice
2022-06-15 3:24 [PATCH] net: don't check skb_count twice Sieng Piaw Liew
@ 2022-06-15 12:00 ` patchwork-bot+netdevbpf
2022-06-15 15:35 ` Alexander Lobakin
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-06-15 12:00 UTC (permalink / raw)
To: Sieng Piaw Liew; +Cc: davem, edumazet, kuba, netdev, linux-kernel
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: don't check skb_count twice
2022-06-15 3:24 [PATCH] net: don't check skb_count twice Sieng Piaw Liew
2022-06-15 12:00 ` patchwork-bot+netdevbpf
@ 2022-06-15 15:35 ` Alexander Lobakin
2022-06-16 2:04 ` Sieng Piaw Liew
1 sibling, 1 reply; 5+ messages in thread
From: Alexander Lobakin @ 2022-06-15 15:35 UTC (permalink / raw)
To: Sieng Piaw Liew; +Cc: Alexander Lobakin, davem, edumazet, kuba, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: don't check skb_count twice
2022-06-15 15:35 ` Alexander Lobakin
@ 2022-06-16 2:04 ` Sieng Piaw Liew
2022-06-16 13:38 ` Alexander Lobakin
0 siblings, 1 reply; 5+ messages in thread
From: Sieng Piaw Liew @ 2022-06-16 2:04 UTC (permalink / raw)
To: Alexander Lobakin; +Cc: davem, edumazet, kuba, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: don't check skb_count twice
2022-06-16 2:04 ` Sieng Piaw Liew
@ 2022-06-16 13:38 ` Alexander Lobakin
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Lobakin @ 2022-06-16 13:38 UTC (permalink / raw)
To: Sieng Piaw Liew; +Cc: Alexander Lobakin, davem, edumazet, kuba, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-16 13:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-15 3:24 [PATCH] net: don't check skb_count twice Sieng Piaw Liew
2022-06-15 12:00 ` patchwork-bot+netdevbpf
2022-06-15 15:35 ` Alexander Lobakin
2022-06-16 2:04 ` Sieng Piaw Liew
2022-06-16 13:38 ` Alexander Lobakin
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).