[PATCH 01/23] infiniband: nes: add unlikely() to assert()

[PATCH 01/23] infiniband: nes: add unlikely() to assert()

[Igor Stoppa]

Typically the assert is expected to not fail.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Chien Tung <chien.tin.tung@intel.com>
Cc: Roland Dreier <rolandd@cisco.com>
Cc: Faisal Latif <faisal.latif@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
---
 drivers/infiniband/hw/nes/nes.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/nes/nes.h b/drivers/infiniband/hw/nes/nes.h
index bedaa02749fb..d2d0098f38e0 100644
--- a/drivers/infiniband/hw/nes/nes.h
+++ b/drivers/infiniband/hw/nes/nes.h
@@ -151,7 +151,7 @@ do { \
 
 #define assert(expr) \
 do { \
-	if (!(expr)) { \
+	if (unlikely(!(expr))) { \
 		printk(KERN_ERR PFX "Assertion failed! %s, %s, %s, line %d\n", \
 			   #expr, __FILE__, __func__, __LINE__); \
 	} \
-- 
2.17.1

Re: [PATCH 01/23] infiniband: nes: add unlikely() to assert()

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On Fri, 2018-08-31 at 01:34 +0300, Igor Stoppa wrote:
> Typically the assert is expected to not fail.
> 
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Chien Tung <chien.tin.tung@intel.com>
> Cc: Roland Dreier <rolandd@cisco.com>
> Cc: Faisal Latif <faisal.latif@intel.com>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>

Acked-by: Doug Ledford <dledford@redhat.com>

> ---
>  drivers/infiniband/hw/nes/nes.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/nes/nes.h b/drivers/infiniband/hw/nes/nes.h
> index bedaa02749fb..d2d0098f38e0 100644
> --- a/drivers/infiniband/hw/nes/nes.h
> +++ b/drivers/infiniband/hw/nes/nes.h
> @@ -151,7 +151,7 @@ do { \
>  
>  #define assert(expr) \
>  do { \
> -	if (!(expr)) { \
> +	if (unlikely(!(expr))) { \
>  		printk(KERN_ERR PFX "Assertion failed! %s, %s, %s, line %d\n", \
>  			   #expr, __FILE__, __func__, __LINE__); \
>  	} \

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 02/23] ethernet: hnae: add unlikely() to assert()

[Igor Stoppa]

The assert() condition is likely to be true.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: huangdaode <huangdaode@hisilicon.com>
Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
Cc: Salil Mehta <salil.mehta@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns/hnae.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h b/drivers/net/ethernet/hisilicon/hns/hnae.h
index cad52bd331f7..431a3cf8cd2f 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.h
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.h
@@ -47,7 +47,7 @@
 #ifndef assert
 #define assert(expr) \
 do { \
-	if (!(expr)) { \
+	if (unlikely(!(expr))) { \
 		pr_err("Assertion failed! %s, %s, %s, line %d\n", \
 			   #expr, __FILE__, __func__, __LINE__); \
 	} \
-- 
2.17.1

[PATCH 03/23] asm-generic: bug: add unlikely() to BUG_ON()

[Igor Stoppa]

Add a hint to the compiler.
If BUG_ON() is used instead of BUG(), it means that probably the
preferred outcome is to not BUG().

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 20561a60db9c..34c48444c942 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -183,7 +183,7 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) BUG(); } while (0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
2.17.1

[PATCH 04/23] mips: bug: add unlikely() to BUG_ON()

[Igor Stoppa]

Add a hint to the compiler that probably it won't be necessary to BUG()

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: David Daney <ddaney@caviumnetworks.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@mips.com>
Cc: James Hogan <jhogan@kernel.org>
---
 arch/mips/include/asm/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
index 745dc160a069..02101b54aec2 100644
--- a/arch/mips/include/asm/bug.h
+++ b/arch/mips/include/asm/bug.h
@@ -31,7 +31,7 @@ static inline void  __BUG_ON(unsigned long condition)
 			     : : "r" (condition), "i" (BRK_BUG));
 }
 
-#define BUG_ON(C) __BUG_ON((unsigned long)(C))
+#define BUG_ON(C) __BUG_ON(unlikely((unsigned long)(C)))
 
 #define HAVE_ARCH_BUG_ON
 
-- 
2.17.1

[PATCH 05/23] selftest: vm: add unlikely() to BUG_ON()

[Igor Stoppa]

BUG_ON() is unlikely() to BUG()

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Dmitry Safonov <dima@arista.com>
Cc: Shuah Khan <shuah@kernel.org>
---
 tools/testing/selftests/vm/map_populate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vm/map_populate.c b/tools/testing/selftests/vm/map_populate.c
index 6b8aeaa0bf7a..ca3f54765897 100644
--- a/tools/testing/selftests/vm/map_populate.c
+++ b/tools/testing/selftests/vm/map_populate.c
@@ -23,7 +23,7 @@
 
 #define BUG_ON(condition, description)					\
 	do {								\
-		if (condition) {					\
+		if (unlikely(condition)) {				\
 			fprintf(stderr, "[FAIL]\t%s:%d\t%s:%s\n", __func__, \
 				__LINE__, (description), strerror(errno)); \
 			exit(1);					\
-- 
2.17.1

Re: [PATCH 05/23] selftest: vm: add unlikely() to BUG_ON()

[Dmitry Safonov]

Hi Igor,

On Fri, 2018-08-31 at 01:34 +0300, Igor Stoppa wrote:
> BUG_ON() is unlikely() to BUG()

This selftest runs in userspace..
So, we should define the macro somehow, as i.e:
rseq/rseq.h:#define rseq_unlikely(x)    __builtin_expect(!!(x), 0)

Otherwise,
[selftests]$ make vm/map_populate
cc     vm/map_populate.c   -o vm/map_populate
vm/map_populate.c: In function ‘parent_f’:
vm/map_populate.c:26:7: warning: implicit declaration of function
‘unlikely’; did you mean ‘unlinkat’? [-Wimplicit-function-declaration]
   if (unlikely(condition)) {     \
       ^
vm/map_populate.c:38:2: note: in expansion of macro ‘BUG_ON’
  BUG_ON(ret <= 0, "read(sock)");
  ^~~~~~
/tmp/cc7evGVG.o: In function `parent_f':
map_populate.c:(.text+0x3d): undefined reference to `unlikely'
map_populate.c:(.text+0xbb): undefined reference to `unlikely'
map_populate.c:(.text+0x135): undefined reference to `unlikely'
map_populate.c:(.text+0x1b0): undefined reference to `unlikely'

Not sure if we care for this at all for userspace test.
I don't mind as it runs each time by kbuild robot and cumulatively may
save something. But it's better be at least compile-tested.

-- 
            Dima

Re: [PATCH 05/23] selftest: vm: add unlikely() to BUG_ON()

[Dmitry Safonov]

On Thu, 2018-08-30 at 23:57 +0100, Dmitry Safonov wrote:
> Hi Igor,
> 
> On Fri, 2018-08-31 at 01:34 +0300, Igor Stoppa wrote:
> > BUG_ON() is unlikely() to BUG()
> 
> This selftest runs in userspace..
> So, we should define the macro somehow, as i.e:
> rseq/rseq.h:#define rseq_unlikely(x)    __builtin_expect(!!(x), 0)
> 
> Otherwise,
> [selftests]$ make vm/map_populate
> cc     vm/map_populate.c   -o vm/map_populate
> vm/map_populate.c: In function ‘parent_f’:
> vm/map_populate.c:26:7: warning: implicit declaration of function
> ‘unlikely’; did you mean ‘unlinkat’? [-Wimplicit-function-
> declaration]
>    if (unlikely(condition)) {     \
>        ^
> vm/map_populate.c:38:2: note: in expansion of macro ‘BUG_ON’
>   BUG_ON(ret <= 0, "read(sock)");
>   ^~~~~~
> /tmp/cc7evGVG.o: In function `parent_f':
> map_populate.c:(.text+0x3d): undefined reference to `unlikely'
> map_populate.c:(.text+0xbb): undefined reference to `unlikely'
> map_populate.c:(.text+0x135): undefined reference to `unlikely'
> map_populate.c:(.text+0x1b0): undefined reference to `unlikely'
> 
> Not sure if we care for this at all for userspace test.
> I don't mind as it runs each time by kbuild robot and cumulatively
> may
> save something. But it's better be at least compile-tested.

JFI, there are a plenty of definitions for unlikely():
[linux]$ git grep 'define unlikely' tools
+rseq_unlikely() from tools/testing/selftests/rseq/rseq.h

Probably, the one from "tools/include/linux/compiler.h" can be used in
tools directory.

-- 
            Dima

Re: [PATCH 05/23] selftest: vm: add unlikely() to BUG_ON()

[Igor Stoppa]

Hi Dmitry,

On 31/08/18 02:04, Dmitry Safonov wrote:

> Probably, the one from "tools/include/linux/compiler.h" can be used in
> tools directory.

Thank you for the advice. It seems to work now.

--
igor

[PATCH 06/23] virtio: add unlikely() to WARN_ON_ONCE()

[Igor Stoppa]

The condition to test is unlikely() to be true. Add the hint.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 tools/virtio/linux/kernel.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h
index fb22bccfbc8a..2b5e417ea2f0 100644
--- a/tools/virtio/linux/kernel.h
+++ b/tools/virtio/linux/kernel.h
@@ -123,7 +123,7 @@ static inline void free_page(unsigned long addr)
 #define dev_err(dev, format, ...) fprintf (stderr, format, ## __VA_ARGS__)
 #define dev_warn(dev, format, ...) fprintf (stderr, format, ## __VA_ARGS__)
 
-#define WARN_ON_ONCE(cond) ((cond) ? fprintf (stderr, "WARNING\n") : 0)
+#define WARN_ON_ONCE(cond) (unlikely(cond) ? fprintf (stderr, "WARNING\n") : 0)
 
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\
-- 
2.17.1

[PATCH 07/23] seccomp: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to wrap it
into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/seccomp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index fd023ac24e10..5a2a9af4663e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -195,7 +195,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
 			READ_ONCE(current->seccomp.filter);
 
 	/* Ensure unexpected behavior doesn't result in failing open. */
-	if (unlikely(WARN_ON(f == NULL)))
+	if (WARN_ON(f == NULL))
 		return SECCOMP_RET_KILL_PROCESS;
 
 	if (!sd) {
@@ -297,7 +297,7 @@ static inline pid_t seccomp_can_sync_threads(void)
 		/* Return the first thread that cannot be synchronized. */
 		failed = task_pid_vnr(thread);
 		/* If the pid cannot be resolved, then return -ESRCH */
-		if (unlikely(WARN_ON(failed == 0)))
+		if (WARN_ON(failed == 0))
 			failed = -ESRCH;
 		return failed;
 	}
-- 
2.17.1

Re: [PATCH 07/23] seccomp: remove unnecessary unlikely()

[Kees Cook]

On Thu, Aug 30, 2018 at 3:34 PM, Igor Stoppa <igor.stoppa@gmail.com> wrote:
> WARN_ON() already contains an unlikely(), so it's not necessary to wrap it
> into another.
>
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Kees Cook <keescook@chromium.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  kernel/seccomp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index fd023ac24e10..5a2a9af4663e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -195,7 +195,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
>                         READ_ONCE(current->seccomp.filter);
>
>         /* Ensure unexpected behavior doesn't result in failing open. */
> -       if (unlikely(WARN_ON(f == NULL)))
> +       if (WARN_ON(f == NULL))
>                 return SECCOMP_RET_KILL_PROCESS;
>
>         if (!sd) {
> @@ -297,7 +297,7 @@ static inline pid_t seccomp_can_sync_threads(void)
>                 /* Return the first thread that cannot be synchronized. */
>                 failed = task_pid_vnr(thread);
>                 /* If the pid cannot be resolved, then return -ESRCH */
> -               if (unlikely(WARN_ON(failed == 0)))
> +               if (WARN_ON(failed == 0))
>                         failed = -ESRCH;
>                 return failed;
>         }
> --
> 2.17.1
>



-- 
Kees Cook
Pixel Security

[PATCH 08/23] drm: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to wrap it
into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Stephane Viau <sviau@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
 drivers/gpu/drm/msm/disp/mdp_format.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
index f93d5681267c..9e0b3730536d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
@@ -261,7 +261,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
 	u32 blend_cfg;
 	struct mdp5_hw_mixer *mixer = pipeline->mixer;
 
-	if (unlikely(WARN_ON(!mixer))) {
+	if (WARN_ON(!mixer)) {
 		dev_err(ctl_mgr->dev->dev, "CTL %d cannot find LM",
 			ctl->id);
 		return -EINVAL;
@@ -703,7 +703,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
 		goto fail;
 	}
 
-	if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
+	if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
 		dev_err(dev->dev, "Increase static pool size to at least %d\n",
 				ctl_cfg->count);
 		ret = -ENOSPC;
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
index 005760bee708..005f24dcfbdd 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -185,7 +185,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
 
 struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
 {
-	if (unlikely(WARN_ON(type >= CSC_MAX)))
+	if (WARN_ON(type >= CSC_MAX))
 		return NULL;
 
 	return &csc_convert[type];
-- 
2.17.1

[PATCH 09/23] cpufreq: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to wrap it
into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f53fb41efb7b..7aa3dcad2175 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -403,7 +403,7 @@ EXPORT_SYMBOL_GPL(cpufreq_freq_transition_begin);
 void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
 		struct cpufreq_freqs *freqs, int transition_failed)
 {
-	if (unlikely(WARN_ON(!policy->transition_ongoing)))
+	if (WARN_ON(!policy->transition_ongoing))
 		return;
 
 	cpufreq_notify_post_transition(policy, freqs, transition_failed);
-- 
2.17.1

[PATCH 10/23] wireless: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON_ONCE() already contains an unlikely(), and the logical or of two of
them is still unlikely(), so it's not necessary to wrap them into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Christian Lamparter <chunkeey@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/carl9170/tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 0cb5b58925dc..8c75651ede6c 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -246,8 +246,8 @@ static void carl9170_release_dev_space(struct ar9170 *ar, struct sk_buff *skb)
 	 *    of available memory blocks, so the number can
 	 *    never execeed the mem_blocks count.
 	 */
-	if (unlikely(WARN_ON_ONCE(cookie == 0) ||
-	    WARN_ON_ONCE(cookie > ar->fw.mem_blocks)))
+	if (WARN_ON_ONCE(cookie == 0) ||
+	    WARN_ON_ONCE(cookie > ar->fw.mem_blocks))
 		return;
 
 	atomic_add(DIV_ROUND_UP(skb->len, ar->fw.mem_block_size),
-- 
2.17.1

Re: [PATCH 10/23] wireless: remove unnecessary unlikely()

[Kalle Valo]

+ linux-wireless

Igor Stoppa <igor.stoppa@gmail.com> writes:

> WARN_ON_ONCE() already contains an unlikely(), and the logical or of two of
> them is still unlikely(), so it's not necessary to wrap them into another.
>
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>

IMHO you could fold patches 10, 11 and 12 into one to avoid having three
patches with duplicate titles. Or alternatively use proper driver
prefixes like "ath10k:", "b43:" and so on.

But how do you want these to be applied? For the wireless patches you
didn't Cc linux-wireless so our patchwork won't see them and hence I
can't take them. So if you want me to take these, please resend and
include linux-wireless.

-- 
Kalle Valo

Re: [PATCH 10/23] wireless: remove unnecessary unlikely()

[Igor Stoppa]

On 31/08/18 12:52, Kalle Valo wrote:
> + linux-wireless

> IMHO you could fold patches 10, 11 and 12 into one to avoid having three
> patches with duplicate titles. Or alternatively use proper driver
> prefixes like "ath10k:", "b43:" and so on.

I was wondering if it would be ok to fold them, but it's easier to fold 
than to split, so I started with 3

> But how do you want these to be applied? For the wireless patches you
> didn't Cc linux-wireless so our patchwork won't see them and hence I
> can't take them. So if you want me to take these, please resend and
> include linux-wireless.


ok, I was hoping to avoid joining the ml but it's done
I had patches for about 20 different trees, so I decided to first see if 
the patches could be taken in right away.
In at least one case (pinctrl) I got lucky :-)

--
thanks, igor

[PATCH 11/23] wireless: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON_ONCE() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 4d1cd90d6d27..1455007f3eb8 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1176,11 +1176,11 @@ static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar,
 	 */
 
 	/* This probably shouldn't happen but warn just in case */
-	if (unlikely(WARN_ON_ONCE(!is_first)))
+	if (WARN_ON_ONCE(!is_first))
 		return;
 
 	/* This probably shouldn't happen but warn just in case */
-	if (unlikely(WARN_ON_ONCE(!(is_first && is_last))))
+	if (WARN_ON_ONCE(!(is_first && is_last)))
 		return;
 
 	skb_trim(msdu, msdu->len - FCS_LEN);
-- 
2.17.1

[PATCH 12/23] wireless: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/broadcom/b43/dma.c       | 2 +-
 drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/b43/dma.c b/drivers/net/wireless/broadcom/b43/dma.c
index 6b0e1ec346cb..d113bd997f4b 100644
--- a/drivers/net/wireless/broadcom/b43/dma.c
+++ b/drivers/net/wireless/broadcom/b43/dma.c
@@ -1432,7 +1432,7 @@ int b43_dma_tx(struct b43_wldev *dev, struct sk_buff *skb)
 		goto out;
 	}
 
-	if (unlikely(WARN_ON(free_slots(ring) < TX_SLOTS_PER_FRAME))) {
+	if (WARN_ON(free_slots(ring) < TX_SLOTS_PER_FRAME)) {
 		/* If we get here, we have a real error with the queue
 		 * full, but queues not stopped. */
 		b43err(dev->wl, "DMA queue overflow\n");
diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
index 2f0c64cef65f..1b1da7d83652 100644
--- a/drivers/net/wireless/broadcom/b43legacy/dma.c
+++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
@@ -1149,7 +1149,7 @@ int b43legacy_dma_tx(struct b43legacy_wldev *dev,
 		return -ENOSPC;
 	}
 
-	if (unlikely(WARN_ON(free_slots(ring) < SLOTS_PER_PACKET))) {
+	if (WARN_ON(free_slots(ring) < SLOTS_PER_PACKET)) {
 		/* If we get here, we have a real error with the queue
 		 * full, but queues not stopped. */
 		b43legacyerr(dev->wl, "DMA queue overflow\n");
-- 
2.17.1

[PATCH 13/23] freescale: ethernet: remove unnecessary unlikely()

[Igor Stoppa]

Both WARN_ON() and WARN_ONCE() already contain an unlikely(), so it's not
necessary to wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Madalin Bucur <madalin.bucur@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 65a22cd9aef2..783134f1b779 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -1280,7 +1280,7 @@ static int dpaa_bman_release(const struct dpaa_bp *dpaa_bp,
 
 	err = bman_release(dpaa_bp->pool, bmb, cnt);
 	/* Should never occur, address anyway to avoid leaking the buffers */
-	if (unlikely(WARN_ON(err)) && dpaa_bp->free_buf_cb)
+	if (WARN_ON(err) && dpaa_bp->free_buf_cb)
 		while (cnt-- > 0)
 			dpaa_bp->free_buf_cb(dpaa_bp, &bmb[cnt]);
 
@@ -1704,10 +1704,8 @@ static struct sk_buff *contig_fd_to_skb(const struct dpaa_priv *priv,
 
 	skb = build_skb(vaddr, dpaa_bp->size +
 			SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
-	if (unlikely(!skb)) {
-		WARN_ONCE(1, "Build skb failure on Rx\n");
+	if (WARN_ONCE(!skb, "Build skb failure on Rx\n"))
 		goto free_buffer;
-	}
 	WARN_ON(fd_off != priv->rx_headroom);
 	skb_reserve(skb, fd_off);
 	skb_put(skb, qm_fd_get_length(fd));
@@ -1770,7 +1768,7 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,
 			sz = dpaa_bp->size +
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 			skb = build_skb(sg_vaddr, sz);
-			if (WARN_ON(unlikely(!skb)))
+			if (WARN_ON(!skb))
 				goto free_buffers;
 
 			skb->ip_summed = rx_csum_offload(priv, fd);
-- 
2.17.1

[PATCH 14/23] wimax: i2400m: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
---
 drivers/net/wimax/i2400m/tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wimax/i2400m/tx.c b/drivers/net/wimax/i2400m/tx.c
index f20886ade1cc..59c70d928249 100644
--- a/drivers/net/wimax/i2400m/tx.c
+++ b/drivers/net/wimax/i2400m/tx.c
@@ -655,8 +655,7 @@ void i2400m_tx_close(struct i2400m *i2400m)
 	padding = aligned_size - tx_msg_moved->size;
 	if (padding > 0) {
 		pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
-		if (unlikely(WARN_ON(pad_buf == NULL
-				     || pad_buf == TAIL_FULL))) {
+		if (WARN_ON(!pad_buf || pad_buf == TAIL_FULL)) {
 			/* This should not happen -- append should verify
 			 * there is always space left at least to append
 			 * tx_block_size */
-- 
2.17.1

[PATCH 15/23] lvm: device mapper: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
---
 drivers/md/dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 20f7e4ef5342..0f0f8547106d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1653,7 +1653,7 @@ static blk_qc_t __process_bio(struct mapped_device *md,
 		 * Defend against IO still getting in during teardown
 		 * - as was seen for a time with nvme-fcloop
 		 */
-		if (unlikely(WARN_ON_ONCE(!ti || !dm_target_is_valid(ti)))) {
+		if (WARN_ON_ONCE(!ti || !dm_target_is_valid(ti))) {
 			error = -EIO;
 			goto out;
 		}
-- 
2.17.1

[PATCH 16/23] lvm: device mapper: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Joe Thornber <ejt@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
---
 drivers/md/dm-cache-policy-smq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-cache-policy-smq.c b/drivers/md/dm-cache-policy-smq.c
index 1b5b9ad9e492..b61aac00ff40 100644
--- a/drivers/md/dm-cache-policy-smq.c
+++ b/drivers/md/dm-cache-policy-smq.c
@@ -1200,7 +1200,7 @@ static void queue_demotion(struct smq_policy *mq)
 	struct policy_work work;
 	struct entry *e;
 
-	if (unlikely(WARN_ON_ONCE(!mq->migrations_allowed)))
+	if (WARN_ON_ONCE(!mq->migrations_allowed))
 		return;
 
 	e = q_peek(&mq->clean, mq->clean.nr_levels / 2, true);
-- 
2.17.1

[PATCH 17/23] usb: octeon-hcd: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cff5e790b196..1a6431879201 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2768,7 +2768,7 @@ static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 	    (pipe->transfer_dir == CVMX_USB_DIRECTION_OUT))
 		pipe->flags |= CVMX_USB_PIPE_FLAGS_NEED_PING;
 
-	if (unlikely(WARN_ON_ONCE(bytes_this_transfer < 0))) {
+	if (WARN_ON_ONCE(bytes_this_transfer < 0)) {
 		/*
 		 * In some rare cases the DMA engine seems to get stuck and
 		 * keeps substracting same byte count over and over again. In
-- 
2.17.1

[PATCH 18/23] infiniband: scsi: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index f37cbad022a2..447d21ea479a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2708,7 +2708,7 @@ static void srpt_queue_response(struct se_cmd *cmd)
 		break;
 	}
 
-	if (unlikely(WARN_ON_ONCE(state == SRPT_STATE_CMD_RSP_SENT)))
+	if (WARN_ON_ONCE(state == SRPT_STATE_CMD_RSP_SENT))
 		return;
 
 	/* For read commands, transfer the data to the initiator. */
-- 
2.17.1

Re: [PATCH 18/23] infiniband: scsi: remove unnecessary unlikely()

[Bart Van Assche]

On Thu, Aug 30, 2018 at 3:35 PM Igor Stoppa <igor.stoppa@gmail.com> wrote:
>
> WARN_ON() already contains an unlikely(), so it's not necessary to
> wrap it into another.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 19/23] pinctrl: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index aefe3c33dffd..eb87ab774269 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -715,7 +715,7 @@ int aspeed_pin_config_set(struct pinctrl_dev *pctldev, unsigned int offset,
 
 		pmap = find_pinconf_map(param, MAP_TYPE_ARG, arg);
 
-		if (unlikely(WARN_ON(!pmap)))
+		if (WARN_ON(!pmap))
 			return -EINVAL;
 
 		val = pmap->val << pconf->bit;
-- 
2.17.1

Re: [PATCH 19/23] pinctrl: remove unnecessary unlikely()

[Linus Walleij]

On Fri, Aug 31, 2018 at 12:35 AM Igor Stoppa <igor.stoppa@gmail.com> wrote:

> WARN_ON() already contains an unlikely(), so it's not necessary to
> wrap it into another.
>
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Patch applied as obviously correct.

Yours,
Linus Walleij

[PATCH 20/23] mm: percpu: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: zijun_hu <zijun_hu@htc.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Dennis Zhou <dennisszhou@gmail.com>
---
 mm/percpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index a749d4d96e3e..f5c2796fe63e 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2588,7 +2588,7 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 	BUG_ON(ai->nr_groups != 1);
 	upa = ai->alloc_size/ai->unit_size;
 	nr_g0_units = roundup(num_possible_cpus(), upa);
-	if (unlikely(WARN_ON(ai->groups[0].nr_units != nr_g0_units))) {
+	if (WARN_ON(ai->groups[0].nr_units != nr_g0_units)) {
 		pcpu_free_alloc_info(ai);
 		return -EINVAL;
 	}
-- 
2.17.1

Re: [PATCH 20/23] mm: percpu: remove unnecessary unlikely()

[Dennis Zhou]

On Fri, Aug 31, 2018 at 01:34:26AM +0300, Igor Stoppa wrote:
> WARN_ON() already contains an unlikely(), so it's not necessary to
> wrap it into another.
> 
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> Cc: zijun_hu <zijun_hu@htc.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Dennis Zhou <dennisszhou@gmail.com>
> ---
>  mm/percpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index a749d4d96e3e..f5c2796fe63e 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -2588,7 +2588,7 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
>  	BUG_ON(ai->nr_groups != 1);
>  	upa = ai->alloc_size/ai->unit_size;
>  	nr_g0_units = roundup(num_possible_cpus(), upa);
> -	if (unlikely(WARN_ON(ai->groups[0].nr_units != nr_g0_units))) {
> +	if (WARN_ON(ai->groups[0].nr_units != nr_g0_units)) {
>  		pcpu_free_alloc_info(ai);
>  		return -EINVAL;
>  	}
> -- 
> 2.17.1
> 

Acked-by: Dennis Zhou <dennisszhou@gmail.com>

Thanks,
Dennis

[PATCH 21/23] filesystems: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
---
 fs/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..19a9e4b378d3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -750,7 +750,7 @@ static int do_dentry_open(struct file *f,
 		f->f_mode |= FMODE_ATOMIC_POS;
 
 	f->f_op = fops_get(inode->i_fop);
-	if (unlikely(WARN_ON(!f->f_op))) {
+	if (WARN_ON(!f->f_op)) {
 		error = -ENODEV;
 		goto cleanup_all;
 	}
-- 
2.17.1

[PATCH 22/23] powerpc: remove unnecessary unlikely()

[Igor Stoppa]

WARN_ON() already contains an unlikely(), so it's not necessary to
wrap it into another.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: Arseny Solokha <asolokha@kb.kras.ru>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/mm/tlb_nohash.c      | 2 +-
 arch/powerpc/sysdev/xive/common.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c
index 15fe5f0c8665..46f5d8c972fb 100644
--- a/arch/powerpc/mm/tlb_nohash.c
+++ b/arch/powerpc/mm/tlb_nohash.c
@@ -302,7 +302,7 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
 	 * This function as well as __local_flush_tlb_page() must only be called
 	 * for user contexts.
 	 */
-	if (unlikely(WARN_ON(!mm)))
+	if (WARN_ON(!mm))
 		return;
 
 	preempt_disable();
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 959a2a62f233..7dde46b38979 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -442,7 +442,7 @@ static void xive_dec_target_count(int cpu)
 	struct xive_cpu *xc = per_cpu(xive_cpu, cpu);
 	struct xive_q *q = &xc->queue[xive_irq_priority];
 
-	if (unlikely(WARN_ON(cpu < 0 || !xc))) {
+	if (WARN_ON(cpu < 0 || !xc)) {
 		pr_err("%s: cpu=%d xc=%p\n", __func__, cpu, xc);
 		return;
 	}
-- 
2.17.1

[PATCH 23/23] scsi: remove unnecessary unlikely()

[Igor Stoppa]

BUG_ON() already contains an unlikely(), there is no need for another one.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0adfb3bce0fd..aaa1819b0a69 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1207,8 +1207,8 @@ int scsi_init_io(struct scsi_cmnd *cmd)
 
 		count = blk_rq_map_integrity_sg(rq->q, rq->bio,
 						prot_sdb->table.sgl);
-		BUG_ON(unlikely(count > ivecs));
-		BUG_ON(unlikely(count > queue_max_integrity_segments(rq->q)));
+		BUG_ON(count > ivecs);
+		BUG_ON(count > queue_max_integrity_segments(rq->q));
 
 		cmd->prot_sdb = prot_sdb;
 		cmd->prot_sdb->table.nents = count;
-- 
2.17.1

Re: [PATCH 00/23] Make use of unlikely() more consistently.

[Arnd Bergmann]

On Fri, Aug 31, 2018 at 12:35 AM Igor Stoppa <igor.stoppa@gmail.com> wrote:
>
> In some cases, checks that are expected to not fail, do not take advantage
> of specifying that the condition being tested is unlikely().
>
> Ex:
>
> #define assert(condition)
> ...
>         if (!(condition))
>                 error_action()
> ...
>
> should be
>
> #define assert(condition)
> ...
>         if (unlikely(!(condition)))
>                 error_action()
> ...

There is a potential that this introduces false-postive -Wmaybe-uninitialized
warnings when CONFIG_PROFILE_ANNOTATED_BRANCHES is
set, since that turns unlikely() into a complex operation that in turn
confuses the compiler so it no longer keeps track of which variables
are initialized or not.

It's possible that none of your patches do that, but one needs to be aware
of the problem, and possibly revert some of your patches if it does cause
warning regressions in drivers that don't actually benefit from the
micro-optimization.

> In other cases, the hint is given twice: once explicitly and once inside
> whatever test is being performed: assert(), BUG_ON(), WARN_ON(), etc.
>
> Ex:
>
> if (unlikely(WARN_ON(condition, message)))
> ...

Removing those is definitely fine.

        Arnd

Re: [PATCH 00/23] Make use of unlikely() more consistently.

[Igor Stoppa]

On 31/08/18 17:09, Arnd Bergmann wrote:

[...]

>> #define assert(condition)
>> ...
>>          if (unlikely(!(condition)))
>>                  error_action()
>> ...
> 
> There is a potential that this introduces false-postive -Wmaybe-uninitialized
> warnings when CONFIG_PROFILE_ANNOTATED_BRANCHES is
> set, since that turns unlikely() into a complex operation that in turn
> confuses the compiler so it no longer keeps track of which variables
> are initialized or not.
> 
> It's possible that none of your patches do that, but one needs to be aware
> of the problem, and possibly revert some of your patches if it does cause
> warning regressions in drivers that don't actually benefit from the
> micro-optimization.

I see.
But if such case might arise, it might be possible, instead of reverting 
these patches, to locally - and conditionally - turn unlikely() into an 
identity macro.

For example:

#ifdef CONFIG_PROFILE_ANNOTATOED_BRANCHES
#define unlikely(x) (x)
#endif

Without giving up the hint for the case of normal compilation.

--
igor