[PATCH] checkpatch: check for nested (un)?likely calls

[PATCH] checkpatch: check for nested (un)?likely calls

[Denis Efremov]

IS_ERR, IS_ERR_OR_NULL, IS_ERR_VALUE already contain unlikely optimization
internally. Thus, there is no point in calling these functions under
likely/unlikely.

This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..81dace5ceea5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,13 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
+# nested likely/unlikely calls
+		if ($perl_version_ok &&
+		    $line =~ /\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)\s*${balanced_parens}\s*\)/) {
+			WARN("LIKELY_MISUSE",
+			     "nested (un)?likely calls, unlikely already used in $1 internally\n" . $herecurr);
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {
-- 
2.21.0

Re: [PATCH] checkpatch: check for nested (un)?likely calls

[Joe Perches]

On Tue, 2019-08-27 at 19:55 +0300, Denis Efremov wrote:
> IS_ERR, IS_ERR_OR_NULL, IS_ERR_VALUE already contain unlikely optimization
> internally. Thus, there is no point in calling these functions under
> likely/unlikely.
> 
> This check is based on the coccinelle rule developed by Enrico Weigelt
> https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6480,6 +6480,13 @@ sub process {
>  			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
>  		}
>  
> +# nested likely/unlikely calls
> +		if ($perl_version_ok &&
> +		    $line =~ /\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)\s*${balanced_parens}\s*\)/) {
> +			WARN("LIKELY_MISUSE",
> +			     "nested (un)?likely calls, unlikely already used in $1 internally\n" . $herecurr);
> +		}
> +

Couple things:

1:

Are you going to submit patches for the just 10 instances that exist?

