Message ID | 20190605142428.84784-3-wangkefeng.wang@huawei.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: Drop unlikely before IS_ERR(_OR_NULL) | expand |
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" <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>
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
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. */
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(-)