netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)
       [not found] <20190605142428.84784-1-wangkefeng.wang@huawei.com>
@ 2019-06-05 14:24 ` Kefeng Wang
  2019-06-05 16:13   ` Jesse Brandeburg
  2019-06-05 16:40   ` Neil Horman
  0 siblings, 2 replies; 5+ messages in thread
From: Kefeng Wang @ 2019-06-05 14:24 UTC (permalink / raw)
  To: linux-kernel
  Cc: Kefeng Wang, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Vlad Yasevich, Neil Horman,
	Marcelo Ricardo Leitner, netdev, linux-sctp

IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag,
so no need to do that again from its callers. Drop it.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-sctp@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/net/udp.h           | 2 +-
 net/ipv4/fib_semantics.c    | 2 +-
 net/ipv4/inet_hashtables.c  | 2 +-
 net/ipv4/udp.c              | 2 +-
 net/ipv4/udp_offload.c      | 2 +-
 net/ipv6/inet6_hashtables.c | 2 +-
 net/ipv6/udp.c              | 2 +-
 net/sctp/socket.c           | 4 ++--
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 79d141d2103b..bad74f780831 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -480,7 +480,7 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
 	 * CB fragment
 	 */
 	segs = __skb_gso_segment(skb, features, false);
-	if (unlikely(IS_ERR_OR_NULL(segs))) {
+	if (IS_ERR_OR_NULL(segs)) {
 		int segs_nr = skb_shinfo(skb)->gso_segs;
 
 		atomic_add(segs_nr, &sk->sk_drops);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index b80410673915..cd35bd0a4d8a 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1295,7 +1295,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto failure;
 	fi->fib_metrics = ip_fib_metrics_init(fi->fib_net, cfg->fc_mx,
 					      cfg->fc_mx_len, extack);
-	if (unlikely(IS_ERR(fi->fib_metrics))) {
+	if (IS_ERR(fi->fib_metrics)) {
 		err = PTR_ERR(fi->fib_metrics);
 		kfree(fi);
 		return ERR_PTR(err);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index c4503073248b..97824864e40d 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -316,7 +316,7 @@ struct sock *__inet_lookup_listener(struct net *net,
 				    saddr, sport, htonl(INADDR_ANY), hnum,
 				    dif, sdif);
 done:
-	if (unlikely(IS_ERR(result)))
+	if (IS_ERR(result))
 		return NULL;
 	return result;
 }
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 189144346cd4..8983afe2fe9e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -478,7 +478,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 					  htonl(INADDR_ANY), hnum, dif, sdif,
 					  exact_dif, hslot2, skb);
 	}
-	if (unlikely(IS_ERR(result)))
+	if (IS_ERR(result))
 		return NULL;
 	return result;
 }
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 06b3e2c1fcdc..0112f64faf69 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -208,7 +208,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 		gso_skb->destructor = NULL;
 
 	segs = skb_segment(gso_skb, features);
-	if (unlikely(IS_ERR_OR_NULL(segs))) {
+	if (IS_ERR_OR_NULL(segs)) {
 		if (copy_dtor)
 			gso_skb->destructor = sock_wfree;
 		return segs;
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index b2a55f300318..cf60fae9533b 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -174,7 +174,7 @@ struct sock *inet6_lookup_listener(struct net *net,
 				     saddr, sport, &in6addr_any, hnum,
 				     dif, sdif);
 done:
-	if (unlikely(IS_ERR(result)))
+	if (IS_ERR(result))
 		return NULL;
 	return result;
 }
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index b3418a7c5c74..693518350f79 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -215,7 +215,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 					  exact_dif, hslot2,
 					  skb);
 	}
-	if (unlikely(IS_ERR(result)))
+	if (IS_ERR(result))
 		return NULL;
 	return result;
 }
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 39ea0a37af09..c7b0f51c19d5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -985,7 +985,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 		return -EINVAL;
 
 	kaddrs = memdup_user(addrs, addrs_size);
-	if (unlikely(IS_ERR(kaddrs)))
+	if (IS_ERR(kaddrs))
 		return PTR_ERR(kaddrs);
 
 	/* Walk through the addrs buffer and count the number of addresses. */
@@ -1315,7 +1315,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 		return -EINVAL;
 
 	kaddrs = memdup_user(addrs, addrs_size);
-	if (unlikely(IS_ERR(kaddrs)))
+	if (IS_ERR(kaddrs))
 		return PTR_ERR(kaddrs);
 
 	/* Allow security module to validate connectx addresses. */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)
  2019-06-05 14:24 ` [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL) Kefeng Wang
