[PATCH net-next] gro: small napi_get_frags() optim

[PATCH net-next] gro: small napi_get_frags() optim

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Remove one useless conditional branch : 
napi->skb is NULL, so nothing bad can happen.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ba3b7ea5ebb3..c98052487e98 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3981,8 +3981,7 @@ struct sk_buff *napi_get_frags(struct napi_struct *napi)
 
 	if (!skb) {
 		skb = netdev_alloc_skb_ip_align(napi->dev, GRO_MAX_HEAD);
-		if (skb)
-			napi->skb = skb;
+		napi->skb = skb;
 	}
 	return skb;
 }

Re: [PATCH net-next] gro: small napi_get_frags() optim

[Joe Perches]

On Thu, 2013-12-05 at 21:44 -0800, Eric Dumazet wrote:
> Remove one useless conditional branch : 
> napi->skb is NULL, so nothing bad can happen.
[]
> diff --git a/net/core/dev.c b/net/core/dev.c
[]
> @@ -3981,8 +3981,7 @@ struct sk_buff *napi_get_frags(struct napi_struct *napi)
>  
>  	if (!skb) {
>  		skb = netdev_alloc_skb_ip_align(napi->dev, GRO_MAX_HEAD);
> -		if (skb)
> -			napi->skb = skb;
> +		napi->skb = skb;

Thanks, I agree this patch is appropriate as skb is unlikely
to be NULL in normal cases.

But just for thought: isn't the test expense essentially
free when skb is in a register vs the member set cost?

Also, netdev_alloc_skb_ip_align is static inline and it has
a test of
	if (NET_IP_ALIGN && skb)
just before the return so this "if (skb)" test could already
be elided by a good compiler but this change requires an
unconditional set.

Re: [PATCH net-next] gro: small napi_get_frags() optim

[Eric Dumazet]

On Thu, 2013-12-05 at 22:11 -0800, Joe Perches wrote:

> Thanks, I agree this patch is appropriate as skb is unlikely
> to be NULL in normal cases.

We do not care if skb is NULL or not.

> 
> But just for thought: isn't the test expense essentially
> free when skb is in a register vs the member set cost?
> 

Test is not 'free', even if predicted.

Study the generated assembly code if you are interested, you'll see this
makes a difference.

> Also, netdev_alloc_skb_ip_align is static inline and it has
> a test of
> 	if (NET_IP_ALIGN && skb)
> just before the return so this "if (skb)" test could already
> be elided by a good compiler but this change requires an
> unconditional set.

NET_IP_ALIGN is 0 on x86, even a 'bad' compiler zaps the whole thing.

Re: [PATCH net-next] gro: small napi_get_frags() optim

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 05 Dec 2013 21:44:27 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> Remove one useless conditional branch : 
> napi->skb is NULL, so nothing bad can happen.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.