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/[email protected]/
Signed-off-by: Denis Efremov <[email protected]>
---
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
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/[email protected]/
[]
> 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)) {
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/[email protected]/
> []
>> 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
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/[email protected]/
Signed-off-by: Denis Efremov <[email protected]>
---
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
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