Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

From: Joe Perches <joe@perches.com>
To: Denis Efremov <efremov@linux.com>, cocci@systeme.lip6.fr
Cc: Michal Marek <michal.lkml@markovi.net>,
	Nicolas Palix <nicolas.palix@imag.fr>,
	linux-kernel@vger.kernel.org
Subject: Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
Date: Sun, 25 Aug 2019 09:37:32 -0700	[thread overview]
Message-ID: <b5bae2981e27d133b61d99b08ee60244bf7aabe3.camel@perches.com> (raw)
In-Reply-To: <20190825130536.14683-1-efremov@linux.com>

On Sun, 2019-08-25 at 16:05 +0300, Denis Efremov wrote:
> This patch adds coccinelle script for detecting !likely and !unlikely
> usage. It's better to use unlikely instead of !likely and vice versa.

Please explain _why_ is it better in the changelog.

btw: there are relatively few uses like this in the kernel.

$ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
40

afaict: It may save 2 bytes of x86/64 object code.

For instance:

$ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
--- kernel/tsacct.lst.old       2019-08-25 09:21:39.936570183 -0700
+++ kernel/tsacct.lst.new       2019-08-25 09:22:20.774324886 -0700
@@ -24,158 +24,153 @@
   15:  48 89 fb                mov    %rdi,%rbx
        u64 time, delta;
 
-       if (!likely(tsk->mm))
+       if (unlikely(tsk->mm))
   18:  4c 8d ab 28 02 00 00    lea    0x228(%rbx),%r13
   1f:  e8 00 00 00 00          callq  24 <__acct_update_integrals+0x24>
                        20: R_X86_64_PLT32      __sanitizer_cov_trace_pc-0x4
   24:  4c 89 ef                mov    %r13,%rdi
   27:  e8 00 00 00 00          callq  2c <__acct_update_integrals+0x2c>
                        28: R_X86_64_PLT32      __asan_load8_noabort-0x4
-  2c:  4c 8b bb 28 02 00 00    mov    0x228(%rbx),%r15
-  33:  4d 85 ff                test   %r15,%r15
-  36:  74 34                   je     6c <__acct_update_integrals+0x6c>
+  2c:  48 83 bb 28 02 00 00    cmpq   $0x0,0x228(%rbx)
+  33:  00 
+  34:  75 34                   jne    6a <__acct_update_integrals+0x6a>
                return;

And here's a possible equivalent checkpatch test.
---
 scripts/checkpatch.pl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 287fe73688f0..364603ad1a47 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6529,6 +6529,24 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
+# !(likely|unlikely)(condition) use should be (unlikely|likely)(condition)
+		if ($perl_version_ok &&
+		    $line =~ /(\!\s*((?:un)?likely))\s*$balanced_parens/) {
+			my $match = $1;
+			my $type =  $2;
+			my $reverse;
+			if ($type eq "likely") {
+				$reverse = "unlikely";
+			} else {
+				$reverse = "likely";
+			}
+			if (WARN("LIKELY_MISUSE",
+				 "Prefer $reverse over $match\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/\Q$match\E\s*\(/$reverse(/;
+			}
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
