Re: [PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

Re: [PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

[Jens Axboe]

On 6/5/19 8:24 AM, 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.

Applied, thanks.

-- 
Jens Axboe

[PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

Use IS_ERR_OR_NULL where appropriate.

Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: cluster-devel@redhat.com
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/gfs2/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index db9a05244a35..3925efd3fd83 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -857,7 +857,7 @@ static struct gfs2_dirent *gfs2_dirent_search(struct inode *inode,
 		return ERR_PTR(error);
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size, scan, name, NULL);
 got_dent:
-	if (unlikely(dent == NULL || IS_ERR(dent))) {
+	if (IS_ERR_OR_NULL(dent)) {
 		brelse(bh);
 		bh = NULL;
 	}
-- 
2.20.1

[Cluster-devel] [PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

Use IS_ERR_OR_NULL where appropriate.

Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: cluster-devel at redhat.com
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/gfs2/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index db9a05244a35..3925efd3fd83 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -857,7 +857,7 @@ static struct gfs2_dirent *gfs2_dirent_search(struct inode *inode,
 		return ERR_PTR(error);
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size, scan, name, NULL);
 got_dent:
-	if (unlikely(dent == NULL || IS_ERR(dent))) {
+	if (IS_ERR_OR_NULL(dent)) {
 		brelse(bh);
 		bh = NULL;
 	}
-- 
2.20.1

[PATCH] fs: cifs: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Cc: Steve French <stfrench@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/cifs/dfs_cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/dfs_cache.c b/fs/cifs/dfs_cache.c
index e3e1c13df439..1692c0c6c23a 100644
--- a/fs/cifs/dfs_cache.c
+++ b/fs/cifs/dfs_cache.c
@@ -492,7 +492,7 @@ static struct dfs_cache_entry *__find_cache_entry(unsigned int hash,
 #ifdef CONFIG_CIFS_DEBUG2
 			char *name = get_tgt_name(ce);
 
-			if (unlikely(IS_ERR(name))) {
+			if (IS_ERR(name)) {
 				rcu_read_unlock();
 				return ERR_CAST(name);
 			}
-- 
2.20.1

[PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

[PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

[PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 block/blk-cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index b97b479e4f64..1f7127b03490 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -881,7 +881,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 			blkg_free(new_blkg);
 		} else {
 			blkg = blkg_create(pos, q, new_blkg);
-			if (unlikely(IS_ERR(blkg))) {
+			if (IS_ERR(blkg)) {
 				ret = PTR_ERR(blkg);
 				goto fail_unlock;
 			}
-- 
2.20.1

[PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Cc: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/input/mouse/alps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 0a6f7ca883e7..791ef0f826c5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 		/* On V2 devices the DualPoint Stick reports bare packets */
 		dev = priv->dev2;
 		dev2 = psmouse->dev;
-	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+	} else if (IS_ERR_OR_NULL(priv->dev3)) {
 		/* Register dev3 mouse if we received PS/2 packet first time */
 		if (!IS_ERR(priv->dev3))
 			psmouse_queue_work(psmouse, &priv->dev3_register_work,
-- 
2.20.1

[PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Cc: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/input/mouse/alps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 0a6f7ca883e7..791ef0f826c5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 		/* On V2 devices the DualPoint Stick reports bare packets */
 		dev = priv->dev2;
 		dev2 = psmouse->dev;
-	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+	} else if (IS_ERR_OR_NULL(priv->dev3)) {
 		/* Register dev3 mouse if we received PS/2 packet first time */
 		if (!IS_ERR(priv->dev3))
 			psmouse_queue_work(psmouse, &priv->dev3_register_work,
-- 
2.20.1

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Pali Rohár]

On Wednesday 05 June 2019 22:24:28 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.

Hi! I already reviewed this patch and rejected it, see:
https://patchwork.kernel.org/patch/10817475/

> Cc: "Pali Rohár" <pali.rohar@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  drivers/input/mouse/alps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 0a6f7ca883e7..791ef0f826c5 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>  		/* On V2 devices the DualPoint Stick reports bare packets */
>  		dev = priv->dev2;
>  		dev2 = psmouse->dev;
> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>  		/* Register dev3 mouse if we received PS/2 packet first time */
>  		if (!IS_ERR(priv->dev3))
>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

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

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

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

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Neil Horman]

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>

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Neil Horman]

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>

Re: [PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

[Enrico Weigelt, metux IT consult]

On 05.06.19 14:24, Kefeng Wang wrote:

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index b97b479e4f64..1f7127b03490 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -881,7 +881,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>   			blkg_free(new_blkg);
>   		} else {
>   			blkg = blkg_create(pos, q, new_blkg);
> -			if (unlikely(IS_ERR(blkg))) {
> +			if (IS_ERR(blkg)) {
>   				ret = PTR_ERR(blkg);
>   				goto fail_unlock;
>   			}
> 

I think this cocci script should do such things automatically:

virtual patch
virtual context
virtual org
virtual report

@@
expression E;
@@
- unlikely(IS_ERR(E))
+ IS_ERR(E)


Do you already do it this way and have a cocci scrip on the way
to mainline ?


--mtx


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

Re: [PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

[Joe Perches]

On Wed, 2019-06-05 at 18:24 +0000, Enrico Weigelt, metux IT consult
wrote:
> On 05.06.19 14:24, Kefeng Wang wrote:
> 
> > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > index b97b479e4f64..1f7127b03490 100644
> > --- a/block/blk-cgroup.c
> > +++ b/block/blk-cgroup.c
> > @@ -881,7 +881,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
> >   			blkg_free(new_blkg);
> >   		} else {
> >   			blkg = blkg_create(pos, q, new_blkg);
> > -			if (unlikely(IS_ERR(blkg))) {
> > +			if (IS_ERR(blkg)) {
> >   				ret = PTR_ERR(blkg);
> >   				goto fail_unlock;
> >   			}
> > 
> 
> I think this cocci script should do such things automatically:
> 
> virtual patch
> virtual context
> virtual org
> virtual report
> 
> @@
> expression E;
> @@
> - unlikely(IS_ERR(E))
> + IS_ERR(E)

Likely change all of these too:

$ git grep -P '\blikely.*IS_ERR'
drivers/net/vxlan.c:    if (likely(!IS_ERR(rt))) {
fs/ntfs/lcnalloc.c:     if (likely(page && !IS_ERR(page))) {
fs/ntfs/mft.c:  if (likely(!IS_ERR(page))) {
fs/ntfs/mft.c:  if (likely(!IS_ERR(m)))
fs/ntfs/mft.c:          if (likely(!IS_ERR(m))) {
fs/ntfs/mft.c:          if (likely(!IS_ERR(rl2)))
fs/ntfs/namei.c:                if (likely(!IS_ERR(dent_inode))) {
fs/ntfs/runlist.c:      if (likely(!IS_ERR(old_rl)))
net/openvswitch/datapath.c:             if (likely(!IS_ERR(reply))) {
net/socket.c:   if (likely(!IS_ERR(newfile))) {

Re: [PATCH] block: Drop unlikely before IS_ERR(_OR_NULL)

[Enrico Weigelt, metux IT consult]

On 05.06.19 18:32, Joe Perches wrote:

> Likely change all of these too:
> 
> $ git grep -P '\blikely.*IS_ERR'
> drivers/net/vxlan.c:    if (likely(!IS_ERR(rt))) {
> fs/ntfs/lcnalloc.c:     if (likely(page && !IS_ERR(page))) {
> fs/ntfs/mft.c:  if (likely(!IS_ERR(page))) {
> fs/ntfs/mft.c:  if (likely(!IS_ERR(m)))
> fs/ntfs/mft.c:          if (likely(!IS_ERR(m))) {
> fs/ntfs/mft.c:          if (likely(!IS_ERR(rl2)))
> fs/ntfs/namei.c:                if (likely(!IS_ERR(dent_inode))) {
> fs/ntfs/runlist.c:      if (likely(!IS_ERR(old_rl)))
> net/openvswitch/datapath.c:             if (likely(!IS_ERR(reply))) {
> net/socket.c:   if (likely(!IS_ERR(newfile))) {

Good point, thx.

Added that to my cocci script, which I've just sent to lkml.


--mtx

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

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

On 2019/6/5 22:42, Pali Rohár wrote:
> On Wednesday 05 June 2019 22:24:28 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.
> Hi! I already reviewed this patch and rejected it, see:
> https://patchwork.kernel.org/patch/10817475/
OK, please ignore it.
>> Cc: "Pali Rohár" <pali.rohar@gmail.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: linux-input@vger.kernel.org
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  drivers/input/mouse/alps.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 0a6f7ca883e7..791ef0f826c5 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>>  		/* On V2 devices the DualPoint Stick reports bare packets */
>>  		dev = priv->dev2;
>>  		dev2 = psmouse->dev;
>> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
>> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>>  		/* Register dev3 mouse if we received PS/2 packet first time */
>>  		if (!IS_ERR(priv->dev3))
>>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

On 2019/6/5 22:42, Pali Rohár wrote:
> On Wednesday 05 June 2019 22:24:28 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.
> Hi! I already reviewed this patch and rejected it, see:
> https://patchwork.kernel.org/patch/10817475/
OK, please ignore it.
>> Cc: "Pali Rohár" <pali.rohar@gmail.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: linux-input@vger.kernel.org
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  drivers/input/mouse/alps.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 0a6f7ca883e7..791ef0f826c5 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>>  		/* On V2 devices the DualPoint Stick reports bare packets */
>>  		dev = priv->dev2;
>>  		dev2 = psmouse->dev;
>> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
>> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>>  		/* Register dev3 mouse if we received PS/2 packet first time */
>>  		if (!IS_ERR(priv->dev3))
>>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Kefeng Wang]

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

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Joe Perches]

On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote:
> On 2019/6/5 22:42, Pali Rohár wrote:
> > On Wednesday 05 June 2019 22:24:28 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.
> > Hi! I already reviewed this patch and rejected it, see:
> > https://patchwork.kernel.org/patch/10817475/
> OK, please ignore it.

I think the stated reason of better readability isn't
particularly sensible as the object code produced is
actually slightly larger.

x86-64 defconfig (gcc 8.3.0)

$ size drivers/input/mouse/alps.o*
   text	   data	    bss	    dec	    hex	filename
  29416	     56	      0	  29472	   7320	drivers/input/mouse/alps.o.new
  29432	     56	      0	  29488	   7330	drivers/input/mouse/alps.o.old

Also if this unlikely is _really_ useful, perhaps the
!IS_ERR immediately after could also use likely as the
test seems only done for an OOM condition.

> > > Cc: "Pali Rohár" <pali.rohar@gmail.com>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: linux-input@vger.kernel.org
> > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > > ---
> > >  drivers/input/mouse/alps.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > > index 0a6f7ca883e7..791ef0f826c5 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -1478,7 +1478,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> > >  		/* On V2 devices the DualPoint Stick reports bare packets */
> > >  		dev = priv->dev2;
> > >  		dev2 = psmouse->dev;
> > > -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> > > +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
> > >  		/* Register dev3 mouse if we received PS/2 packet first time */
> > >  		if (!IS_ERR(priv->dev3))
> > >  			psmouse_queue_work(psmouse, &priv->dev3_register_work,

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Enrico Weigelt, metux IT consult]

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

Re: [PATCH net-next] net: Drop unlikely before IS_ERR(_OR_NULL)

[Enrico Weigelt, metux IT consult]

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

Re: [PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Andreas Gruenbacher]

Kefeng,

On Wed, 5 Jun 2019 at 16:17, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> Use IS_ERR_OR_NULL where appropriate.

It seems there are several more instances in which IS_ERR_OR_NULL should
be used (see below).

Thanks,
Andreas

---
 fs/gfs2/dir.c        | 2 +-
 fs/gfs2/glock.c      | 2 +-
 fs/gfs2/inode.c      | 2 +-
 fs/gfs2/ops_fstype.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 3925efd3fd83..761a37a3c656 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -753,7 +753,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
 	struct gfs2_dirent *dent;
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
 				gfs2_dirent_find_offset, name, ptr);
-	if (!dent || IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		return dent;
 	return do_init_dirent(inode, dent, name, bh,
 			      (unsigned)(ptr - (void *)dent));
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 15c605cfcfc8..f6281470a182 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -684,7 +684,7 @@ static void delete_work_func(struct work_struct *work)
 		goto out;
 
 	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
 	}
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 998051c4aea7..1cc99da705fc 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -796,7 +796,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 fail_gunlock:
 	gfs2_dir_no_add(&da);
 	gfs2_glock_dq_uninit(ghs);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		clear_nlink(inode);
 		if (!free_vfs_inode)
 			mark_inode_dirty(inode);
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index f5312f3b58ee..d35772d90b0a 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -579,7 +579,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
 
 		INIT_WORK(&jd->jd_work, gfs2_recover_func);
 		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
-		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
+		if (IS_ERR_OR_NULL(jd->jd_inode)) {
 			if (!jd->jd_inode)
 				error = -ENOENT;
 			else
-- 
2.20.1

[Cluster-devel] [PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Andreas Gruenbacher]

Kefeng,

On Wed, 5 Jun 2019 at 16:17, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> Use IS_ERR_OR_NULL where appropriate.

It seems there are several more instances in which IS_ERR_OR_NULL should
be used (see below).

Thanks,
Andreas

---
 fs/gfs2/dir.c        | 2 +-
 fs/gfs2/glock.c      | 2 +-
 fs/gfs2/inode.c      | 2 +-
 fs/gfs2/ops_fstype.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 3925efd3fd83..761a37a3c656 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -753,7 +753,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
 	struct gfs2_dirent *dent;
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
 				gfs2_dirent_find_offset, name, ptr);
-	if (!dent || IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		return dent;
 	return do_init_dirent(inode, dent, name, bh,
 			      (unsigned)(ptr - (void *)dent));
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 15c605cfcfc8..f6281470a182 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -684,7 +684,7 @@ static void delete_work_func(struct work_struct *work)
 		goto out;
 
 	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
 	}
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 998051c4aea7..1cc99da705fc 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -796,7 +796,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 fail_gunlock:
 	gfs2_dir_no_add(&da);
 	gfs2_glock_dq_uninit(ghs);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		clear_nlink(inode);
 		if (!free_vfs_inode)
 			mark_inode_dirty(inode);
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index f5312f3b58ee..d35772d90b0a 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -579,7 +579,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
 
 		INIT_WORK(&jd->jd_work, gfs2_recover_func);
 		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
-		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
+		if (IS_ERR_OR_NULL(jd->jd_inode)) {
 			if (!jd->jd_inode)
 				error = -ENOENT;
 			else
-- 
2.20.1

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Dmitry Torokhov]

Hi Joe,

On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote:
> On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote:
> > On 2019/6/5 22:42, Pali Rohár wrote:
> > > On Wednesday 05 June 2019 22:24:28 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.
> > > Hi! I already reviewed this patch and rejected it, see:
> > > https://patchwork.kernel.org/patch/10817475/
> > OK, please ignore it.
> 
> I think the stated reason of better readability isn't
> particularly sensible as the object code produced is
> actually slightly larger.
> 
> x86-64 defconfig (gcc 8.3.0)
> 
> $ size drivers/input/mouse/alps.o*
>    text	   data	    bss	    dec	    hex	filename
>   29416	     56	      0	  29472	   7320	drivers/input/mouse/alps.o.new
>   29432	     56	      0	  29488	   7330	drivers/input/mouse/alps.o.old

If gcc produces worse code for double unlikely, you should probably
report it to gcc folks, no? Or double unlikely turns into likely?

> 
> Also if this unlikely is _really_ useful, perhaps the
> !IS_ERR immediately after could also use likely as the
> test seems only done for an OOM condition.

No, once you take the IS_ERR_OR_NULL(priv->dev3) == true branch it stops
being hot path and additional annotations are completely unneeded.

And if we failed to create and register priv->dev3 device - that's an
error and system is degraded. Can't do much here.

Thanks.

-- 
Dmitry

Re: [PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

On 2019/6/12 0:23, Andreas Gruenbacher wrote:
> Kefeng,
> 
> On Wed, 5 Jun 2019 at 16:17, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> Use IS_ERR_OR_NULL where appropriate.
> 
> It seems there are several more instances in which IS_ERR_OR_NULL should
> be used (see below).
> 

Right, will collect the following changes and send a new one, thanks.

> Thanks,
> Andreas
> 
> ---
>  fs/gfs2/dir.c        | 2 +-
>  fs/gfs2/glock.c      | 2 +-
>  fs/gfs2/inode.c      | 2 +-
>  fs/gfs2/ops_fstype.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 3925efd3fd83..761a37a3c656 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -753,7 +753,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
>  	struct gfs2_dirent *dent;
>  	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
>  				gfs2_dirent_find_offset, name, ptr);
> -	if (!dent || IS_ERR(dent))
> +	if (IS_ERR_OR_NULL(dent))
>  		return dent;
>  	return do_init_dirent(inode, dent, name, bh,
>  			      (unsigned)(ptr - (void *)dent));
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 15c605cfcfc8..f6281470a182 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -684,7 +684,7 @@ static void delete_work_func(struct work_struct *work)
>  		goto out;
>  
>  	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
> -	if (inode && !IS_ERR(inode)) {
> +	if (!IS_ERR_OR_NULL(inode)) {
>  		d_prune_aliases(inode);
>  		iput(inode);
>  	}
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 998051c4aea7..1cc99da705fc 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -796,7 +796,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  fail_gunlock:
>  	gfs2_dir_no_add(&da);
>  	gfs2_glock_dq_uninit(ghs);
> -	if (inode && !IS_ERR(inode)) {
> +	if (!IS_ERR_OR_NULL(inode)) {
>  		clear_nlink(inode);
>  		if (!free_vfs_inode)
>  			mark_inode_dirty(inode);
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index f5312f3b58ee..d35772d90b0a 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -579,7 +579,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
>  
>  		INIT_WORK(&jd->jd_work, gfs2_recover_func);
>  		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
> -		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
> +		if (IS_ERR_OR_NULL(jd->jd_inode)) {
>  			if (!jd->jd_inode)
>  				error = -ENOENT;
>  			else
> 

[Cluster-devel] [PATCH] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

On 2019/6/12 0:23, Andreas Gruenbacher wrote:
> Kefeng,
> 
> On Wed, 5 Jun 2019 at 16:17, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>> Use IS_ERR_OR_NULL where appropriate.
> 
> It seems there are several more instances in which IS_ERR_OR_NULL should
> be used (see below).
> 

Right, will collect the following changes and send a new one, thanks.

> Thanks,
> Andreas
> 
> ---
>  fs/gfs2/dir.c        | 2 +-
>  fs/gfs2/glock.c      | 2 +-
>  fs/gfs2/inode.c      | 2 +-
>  fs/gfs2/ops_fstype.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 3925efd3fd83..761a37a3c656 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -753,7 +753,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
>  	struct gfs2_dirent *dent;
>  	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
>  				gfs2_dirent_find_offset, name, ptr);
> -	if (!dent || IS_ERR(dent))
> +	if (IS_ERR_OR_NULL(dent))
>  		return dent;
>  	return do_init_dirent(inode, dent, name, bh,
>  			      (unsigned)(ptr - (void *)dent));
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 15c605cfcfc8..f6281470a182 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -684,7 +684,7 @@ static void delete_work_func(struct work_struct *work)
>  		goto out;
>  
>  	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
> -	if (inode && !IS_ERR(inode)) {
> +	if (!IS_ERR_OR_NULL(inode)) {
>  		d_prune_aliases(inode);
>  		iput(inode);
>  	}
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 998051c4aea7..1cc99da705fc 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -796,7 +796,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  fail_gunlock:
>  	gfs2_dir_no_add(&da);
>  	gfs2_glock_dq_uninit(ghs);
> -	if (inode && !IS_ERR(inode)) {
> +	if (!IS_ERR_OR_NULL(inode)) {
>  		clear_nlink(inode);
>  		if (!free_vfs_inode)
>  			mark_inode_dirty(inode);
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index f5312f3b58ee..d35772d90b0a 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -579,7 +579,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
>  
>  		INIT_WORK(&jd->jd_work, gfs2_recover_func);
>  		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
> -		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
> +		if (IS_ERR_OR_NULL(jd->jd_inode)) {
>  			if (!jd->jd_inode)
>  				error = -ENOENT;
>  			else
> 

[PATCH v2] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

Use IS_ERR_OR_NULL where appropriate.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/dir.c        | 4 ++--
 fs/gfs2/glock.c      | 2 +-
 fs/gfs2/inode.c      | 2 +-
 fs/gfs2/ops_fstype.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 88e4f955c518..6f35d19eec25 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -750,7 +750,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
 	struct gfs2_dirent *dent;
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
 				gfs2_dirent_find_offset, name, ptr);
-	if (!dent || IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		return dent;
 	return do_init_dirent(inode, dent, name, bh,
 			      (unsigned)(ptr - (void *)dent));
@@ -854,7 +854,7 @@ static struct gfs2_dirent *gfs2_dirent_search(struct inode *inode,
 		return ERR_PTR(error);
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size, scan, name, NULL);
 got_dent:
-	if (unlikely(dent == NULL || IS_ERR(dent))) {
+	if (IS_ERR_OR_NULL(dent)) {
 		brelse(bh);
 		bh = NULL;
 	}
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f1ebcb42cbf5..44718098cc60 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -681,7 +681,7 @@ static void delete_work_func(struct work_struct *work)
 		goto out;
 
 	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
 	}
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index b296c59832a7..2e2a8a2fb51d 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -793,7 +793,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 fail_gunlock:
 	gfs2_dir_no_add(&da);
 	gfs2_glock_dq_uninit(ghs);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		clear_nlink(inode);
 		if (!free_vfs_inode)
 			mark_inode_dirty(inode);
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 08823bb3b2d0..9683d534e1a1 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -568,7 +568,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
 
 		INIT_WORK(&jd->jd_work, gfs2_recover_func);
 		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
-		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
+		if (IS_ERR_OR_NULL(jd->jd_inode)) {
 			if (!jd->jd_inode)
 				error = -ENOENT;
 			else
-- 
2.20.1

[Cluster-devel] [PATCH v2] fs: gfs2: Use IS_ERR_OR_NULL

[Kefeng Wang]

Use IS_ERR_OR_NULL where appropriate.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/dir.c        | 4 ++--
 fs/gfs2/glock.c      | 2 +-
 fs/gfs2/inode.c      | 2 +-
 fs/gfs2/ops_fstype.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 88e4f955c518..6f35d19eec25 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -750,7 +750,7 @@ static struct gfs2_dirent *gfs2_dirent_split_alloc(struct inode *inode,
 	struct gfs2_dirent *dent;
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size,
 				gfs2_dirent_find_offset, name, ptr);
-	if (!dent || IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		return dent;
 	return do_init_dirent(inode, dent, name, bh,
 			      (unsigned)(ptr - (void *)dent));
@@ -854,7 +854,7 @@ static struct gfs2_dirent *gfs2_dirent_search(struct inode *inode,
 		return ERR_PTR(error);
 	dent = gfs2_dirent_scan(inode, bh->b_data, bh->b_size, scan, name, NULL);
 got_dent:
-	if (unlikely(dent == NULL || IS_ERR(dent))) {
+	if (IS_ERR_OR_NULL(dent)) {
 		brelse(bh);
 		bh = NULL;
 	}
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f1ebcb42cbf5..44718098cc60 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -681,7 +681,7 @@ static void delete_work_func(struct work_struct *work)
 		goto out;
 
 	inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
 	}
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index b296c59832a7..2e2a8a2fb51d 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -793,7 +793,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 fail_gunlock:
 	gfs2_dir_no_add(&da);
 	gfs2_glock_dq_uninit(ghs);
-	if (inode && !IS_ERR(inode)) {
+	if (!IS_ERR_OR_NULL(inode)) {
 		clear_nlink(inode);
 		if (!free_vfs_inode)
 			mark_inode_dirty(inode);
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 08823bb3b2d0..9683d534e1a1 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -568,7 +568,7 @@ static int gfs2_jindex_hold(struct gfs2_sbd *sdp, struct gfs2_holder *ji_gh)
 
 		INIT_WORK(&jd->jd_work, gfs2_recover_func);
 		jd->jd_inode = gfs2_lookupi(sdp->sd_jindex, &name, 1);
-		if (!jd->jd_inode || IS_ERR(jd->jd_inode)) {
+		if (IS_ERR_OR_NULL(jd->jd_inode)) {
 			if (!jd->jd_inode)
 				error = -ENOENT;
 			else
-- 
2.20.1

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Pali Rohár]

On Tuesday 11 June 2019 17:59:13 Dmitry Torokhov wrote:
> Hi Joe,
> 
> On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote:
> > On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote:
> > > On 2019/6/5 22:42, Pali Rohár wrote:
> > > > On Wednesday 05 June 2019 22:24:28 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.
> > > > Hi! I already reviewed this patch and rejected it, see:
> > > > https://patchwork.kernel.org/patch/10817475/
> > > OK, please ignore it.
> > 
> > I think the stated reason of better readability isn't
> > particularly sensible as the object code produced is
> > actually slightly larger.
> > 
> > x86-64 defconfig (gcc 8.3.0)
> > 
> > $ size drivers/input/mouse/alps.o*
> >    text	   data	    bss	    dec	    hex	filename
> >   29416	     56	      0	  29472	   7320	drivers/input/mouse/alps.o.new
> >   29432	     56	      0	  29488	   7330	drivers/input/mouse/alps.o.old
> 
> If gcc produces worse code for double unlikely, you should probably
> report it to gcc folks, no? Or double unlikely turns into likely?

Is measured size of stripped or unstripped binary? Plus with or without
debug symbols? Double unlikely version should have more debug symbols
and therefore also larger size.

But if unstripped version with double unlikely is larger then it is for
sure compiler bug.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Input: alps: Drop unlikely before IS_ERR(_OR_NULL)

[Joe Perches]

On Wed, 2019-06-12 at 09:14 +0200, Pali Rohár wrote:
> On Tuesday 11 June 2019 17:59:13 Dmitry Torokhov wrote:
> > Hi Joe,
> > 
> > On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote:
> > > On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote:
> > > > On 2019/6/5 22:42, Pali Rohár wrote:
> > > > > On Wednesday 05 June 2019 22:24:28 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.
> > > > > Hi! I already reviewed this patch and rejected it, see:
> > > > > https://patchwork.kernel.org/patch/10817475/
> > > > OK, please ignore it.
> > > 
> > > I think the stated reason of better readability isn't
> > > particularly sensible as the object code produced is
> > > actually slightly larger.
> > > 
> > > x86-64 defconfig (gcc 8.3.0)
> > > 
> > > $ size drivers/input/mouse/alps.o*
> > >    text	   data	    bss	    dec	    hex	filename
> > >   29416	     56	      0	  29472	   7320	drivers/input/mouse/alps.o.new
> > >   29432	     56	      0	  29488	   7330	drivers/input/mouse/alps.o.old
> > 
> > If gcc produces worse code for double unlikely, you should probably
> > report it to gcc folks, no? Or double unlikely turns into likely?
> 
> Is measured size of stripped or unstripped binary? Plus with or without
> debug symbols? Double unlikely version should have more debug symbols
> and therefore also larger size.
> 
> But if unstripped version with double unlikely is larger then it is for
> sure compiler bug.

defconfig so no debug symbols.

It's not necessarily a gcc bug as gcc doesn't
guarantee compiler repeatability.