diff mbox series

net: don't check skb_count twice

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

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers warning 1 maintainers not CCed: pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 6 this patch: 6
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Sieng-Piaw Liew June 15, 2022, 3:24 a.m. UTC
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(-)

Comments

patchwork-bot+netdevbpf@kernel.org June 15, 2022, noon UTC | #1
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!
Alexander Lobakin June 15, 2022, 3:35 p.m. UTC | #2
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
Sieng-Piaw Liew June 16, 2022, 2:04 a.m. UTC | #3
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
Alexander Lobakin June 16, 2022, 1:38 p.m. UTC | #4
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 mbox series

Patch

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);