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" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Vlad Yasevich <[email protected]>
Cc: Neil Horman <[email protected]>
Cc: Marcelo Ricardo Leitner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kefeng Wang <[email protected]>
---
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
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. :-)
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" <[email protected]>
> Cc: Alexey Kuznetsov <[email protected]>
> Cc: Hideaki YOSHIFUJI <[email protected]>
> Cc: Vlad Yasevich <[email protected]>
> Cc: Neil Horman <[email protected]>
> Cc: Marcelo Ricardo Leitner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
> 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 <[email protected]>
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. :-)
>
>
> .
>
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
[email protected] -- +49-151-27565287