@ 2019-06-05 16:13   ` Jesse Brandeburg
  2019-06-06  1:39     ` Kefeng Wang
  2019-06-05 16:40   ` Neil Horman
  1 sibling, 1 reply; 5+ messages in thread
From: Jesse Brandeburg @ 2019-06-05 16:13 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Vlad Yasevich, Neil Horman,
	Marcelo Ricardo Leitner, netdev, linux-sctp, jesse.brandeburg

On Wed, 5 Jun 2019 22:24:26 +0800 Kefeng wrote:
> IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag,
> so no need to do that again from its callers. Drop it.
> 

<snip>

>  	segs = __skb_gso_segment(skb, features, false);
> -	if (unlikely(IS_ERR_OR_NULL(segs))) {
> +	if (IS_ERR_OR_NULL(segs)) {
>  		int segs_nr = skb_shinfo(skb)->gso_segs;
>  

The change itself seems reasonable, but did you check to see if the
paths changed are faster/slower with your fix?  Did you look at any
assembly output to see if the compiler actually generated different
code?  Is there a set of similar changes somewhere else in the kernel
we can refer to?

I'm not sure in the end that the change is worth it, so would like you
to prove it is, unless davem overrides me. :-)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)
  2019-06-05 14:24 ` [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL) Kefeng Wang
  2019-06-05 16:13   ` Jesse Brandeburg
@ 2019-06-05 16:40   ` Neil Horman
  1 sibling, 0 replies; 5+ messages in thread
From: Neil Horman @ 2019-06-05 16:40 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Vlad Yasevich, Marcelo Ricardo Leitner,
	netdev, linux-sctp

On Wed, Jun 05, 2019 at 10:24:26PM +0800, Kefeng Wang wrote:
> IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag,
> so no need to do that again from its callers. Drop it.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-sctp@vger.kernel.org
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  include/net/udp.h           | 2 +-
>  net/ipv4/fib_semantics.c    | 2 +-
>  net/ipv4/inet_hashtables.c  | 2 +-
>  net/ipv4/udp.c              | 2 +-
>  net/ipv4/udp_offload.c      | 2 +-
>  net/ipv6/inet6_hashtables.c | 2 +-
>  net/ipv6/udp.c              | 2 +-
>  net/sctp/socket.c           | 4 ++--
>  8 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 79d141d2103b..bad74f780831 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -480,7 +480,7 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
>  	 * CB fragment
>  	 */
>  	segs = __skb_gso_segment(skb, features, false);
> -	if (unlikely(IS_ERR_OR_NULL(segs))) {
> +	if (IS_ERR_OR_NULL(segs)) {
>  		int segs_nr = skb_shinfo(skb)->gso_segs;
>  
>  		atomic_add(segs_nr, &sk->sk_drops);
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index b80410673915..cd35bd0a4d8a 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1295,7 +1295,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
>  		goto failure;
>  	fi->fib_metrics = ip_fib_metrics_init(fi->fib_net, cfg->fc_mx,
>  					      cfg->fc_mx_len, extack);
> -	if (unlikely(IS_ERR(fi->fib_metrics))) {
> +	if (IS_ERR(fi->fib_metrics)) {
>  		err = PTR_ERR(fi->fib_metrics);
>  		kfree(fi);
>  		return ERR_PTR(err);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index c4503073248b..97824864e40d 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -316,7 +316,7 @@ struct sock *__inet_lookup_listener(struct net *net,
>  				    saddr, sport, htonl(INADDR_ANY), hnum,
>  				    dif, sdif);
>  done:
> -	if (unlikely(IS_ERR(result)))
> +	if (IS_ERR(result))
>  		return NULL;
>  	return result;
>  }
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 189144346cd4..8983afe2fe9e 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -478,7 +478,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
>  					  htonl(INADDR_ANY), hnum, dif, sdif,
>  					  exact_dif, hslot2, skb);
>  	}
> -	if (unlikely(IS_ERR(result)))
> +	if (IS_ERR(result))
>  		return NULL;
>  	return result;
>  }
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 06b3e2c1fcdc..0112f64faf69 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -208,7 +208,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  		gso_skb->destructor = NULL;
>  
>  	segs = skb_segment(gso_skb, features);
> -	if (unlikely(IS_ERR_OR_NULL(segs))) {
> +	if (IS_ERR_OR_NULL(segs)) {
>  		if (copy_dtor)
>  			gso_skb->destructor = sock_wfree;
>  		return segs;
> diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
> index b2a55f300318..cf60fae9533b 100644
> --- a/net/ipv6/inet6_hashtables.c
> +++ b/net/ipv6/inet6_hashtables.c
> @@ -174,7 +174,7 @@ struct sock *inet6_lookup_listener(struct net *net,
>  				     saddr, sport, &in6addr_any, hnum,
>  				     dif, sdif);
>  done:
> -	if (unlikely(IS_ERR(result)))
> +	if (IS_ERR(result))
>  		return NULL;
>  	return result;
>  }
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index b3418a7c5c74..693518350f79 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -215,7 +215,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
>  					  exact_dif, hslot2,
>  					  skb);
>  	}
> -	if (unlikely(IS_ERR(result)))
> +	if (IS_ERR(result))
>  		return NULL;
>  	return result;
>  }
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 39ea0a37af09..c7b0f51c19d5 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -985,7 +985,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  		return -EINVAL;
>  
>  	kaddrs = memdup_user(addrs, addrs_size);
> -	if (unlikely(IS_ERR(kaddrs)))
> +	if (IS_ERR(kaddrs))
>  		return PTR_ERR(kaddrs);
>  
>  	/* Walk through the addrs buffer and count the number of addresses. */
> @@ -1315,7 +1315,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  		return -EINVAL;
>  
>  	kaddrs = memdup_user(addrs, addrs_size);
> -	if (unlikely(IS_ERR(kaddrs)))
> +	if (IS_ERR(kaddrs))
>  		return PTR_ERR(kaddrs);
>  
>  	/* Allow security module to validate connectx addresses. */
> -- 
> 2.20.1
> 
> 
for the SCTP bits
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)
  2019-06-05 16:13   ` Jesse Brandeburg
@ 2019-06-06  1:39     ` Kefeng Wang
  2019-06-06 20:41       ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 5+ messages in thread