$ git grep -P -n '\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)\s*\([^\)]+\)\s*\)'
arch/x86/kernel/irq.c:246:      if (likely(!IS_ERR_OR_NULL(desc))) {
drivers/infiniband/hw/hfi1/verbs.c:1044:        if (unlikely(IS_ERR_OR_NULL(pbuf))) {
drivers/input/mouse/alps.c:1479:        } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
fs/ntfs/mft.c:74:       if (likely(!IS_ERR(page))) {
fs/ntfs/mft.c:157:      if (likely(!IS_ERR(m)))
fs/ntfs/mft.c:274:              if (likely(!IS_ERR(m))) {
fs/ntfs/mft.c:1779:             if (likely(!IS_ERR(rl2)))
fs/ntfs/namei.c:118:            if (likely(!IS_ERR(dent_inode))) {
fs/ntfs/runlist.c:954:  if (likely(!IS_ERR(old_rl)))
include/net/udp.h:483:  if (unlikely(IS_ERR_OR_NULL(segs))) {

2:

This will not warn about code like:

fs/ntfs/mft.c:  if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {

that could probably be better written as:

		if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {

Re: [PATCH] checkpatch: check for nested (un)?likely calls

[Denis Efremov]

On 8/27/19 8:21 PM, Joe Perches wrote:
> On Tue, 2019-08-27 at 19:55 +0300, Denis Efremov wrote:
>> IS_ERR, IS_ERR_OR_NULL, IS_ERR_VALUE already contain unlikely optimization
>> internally. Thus, there is no point in calling these functions under
>> likely/unlikely.
>>
>> This check is based on the coccinelle rule developed by Enrico Weigelt
>> https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -6480,6 +6480,13 @@ sub process {
>>  			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
>>  		}
>>  
>> +# nested likely/unlikely calls
>> +		if ($perl_version_ok &&
>> +		    $line =~ /\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)\s*${balanced_parens}\s*\)/) {
>> +			WARN("LIKELY_MISUSE",
>> +			     "nested (un)?likely calls, unlikely already used in $1 internally\n" . $herecurr);
>> +		}
>> +
> 
> Couple things:
> 
> 1:
> 
> Are you going to submit patches for the just 10 instances that exist?
> 
> $ git grep -P -n '\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)\s*\([^\)]+\)\s*\)'
> arch/x86/kernel/irq.c:246:      if (likely(!IS_ERR_OR_NULL(desc))) {
> drivers/infiniband/hw/hfi1/verbs.c:1044:        if (unlikely(IS_ERR_OR_NULL(pbuf))) {
> drivers/input/mouse/alps.c:1479:        } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> fs/ntfs/mft.c:74:       if (likely(!IS_ERR(page))) {
> fs/ntfs/mft.c:157:      if (likely(!IS_ERR(m)))
> fs/ntfs/mft.c:274:              if (likely(!IS_ERR(m))) {
> fs/ntfs/mft.c:1779:             if (likely(!IS_ERR(rl2)))
> fs/ntfs/namei.c:118:            if (likely(!IS_ERR(dent_inode))) {
> fs/ntfs/runlist.c:954:  if (likely(!IS_ERR(old_rl)))
> include/net/udp.h:483:  if (unlikely(IS_ERR_OR_NULL(segs))) {

Yes, I will do it in days.

> 
> 2:
> 
> This will not warn about code like:
> 
> fs/ntfs/mft.c:  if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {
> 
> that could probably be better written as:
> 
> 		if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {
> 
> 

Ok, I will change the regex in v2 to:
$line =~ /\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)/

Should I skip $perl_version_ok check then?

If there are no other suggestions about, for example, warn message or commit
description I will send v2.

Thanks,
Denis

[PATCH v2] checkpatch: check for nested unlikely calls

[Denis Efremov]

IS_ERR, IS_ERR_OR_NULL, IS_ERR_VALUE already contain unlikely optimization
internally. Thus, there is no point in calling these functions under
likely/unlikely.

This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..55b90e1334d2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,12 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
+# nested likely/unlikely calls
+		if ($line =~ /\b(?:(?:un)?likely)\s*\(!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?)/) {
+			WARN("LIKELY_MISUSE",
+			     "nested (un)?likely calls, unlikely already used in $1 internally\n" . $herecurr);
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {
-- 
2.21.0

Re: [PATCH v2] checkpatch: check for nested unlikely calls

[Denis Efremov]

On 8/28/19 4:32 PM, Denis Efremov wrote:
> IS_ERR, IS_ERR_OR_NULL, IS_ERR_VALUE already contain unlikely optimization
> internally. Thus, there is no point in calling these functions under
> likely/unlikely.

It looks like this rule could be extended with this list:
CHECK_DATA_CORRUPTION
GLOCK_BUG_ON
SPIN_BUG_ON
RWLOCK_BUG_ON
DCCP_BUG_ON
GEM_BUG_ON
BUG_ON
WARN
WARN_TAINT
WARN_ON_ONCE
WARN_ONCE
WARN_TAINT_ONCE
WARN_ON_SMP

However, grep shows that maybe only BUG_ON, WARN_ON, WARN, WARN_ON_ONCE worth checking:
git grep 'likely(\s*\(CHECK_DATA_CORRUPTION\|GLOCK_BUG_ON\|SPIN_BUG_ON\|RWLOCK_BUG_ON\|DCCP_BUG_ON\|GEM_BUG_ON\|BUG_ON\|WARN\|WARN_TAINT\|WARN_ON_ONCE\|WARN_ONCE\|WARN_TAINT_ONCE\|WARN_ON_SMP\)' .
drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c:       if (unlikely(WARN_ON(!mixer))) {
drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c:       if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
drivers/gpu/drm/msm/disp/mdp_format.c:  if (unlikely(WARN_ON(type >= CSC_MAX)))
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c:     if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
drivers/net/wimax/i2400m/tx.c:          if (unlikely(WARN_ON(pad_buf == NULL
drivers/xen/events/events_base.c:       if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
fs/open.c:      if (unlikely(WARN_ON(!f->f_op))) {
fs/xfs/xfs_buf.c:       if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
fs/xfs/xfs_buf.c:       if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))

> +# nested likely/unlikely calls
		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*((?:IS_ERR(?:_OR_NULL|_VALUE)?)|(?:BUG_ON|WARN(?:_ON(?:_ONCE)?)?)))/) {
or maybe even to match all possible WARNs:
		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*((?:IS_ERR(?:_OR_NULL|_VALUE)?)|(?:BUG_ON|WARN)))/) {
> +			WARN("LIKELY_MISUSE",
> +			     "nested (un)?likely calls, unlikely already used in $1 internally\n" . $herecurr);
> +		}

Any suggestions for v3?


Thanks,
Denis