From: Kefeng Wang @ 2019-06-06  1:39 UTC (permalink / raw)
  To: Jesse Brandeburg
  Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Vlad Yasevich, Neil Horman,
	Marcelo Ricardo Leitner, netdev, linux-sctp, Enrico Weigelt,
	metux IT consult


On 2019/6/6 0:13, Jesse Brandeburg wrote:
> On Wed, 5 Jun 2019 22:24:26 +0800 Kefeng wrote:
>> IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag,
>> so no need to do that again from its callers. Drop it.
>>
> <snip>
>
>>  	segs = __skb_gso_segment(skb, features, false);
>> -	if (unlikely(IS_ERR_OR_NULL(segs))) {
>> +	if (IS_ERR_OR_NULL(segs)) {
>>  		int segs_nr = skb_shinfo(skb)->gso_segs;
>>  
> The change itself seems reasonable, but did you check to see if the
> paths changed are faster/slower with your fix?  Did you look at any
> assembly output to see if the compiler actually generated different
> code?  Is there a set of similar changes somewhere else in the kernel
> we can refer to?

+Enrico Weigelt

There is no different in assembly output (only check the x86/arm64), and

the Enrico Weigelt have finished a cocci script to do this cleanup.


>
> I'm not sure in the end that the change is worth it, so would like you
> to prove it is, unless davem overrides me. :-)
>
>
> .
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)
  2019-06-06  1:39     ` Kefeng Wang
@ 2019-06-06 20:41       ` Enrico Weigelt, metux IT consult
  0 siblings, 0 replies; 5+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-06-06 20:41 UTC (permalink / raw)
  To: Kefeng Wang, Jesse Brandeburg
  Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Vlad Yasevich, Neil Horman,
	Marcelo Ricardo Leitner, netdev, linux-sctp

On 06.06.19 03:39, Kefeng Wang wrote:

Hi folks,

> There is no different in assembly output (only check the x86/arm64), and
> the Enrico Weigelt have finished a cocci script to do this cleanup.

I haven't compared the assembly output, just logically deduced from the
macro. If I understand it correctly, the likely()/unlikely() macros
just add a hint to the compiler, which branch it should optimize harder.

No idea what the compiler's actually doing, but I believe its things
like optimized shortcut evaluation and avoiding jumps to likely
branches.

>> I'm not sure in the end that the change is worth it, so would like you
>> to prove it is, unless davem overrides me. :-)

Depends on what you count as worthy ;-)

This patch just makes a source a bit more compact / easier to read.
But shouldn't have any actual consequence on the generated binary.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-06-06 20:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190605142428.84784-1-wangkefeng.wang@huawei.com>
2019-06-05 14:24 ` [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL) Kefeng Wang
2019-06-05 16:13   ` Jesse Brandeburg
2019-06-06  1:39     ` Kefeng Wang
2019-06-06 20:41       ` Enrico Weigelt, metux IT consult
2019-06-05 16:40   ` Neil Horman

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