Coccinelle archive on lore.kernel.org
 help / color / Atom feed
* [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
@ 2019-08-25 13:05 Denis Efremov
  2019-08-25 15:30 ` Markus Elfring
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-25 13:05 UTC (permalink / raw)
  To: cocci; +Cc: Michal Marek, Nicolas Palix, linux-kernel

This patch adds coccinelle script for detecting !likely and !unlikely
usage. It's better to use unlikely instead of !likely and vice versa.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/misc/unlikely.cocci | 70 ++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 scripts/coccinelle/misc/unlikely.cocci

diff --git a/scripts/coccinelle/misc/unlikely.cocci b/scripts/coccinelle/misc/unlikely.cocci
new file mode 100644
index 000000000000..88278295d76a
--- /dev/null
+++ b/scripts/coccinelle/misc/unlikely.cocci
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Use unlikely instead of !likely and likely instead of !unlikely.
+///
+// Confidence: High
+// Copyright: (C) 2019 Denis Efremov, ISPRAS.
+// Options: --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+//  For context mode
+//----------------------------------------------------------
+
+@depends on context disable unlikely@
+expression E;
+@@
+
+(
+* !likely(E)
+|
+* !unlikely(E)
+)
+
+//----------------------------------------------------------
+//  For patch mode
+//----------------------------------------------------------
+
+@depends on patch disable unlikely@
+expression E;
+@@
+
+(
+-!likely(E)
++unlikely(E)
+|
+-!unlikely(E)
++likely(E)
+)
+
+//----------------------------------------------------------
+//  For org and report mode
+//----------------------------------------------------------
+
+@r depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+(
+ !likely@p(E)
+|
+ !unlikely@p(E)
+)
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg="WARNING: Use unlikely instead of !likely"
+coccilib.report.print_report(p[0], msg)
+
-- 
2.21.0

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 13:05 [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage Denis Efremov
@ 2019-08-25 15:30 ` Markus Elfring
  2019-08-25 21:06   ` Denis Efremov
  2019-08-25 15:30 ` Markus Elfring
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 25+ messages in thread
From: Markus Elfring @ 2019-08-25 15:30 UTC (permalink / raw)
  To: Denis Efremov, Gilles Muller, Julia Lawall, Masahiro Yamada,
	Michal Marek, Nicolas Palix, cocci
  Cc: linux-kernel

> +(
> +* !likely(E)
> +|
> +* !unlikely(E)
> +)

Can the following code variant be nicer?

+*! \( likely \| unlikely \) (E)


> +(
> +-!likely(E)
> ++unlikely(E)
> +|
> +-!unlikely(E)
> ++likely(E)
> +)

I would find the following SmPL change specification more succinct.

+(
+-!likely
++unlikely
+|
+-!unlikely
++likely
+)(E)


> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
> +msg="WARNING: Use unlikely instead of !likely"
> +coccilib.report.print_report(p[0], msg)

1. I find such a message construction nicer without the extra variable “msg”.

2. I recommend to make the provided information unique.
   * How do you think about to split the SmPL disjunction in the rule “r”
     for this purpose?

   * Should the transformation become clearer?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 13:05 [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage Denis Efremov
  2019-08-25 15:30 ` Markus Elfring
@ 2019-08-25 15:30 ` Markus Elfring
  2019-08-25 16:37 ` Joe Perches
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
  3 siblings, 0 replies; 25+ messages in thread
From: Markus Elfring @ 2019-08-25 15:30 UTC (permalink / raw)
  To: Denis Efremov, Gilles Muller, Julia Lawall, Masahiro Yamada,
	Michal Marek, Nicolas Palix, cocci
  Cc: linux-kernel

> +(
> +* !likely(E)
> +|
> +* !unlikely(E)
> +)

Can the following code variant be nicer?

+*! \( likely \| unlikely \) (E)


> +(
> +-!likely(E)
> ++unlikely(E)
> +|
> +-!unlikely(E)
> ++likely(E)
> +)

I would find the following SmPL change specification more succinct.

+(
+-!likely
++unlikely
+|
+-!unlikely
++likely
+)(E)


> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
> +msg="WARNING: Use unlikely instead of !likely"
> +coccilib.report.print_report(p[0], msg)

1. I find such a message construction nicer without the extra variable “msg”.

2. I recommend to make the provided information unique.
   * How do you think about to split the SmPL disjunction in the rule “r”
     for this purpose?

   * Should the transformation become clearer?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 13:05 [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage Denis Efremov
  2019-08-25 15:30 ` Markus Elfring
  2019-08-25 15:30 ` Markus Elfring
@ 2019-08-25 16:37 ` Joe Perches
  2019-08-25 18:59   ` Denis Efremov
       [not found]   ` <20190901172403.GA1047@bug>
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
  3 siblings, 2 replies; 25+ messages in thread
From: Joe Perches @ 2019-08-25 16:37 UTC (permalink / raw)
  To: Denis Efremov, cocci; +Cc: Michal Marek, Nicolas Palix, linux-kernel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 16:37 ` Joe Perches
@ 2019-08-25 18:59   ` Denis Efremov
  2019-08-25 19:19     ` Julia Lawall
  2019-08-25 21:19     ` Denis Efremov
       [not found]   ` <20190901172403.GA1047@bug>
  1 sibling, 2 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-25 18:59 UTC (permalink / raw)
  To: Joe Perches, cocci; +Cc: Michal Marek, Nicolas Palix, linux-kernel



On 25.08.2019 19:37, Joe Perches wrote:
> 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.
> 

In my naive understanding the negation (!) before the likely/unlikely
could confuse the compiler and the initial branch-prediction intent
could be "falsified". I would say that either you need to move the
negation under the bracket "!unlikely(cond) -> unlikely(!cond)" or
you need to use likely instead "!unlikely(cond) -> likely(cond)".
However, I'm not a compiler expert to state that this is a general
rule. But, we've got 2 special macro for branch predicting, not one.
There is also ftrace in-between. I will try to do some simple
benchmarking.
 
> 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;

I think it's incorrect to say so in general. For example, on x86/64:

$ make mrproper
$ make allyesconfig
$ make && mv vmlinux vmlinux-000
$ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
$ make 
$ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
Function                                     old     new   delta
dpaa2_io_service_rearm                       357     382     +25
intel_pmu_hw_config                         1277    1285      +8
get_sigframe.isra.constprop                 1657    1665      +8
csum_partial_copy_from_user                  605     603      -2
wait_consider_task                          3807    3797     -10
__acct_update_integrals                      384     373     -11
pipe_to_sendpage                             459     447     -12
Total: Before=312759461, After=312759467, chg +0.00%

It definitely influence the way the compiler optimizes the code.  

> 
> 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 18:59   ` Denis Efremov
@ 2019-08-25 19:19     ` Julia Lawall
  2019-08-28 11:33       ` Rasmus Villemoes
  2019-08-28 12:41       ` Denis Efremov
  2019-08-25 21:19     ` Denis Efremov
  1 sibling, 2 replies; 25+ messages in thread
From: Julia Lawall @ 2019-08-25 19:19 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Joe Perches, cocci



> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote:
> 
> 
> 
>> On 25.08.2019 19:37, Joe Perches wrote:
>>> 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.
>> 
> 
> In my naive understanding the negation (!) before the likely/unlikely
> could confuse the compiler

As a human I am confused. Is !likely(x) equivalent to x or !x?

Julia


> and the initial branch-prediction intent
> could be "falsified". I would say that either you need to move the
> negation under the bracket "!unlikely(cond) -> unlikely(!cond)" or
> you need to use likely instead "!unlikely(cond) -> likely(cond)".
> However, I'm not a compiler expert to state that this is a general
> rule. But, we've got 2 special macro for branch predicting, not one.
> There is also ftrace in-between. I will try to do some simple
> benchmarking.
> 
>> 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;
> 
> I think it's incorrect to say so in general. For example, on x86/64:
> 
> $ make mrproper
> $ make allyesconfig
> $ make && mv vmlinux vmlinux-000
> $ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
> $ make 
> $ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
> add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
> Function                                     old     new   delta
> dpaa2_io_service_rearm                       357     382     +25
> intel_pmu_hw_config                         1277    1285      +8
> get_sigframe.isra.constprop                 1657    1665      +8
> csum_partial_copy_from_user                  605     603      -2
> wait_consider_task                          3807    3797     -10
> __acct_update_integrals                      384     373     -11
> pipe_to_sendpage                             459     447     -12
> Total: Before=312759461, After=312759467, chg +0.00%
> 
> It definitely influence the way the compiler optimizes the code.  
> 
>> 
>> 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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 15:30 ` Markus Elfring
@ 2019-08-25 21:06   ` Denis Efremov
  0 siblings, 0 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-25 21:06 UTC (permalink / raw)
  To: Markus Elfring, Gilles Muller, Julia Lawall, Masahiro Yamada,
	Michal Marek, Nicolas Palix, cocci
  Cc: linux-kernel



On 25.08.2019 18:30, Markus Elfring wrote:
>> +(
>> +* !likely(E)
>> +|
>> +* !unlikely(E)
>> +)
> 
> Can the following code variant be nicer?
> 
> +*! \( likely \| unlikely \) (E)
> 
> 
>> +(
>> +-!likely(E)
>> ++unlikely(E)
>> +|
>> +-!unlikely(E)
>> ++likely(E)
>> +)
> 
> I would find the following SmPL change specification more succinct.
> 
> +(
> +-!likely
> ++unlikely
> +|
> +-!unlikely
> ++likely
> +)(E)
> 
> 
>> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
> …
>> +msg="WARNING: Use unlikely instead of !likely"
>> +coccilib.report.print_report(p[0], msg)
> 
> 1. I find such a message construction nicer without the extra variable “msg”.
> 
> 2. I recommend to make the provided information unique.
>    * How do you think about to split the SmPL disjunction in the rule “r”
>      for this purpose?
> 
>    * Should the transformation become clearer?

Thank you for the review, I will prepare v2.

> 
> Regards,
> Markus
> 
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 18:59   ` Denis Efremov
  2019-08-25 19:19     ` Julia Lawall
@ 2019-08-25 21:19     ` Denis Efremov
  1 sibling, 0 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-25 21:19 UTC (permalink / raw)
  To: Joe Perches, cocci; +Cc: Michal Marek, Nicolas Palix, linux-kernel

> I think it's incorrect to say so in general. For example, on x86/64:
> 
> $ make mrproper
> $ make allyesconfig
> $ make && mv vmlinux vmlinux-000
> $ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
> $ make 
> $ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
> add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
> Function                                     old     new   delta
> dpaa2_io_service_rearm                       357     382     +25
> intel_pmu_hw_config                         1277    1285      +8
> get_sigframe.isra.constprop                 1657    1665      +8
> csum_partial_copy_from_user                  605     603      -2
> wait_consider_task                          3807    3797     -10
> __acct_update_integrals                      384     373     -11
> pipe_to_sendpage                             459     447     -12
> Total: Before=312759461, After=312759467, chg +0.00%
> 
> It definitely influence the way the compiler optimizes the code.  

Small addition:

Results with allyesconfig and KCOV, KASAN, KUBSAN, FTRACE, TRACE_BRANCH_PROFILING,
PROFILE_ALL_BRANCHES disabled:
./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
add/remove: 0/0 grow/shrink: 2/3 up/down: 22/-22 (0)
Function                                     old     new   delta
i40e_xmit_xdp_ring                           457     477     +20
__acct_update_integrals                      127     129      +2
csum_partial_copy_from_user                  208     207      -1
dpaa2_io_service_rearm                       180     177      -3
wait_consider_task                          1338    1320     -18

For defconfig:
./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux 
add/remove: 0/0 grow/shrink: 3/1 up/down: 16/-5 (11)
Function                                     old     new   delta
do_signal                                   1513    1521      +8
wait_consider_task                          2151    2157      +6
__acct_update_integrals                      127     129      +2
csum_partial_copy_from_user                  223     218      -5

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 19:19     ` Julia Lawall
@ 2019-08-28 11:33       ` Rasmus Villemoes
  2019-08-28 11:59         ` Joe Perches
                           ` (2 more replies)
  2019-08-28 12:41       ` Denis Efremov
  1 sibling, 3 replies; 25+ messages in thread
From: Rasmus Villemoes @ 2019-08-28 11:33 UTC (permalink / raw)
  To: Julia Lawall, Denis Efremov
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Joe Perches, cocci

On 25/08/2019 21.19, Julia Lawall wrote:
> 
> 
>> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote:
>>
>>
>>
>>> On 25.08.2019 19:37, Joe Perches wrote:
>>>> 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.
>>>
>>
>> In my naive understanding the negation (!) before the likely/unlikely
>> could confuse the compiler
> 
> As a human I am confused. Is !likely(x) equivalent to x or !x?

#undef likely
#undef unlikely
#define likely(x) (x)
#define unlikely(x) (x)

should be a semantic no-op. So changing !likely(x) to unlikely(x) is
completely wrong. If anything, !likely(x) can be transformed to
unlikely(!x).

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-28 11:33       ` Rasmus Villemoes
@ 2019-08-28 11:59         ` Joe Perches
  2019-08-28 12:33         ` Denis Efremov
  2019-08-28 12:33         ` Julia Lawall
  2 siblings, 0 replies; 25+ messages in thread
From: Joe Perches @ 2019-08-28 11:59 UTC (permalink / raw)
  To: Rasmus Villemoes, Julia Lawall, Denis Efremov
  Cc: Michal Marek, Nicolas Palix, cocci, linux-kernel

On Wed, 2019-08-28 at 13:33 +0200, Rasmus Villemoes wrote:
> On 25/08/2019 21.19, Julia Lawall wrote:
> > > On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote:
> > > > On 25.08.2019 19:37, Joe Perches wrote:
> > > > > 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.
> > > In my naive understanding the negation (!) before the likely/unlikely
> > > could confuse the compiler
> > As a human I am confused. Is !likely(x) equivalent to x or !x?
> 
> #undef likely
> #undef unlikely
> #define likely(x) (x)
> #define unlikely(x) (x)
> 
> should be a semantic no-op. So changing !likely(x) to unlikely(x) is
> completely wrong. If anything, !likely(x) can be transformed to
> unlikely(!x).

likely and unlikely use __builtin_expect

https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fexpect
https://stackoverflow.com/questions/7346929/what-is-the-advantage-of-gccs-builtin-expect-in-if-else-statements

It's probable that of the more than 20K uses of
likely and unlikely in the kernel, most have no
real performance effect.


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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-28 11:33       ` Rasmus Villemoes
  2019-08-28 11:59         ` Joe Perches
@ 2019-08-28 12:33         ` Denis Efremov
  2019-08-28 12:33         ` Julia Lawall
  2 siblings, 0 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-28 12:33 UTC (permalink / raw)
  To: Rasmus Villemoes, Julia Lawall
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Joe Perches, cocci

On 8/28/19 2:33 PM, Rasmus Villemoes wrote:
> On 25/08/2019 21.19, Julia Lawall wrote:
>>
>>
>>> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote:
>>>
>>>
>>>
>>>> On 25.08.2019 19:37, Joe Perches wrote:
>>>>> 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.
>>>>
>>>
>>> In my naive understanding the negation (!) before the likely/unlikely
>>> could confuse the compiler
>>
>> As a human I am confused. Is !likely(x) equivalent to x or !x?
> 
> #undef likely
> #undef unlikely
> #define likely(x) (x)
> #define unlikely(x) (x)
> 
> should be a semantic no-op. So changing !likely(x) to unlikely(x) is
> completely wrong. If anything, !likely(x) can be transformed to
> unlikely(!x).

As far as I could understand it:

# define likely(x)	__builtin_expect(!!(x), 1)
# define unlikely(x)	__builtin_expect(!!(x), 0)

From GCC doc:
__builtin_expect compares the values. The semantics of the built-in are that it is expected that exp == c.

if (!likely(cond))
if (!__builtin_expect(!!(cond), 1))
if (!((!!(cond)) == 1))
if ((!!(cond)) != 1) and since !! could result in 0 or 1
if ((!!(cond)) == 0)

if (unlikely(cond))
if (__builtin_expect(!!(cond), 0))
if ((!!(cond)) == 0))

Thanks,
Denis

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-28 11:33       ` Rasmus Villemoes
  2019-08-28 11:59         ` Joe Perches
  2019-08-28 12:33         ` Denis Efremov
@ 2019-08-28 12:33         ` Julia Lawall
  2 siblings, 0 replies; 25+ messages in thread
From: Julia Lawall @ 2019-08-28 12:33 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Joe Perches, cocci



On Wed, 28 Aug 2019, Rasmus Villemoes wrote:

> On 25/08/2019 21.19, Julia Lawall wrote:
> >
> >
> >> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote:
> >>
> >>
> >>
> >>> On 25.08.2019 19:37, Joe Perches wrote:
> >>>> 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.
> >>>
> >>
> >> In my naive understanding the negation (!) before the likely/unlikely
> >> could confuse the compiler
> >
> > As a human I am confused. Is !likely(x) equivalent to x or !x?
>
> #undef likely
> #undef unlikely
> #define likely(x) (x)
> #define unlikely(x) (x)
>
> should be a semantic no-op. So changing !likely(x) to unlikely(x) is
> completely wrong. If anything, !likely(x) can be transformed to
> unlikely(!x).

Thanks.  Making the change seems like a good idea.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 19:19     ` Julia Lawall
  2019-08-28 11:33       ` Rasmus Villemoes
@ 2019-08-28 12:41       ` Denis Efremov
  2019-08-28 13:57         ` Denis Efremov
  1 sibling, 1 reply; 25+ messages in thread
From: Denis Efremov @ 2019-08-28 12:41 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Joe Perches, cocci


> 
> As a human I am confused. Is !likely(x) equivalent to x or !x?
> 
> Julia
> 

As far as I could understand it:

# define likely(x)	__builtin_expect(!!(x), 1)

!likely(x)
!__builtin_expect(!!(x), 1)
!((!!(x)) == 1)
(!!(x)) != 1, since !! could result in 0 or 1
(!!(x)) == 0
!(!!(x))
!!!(x)
!(x)

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
  2019-08-28 12:41       ` Denis Efremov
@ 2019-08-28 13:57         ` Denis Efremov
  0 siblings, 0 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-28 13:57 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, linux-kernel,
	Joe Perches, cocci

On 8/28/19 3:41 PM, Denis Efremov wrote:
> 
>>
>> As a human I am confused. Is !likely(x) equivalent to x or !x?
>>
>> Julia
>>
> 
> As far as I could understand it:
> 
> # define likely(x)	__builtin_expect(!!(x), 1)
> !likely(x)
> !__builtin_expect(!!(x), 1)
> !((!!(x)) == 1)
> (!!(x)) != 1, since !! could result in 0 or 1
> (!!(x)) == 0
> !(!!(x))
> !!!(x)
> !(x)
> 

Thanks Rasmus for the explanation, this should be:
!likely(x)
!__builtin_expect(!!(x), 1)
!(!!(x))
!!!(x)
!(x)

Denis

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-25 13:05 [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage Denis Efremov
                   ` (2 preceding siblings ...)
  2019-08-25 16:37 ` Joe Perches
@ 2019-08-29 17:10 ` " Denis Efremov
  2019-08-29 17:13   ` Denis Efremov
                     ` (3 more replies)
  3 siblings, 4 replies; 25+ messages in thread
From: Denis Efremov @ 2019-08-29 17:10 UTC (permalink / raw)
  To: linux-kernel, cocci
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, Markus Elfring,
	Joe Perches

This patch adds coccinelle script for detecting !likely and
!unlikely usage. These notations are confusing. It's better
to replace !likely(x) with unlikely(!x) and !unlikely(x) with
likely(!x) for readability.

The rule transforms !likely(x) to unlikely(!x) based on this logic:
  !likely(x) iff
  !__builtin_expect(!!(x), 1) iff
   __builtin_expect(!!!(x), 0) iff
  unlikely(!x)

For !unlikely(x) to likely(!x):
  !unlikely(x) iff
  !__builtin_expect(!!(x), 0) iff
  __builtin_expect(!!!(x), 1) iff
  likely(!x)

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: Joe Perches <joe@perches.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 scripts/coccinelle/misc/neg_unlikely.cocci | 89 ++++++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 scripts/coccinelle/misc/neg_unlikely.cocci

diff --git a/scripts/coccinelle/misc/neg_unlikely.cocci b/scripts/coccinelle/misc/neg_unlikely.cocci
new file mode 100644
index 000000000000..9aac0a7ff042
--- /dev/null
+++ b/scripts/coccinelle/misc/neg_unlikely.cocci
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Use unlikely(!x) instead of !likely(x) and vice versa.
+/// Notations !unlikely(x) and !likely(x) are confusing.
+//
+// Confidence: High
+// Copyright: (C) 2019 Denis Efremov, ISPRAS.
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+//  For context mode
+//----------------------------------------------------------
+
+@depends on context disable unlikely@
+expression E;
+@@
+
+*! \( likely \| unlikely \) (E)
+
+//----------------------------------------------------------
+//  For patch mode
+//----------------------------------------------------------
+
+@depends on patch disable unlikely@
+expression E;
+@@
+
+(
+-!likely(!E)
++unlikely(E)
+|
+-!likely(E)
++unlikely(!E)
+|
+-!unlikely(!E)
++likely(E)
+|
+-!unlikely(E)
++likely(!E)
+)
+
+//----------------------------------------------------------
+//  For org and report mode
+//----------------------------------------------------------
+
+@r1 depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+!likely@p(E)
+
+@r2 depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+!unlikely@p(E)
+
+@script:python depends on org && r1@
+p << r1.p;
+@@
+
+coccilib.org.print_todo(p[0], "unlikely(!x) is more readable than !likely(x)")
+
+@script:python depends on org && r2@
+p << r2.p;
+@@
+
+coccilib.org.print_todo(p[0], "likely(!x) is more readable than !unlikely(x)")
+
+@script:python depends on report && r1@
+p << r1.p;
+@@
+
+coccilib.report.print_report(p[0],
+	"unlikely(!x) is more readable than !likely(x)")
+
+@script:python depends on report && r2@
+p << r2.p;
+@@
+
+coccilib.report.print_report(p[0],
+	"likely(!x) is more readable than !unlikely(x)")
+
-- 
2.21.0

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
@ 2019-08-29 17:13   ` Denis Efremov
  2019-08-30  0:42     ` Julia Lawall
  2019-08-29 20:07   ` Markus Elfring
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 25+ messages in thread
From: Denis Efremov @ 2019-08-29 17:13 UTC (permalink / raw)
  To: linux-kernel, cocci
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, Markus Elfring,
	Joe Perches

On 8/29/19 8:10 PM, Denis Efremov wrote:
> This patch adds coccinelle script for detecting !likely and
> !unlikely usage. These notations are confusing. It's better
> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> likely(!x) for readability.

I'm not sure that this rule deserves the acceptance.
Just to want to be sure that "!unlikely(x)" and "!likely(x)"
are hard-readable is not only my perception and that they
become more clear in form "likely(!x)" and "unlikely(!x)" too.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
  2019-08-29 17:13   ` Denis Efremov
@ 2019-08-29 20:07   ` Markus Elfring
  2019-08-30  7:55   ` Markus Elfring
  2019-09-06 20:19   ` Julia Lawall
  3 siblings, 0 replies; 25+ messages in thread
From: Markus Elfring @ 2019-08-29 20:07 UTC (permalink / raw)
  To: Denis Efremov, Gilles Muller, Julia Lawall, Joe Perches,
	Masahiro Yamada, Michal Marek, Nicolas Palix, Rasmus Villemoes,
	cocci
  Cc: linux-kernel

> +@script:python depends on report && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> +	"unlikely(!x) is more readable than !likely(x)")

Can alignment matter for the string literal together with
the first method parameter?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-29 17:13   ` Denis Efremov
@ 2019-08-30  0:42     ` Julia Lawall
  2019-08-30  6:56       ` Denis Efremov
  0 siblings, 1 reply; 25+ messages in thread
From: Julia Lawall @ 2019-08-30  0:42 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, linux-kernel,
	Markus Elfring, Joe Perches, cocci



On Thu, 29 Aug 2019, Denis Efremov wrote:

> On 8/29/19 8:10 PM, Denis Efremov wrote:
> > This patch adds coccinelle script for detecting !likely and
> > !unlikely usage. These notations are confusing. It's better
> > to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> > likely(!x) for readability.
>
> I'm not sure that this rule deserves the acceptance.
> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
> are hard-readable is not only my perception and that they
> become more clear in form "likely(!x)" and "unlikely(!x)" too.

Is likely/unlikely even useful for anything once it is a subexpression?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-30  0:42     ` Julia Lawall
@ 2019-08-30  6:56       ` Denis Efremov
  2019-08-30  8:06         ` Rasmus Villemoes
  0 siblings, 1 reply; 25+ messages in thread
From: Denis Efremov @ 2019-08-30  6:56 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, linux-kernel,
	Markus Elfring, Joe Perches, cocci



On 30.08.2019 03:42, Julia Lawall wrote:
> 
> 
> On Thu, 29 Aug 2019, Denis Efremov wrote:
> 
>> On 8/29/19 8:10 PM, Denis Efremov wrote:
>>> This patch adds coccinelle script for detecting !likely and
>>> !unlikely usage. These notations are confusing. It's better
>>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>>> likely(!x) for readability.
>>
>> I'm not sure that this rule deserves the acceptance.
>> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
>> are hard-readable is not only my perception and that they
>> become more clear in form "likely(!x)" and "unlikely(!x)" too.
> 
> Is likely/unlikely even useful for anything once it is a subexpression?
>> julia
> 

Well, as far as I understand it,

It's correct since it sets the probability of likely/unlikely subexpression
is true to 90% (see https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Other-Builtins.html).
The probability of a whole expression is then computed by GCC
in this case. It's kind of assigning individual weights to conjuncts/disjuncts.
I think that it can be useful when you are not sure about the probability
of the whole expression but you know something about subexpressions it consists, e.g.,
likely(E1) && E2. However, I think that "!unlikely(x)" is fully equivalent in this sense
to "likely(!x)". I tested it once again for allyesconfig with branch profiling
disabled and bloat-o-meter shows no diff in binary size.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
  2019-08-29 17:13   ` Denis Efremov
  2019-08-29 20:07   ` Markus Elfring
@ 2019-08-30  7:55   ` Markus Elfring
  2019-09-06 20:19   ` Julia Lawall
  3 siblings, 0 replies; 25+ messages in thread
From: Markus Elfring @ 2019-08-30  7:55 UTC (permalink / raw)
  To: Denis Efremov, cocci, kernel-janitors, Gilles Muller,
	Julia Lawall, Joe Perches, Masahiro Yamada, Michal Marek,
	Nicolas Palix, Rasmus Villemoes
  Cc: linux-kernel

> +/// Notations !unlikely(x) and !likely(x) are confusing.

I am curious if more software developers will share their views around
these likeliness annotations.

* How much does the scope matter for expressions?

* Are different coding style preferences involved?


> +//----------------------------------------------------------
> +//  For context mode
> +//----------------------------------------------------------
> +
> +@depends on context disable unlikely@

I wonder about the need for such a comment when the specification
of SmPL rule dependencies should be sufficient.


> +@depends on patch disable unlikely@
> +expression E;
> +@@
> +
> +(
> +-!likely(!E)
> ++unlikely(E)
> +|
> +-!likely(E)
> ++unlikely(!E)
> +|
> +-!unlikely(!E)
> ++likely(E)
> +|
> +-!unlikely(E)
> ++likely(!E)
> +)

Will another variant for the change specification with the semantic
patch language influence corresponding readability concerns?

+@replacement depends on patch disable unlikely@
+expression x;
+@@
+-!
+(
+(
+-unlikely
++likely
+|
+-likely
++unlikely
+)
+       (
+-       !
+        x
+       )
+|
+(
+-unlikely
++likely
+|
+-likely
++unlikely
+)
+       (
++       !
+        x
+       )
+)


Can the use of nested SmPL disjunctions help here together with
an other SmPL code formatting?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-30  6:56       ` Denis Efremov
@ 2019-08-30  8:06         ` Rasmus Villemoes
  0 siblings, 0 replies; 25+ messages in thread
From: Rasmus Villemoes @ 2019-08-30  8:06 UTC (permalink / raw)
  To: Denis Efremov, Julia Lawall
  Cc: Michal Marek, Nicolas Palix, linux-kernel, Markus Elfring,
	Joe Perches, cocci

On 30/08/2019 08.56, Denis Efremov wrote:
> 
> 
> On 30.08.2019 03:42, Julia Lawall wrote:
>>
>>
>> On Thu, 29 Aug 2019, Denis Efremov wrote:
>>
>>> On 8/29/19 8:10 PM, Denis Efremov wrote:
>>>> This patch adds coccinelle script for detecting !likely and
>>>> !unlikely usage. These notations are confusing. It's better
>>>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>>>> likely(!x) for readability.
>>>
>>> I'm not sure that this rule deserves the acceptance.
>>> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
>>> are hard-readable is not only my perception and that they
>>> become more clear in form "likely(!x)" and "unlikely(!x)" too.
>>
>> Is likely/unlikely even useful for anything once it is a subexpression?
>>> julia
>>
> 
> Well, as far as I understand it,

Yes, and it could in fact make sense in cases like

  if (likely(foo->bar) && unlikely(foo->bar->baz)) {
     do_stuff_with(foo->bar->baz);
     do_more_stuff();
  }

which the compiler could then compile as (of course actual code
generation is always much more complicated due to things in the
surrounding code)

load foo->bar;
test bar;
if 0 goto skip;
load bar->baz;
test baz;
if !0 goto far_away;
skip:
  ....

so in the normal flow, neither branch is taken. If instead one wrote
unlikely(foo->bar && foo->bar->baz), gcc doesn't really know why we
expect the whole conjuntion to turn out false, so it could compile this
as a jump when foo->bar turns out non-zero - i.e., in the normal case,
we'd end up jumping.

But as far as !(un)likely(), I agree that it's easier to read as a human
if the ! operator is moved inside (and the "un" prefix stripped/added).
Whether it deserves a cocci script I don't know.

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
       [not found]   ` <20190901172403.GA1047@bug>
@ 2019-09-01 17:39     ` Denis Efremov
  0 siblings, 0 replies; 25+ messages in thread
From: Denis Efremov @ 2019-09-01 17:39 UTC (permalink / raw)
  To: Pavel Machek, Joe Perches
  Cc: Michal Marek, Nicolas Palix, linux-kernel, cocci



On 01.09.2019 20:24, Pavel Machek wrote:
> Hi!
> 
>>> 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))
> 
> Are you sure this is equivalent?
> 
> 									Pavel

Hi,

No, it's not correct. Thanks Rasmus for clarifying the things here.
This was my mistake. As a human, I failed to parse "likely" and "!"
and made too hasty conclusions :)

The correct transformation is in v2. This will be
!likely(tsk->mm) -> unlikely(!tsk->mm)

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
                     ` (2 preceding siblings ...)
  2019-08-30  7:55   ` Markus Elfring
@ 2019-09-06 20:19   ` Julia Lawall
  2019-09-06 20:55     ` Denis Efremov
  3 siblings, 1 reply; 25+ messages in thread
From: Julia Lawall @ 2019-09-06 20:19 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, linux-kernel,
	Markus Elfring, Joe Perches, cocci



On Thu, 29 Aug 2019, Denis Efremov wrote:

> This patch adds coccinelle script for detecting !likely and
> !unlikely usage. These notations are confusing. It's better
> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> likely(!x) for readability.
>
> The rule transforms !likely(x) to unlikely(!x) based on this logic:
>   !likely(x) iff
>   !__builtin_expect(!!(x), 1) iff
>    __builtin_expect(!!!(x), 0) iff
>   unlikely(!x)
>
> For !unlikely(x) to likely(!x):
>   !unlikely(x) iff
>   !__builtin_expect(!!(x), 0) iff
>   __builtin_expect(!!!(x), 1) iff
>   likely(!x)
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> Cc: Nicolas Palix <nicolas.palix@imag.fr>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: Markus Elfring <Markus.Elfring@web.de>
> Cc: Joe Perches <joe@perches.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

A small improvement though would be to improve the explicit dependency of
the last four python rules on r1 and r2.  Those rules won't execute unless
the inherited metavariable has a value, which makes the same dependency.

julia


> ---
>  scripts/coccinelle/misc/neg_unlikely.cocci | 89 ++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/neg_unlikely.cocci
>
> diff --git a/scripts/coccinelle/misc/neg_unlikely.cocci b/scripts/coccinelle/misc/neg_unlikely.cocci
> new file mode 100644
> index 000000000000..9aac0a7ff042
> --- /dev/null
> +++ b/scripts/coccinelle/misc/neg_unlikely.cocci
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// Use unlikely(!x) instead of !likely(x) and vice versa.
> +/// Notations !unlikely(x) and !likely(x) are confusing.
> +//
> +// Confidence: High
> +// Copyright: (C) 2019 Denis Efremov, ISPRAS.
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +//----------------------------------------------------------
> +//  For context mode
> +//----------------------------------------------------------
> +
> +@depends on context disable unlikely@
> +expression E;
> +@@
> +
> +*! \( likely \| unlikely \) (E)
> +
> +//----------------------------------------------------------
> +//  For patch mode
> +//----------------------------------------------------------
> +
> +@depends on patch disable unlikely@
> +expression E;
> +@@
> +
> +(
> +-!likely(!E)
> ++unlikely(E)
> +|
> +-!likely(E)
> ++unlikely(!E)
> +|
> +-!unlikely(!E)
> ++likely(E)
> +|
> +-!unlikely(E)
> ++likely(!E)
> +)
> +
> +//----------------------------------------------------------
> +//  For org and report mode
> +//----------------------------------------------------------
> +
> +@r1 depends on (org || report) disable unlikely@
> +expression E;
> +position p;
> +@@
> +
> +!likely@p(E)
> +
> +@r2 depends on (org || report) disable unlikely@
> +expression E;
> +position p;
> +@@
> +
> +!unlikely@p(E)
> +
> +@script:python depends on org && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "unlikely(!x) is more readable than !likely(x)")
> +
> +@script:python depends on org && r2@
> +p << r2.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "likely(!x) is more readable than !unlikely(x)")
> +
> +@script:python depends on report && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> +	"unlikely(!x) is more readable than !likely(x)")
> +
> +@script:python depends on report && r2@
> +p << r2.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> +	"likely(!x) is more readable than !unlikely(x)")
> +
> --
> 2.21.0
>
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage
  2019-09-06 20:19   ` Julia Lawall
@ 2019-09-06 20:55     ` Denis Efremov
  2019-09-07  8:05       ` [Cocci] [v2] " Markus Elfring
  0 siblings, 1 reply; 25+ messages in thread
From: Denis Efremov @ 2019-09-06 20:55 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Michal Marek, Rasmus Villemoes, Nicolas Palix, linux-kernel,
	Markus Elfring, Joe Perches, cocci

Hi,

On 06.09.2019 23:19, Julia Lawall wrote:
> 
> 
> On Thu, 29 Aug 2019, Denis Efremov wrote:
> 
>> This patch adds coccinelle script for detecting !likely and
>> !unlikely usage. These notations are confusing. It's better
>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>> likely(!x) for readability.
>>
>> The rule transforms !likely(x) to unlikely(!x) based on this logic:
>>   !likely(x) iff
>>   !__builtin_expect(!!(x), 1) iff
>>    __builtin_expect(!!!(x), 0) iff
>>   unlikely(!x)
>>
>> For !unlikely(x) to likely(!x):
>>   !unlikely(x) iff
>>   !__builtin_expect(!!(x), 0) iff
>>   __builtin_expect(!!!(x), 1) iff
>>   likely(!x)
>>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
>> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
>> Cc: Nicolas Palix <nicolas.palix@imag.fr>
>> Cc: Michal Marek <michal.lkml@markovi.net>
>> Cc: Markus Elfring <Markus.Elfring@web.de>
>> Cc: Joe Perches <joe@perches.com>
>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>
> 
> A small improvement though would be to improve the explicit dependency of
> the last four python rules on r1 and r2.  Those rules won't execute unless
> the inherited metavariable has a value, which makes the same dependency.
> 
> julia

I think I will resend this patch as a part of patchset with all warnings fixed
in a couple of days. Hope this will help to create a discussion point with other
developers about readability of "!likely" and "!unlikely".

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Cocci] [v2] scripts: coccinelle: check for !(un)?likely usage
  2019-09-06 20:55     ` Denis Efremov
@ 2019-09-07  8:05       ` " Markus Elfring
  0 siblings, 0 replies; 25+ messages in thread
From: Markus Elfring @ 2019-09-07  8:05 UTC (permalink / raw)
  To: Denis Efremov, Julia Lawall, cocci
  Cc: Michal Marek, Nicolas Palix, Rasmus Villemoes, linux-kernel, Joe Perches

> I think I will resend this patch as a part of patchset with all warnings fixed
> in a couple of days. Hope this will help to create a discussion point with other
> developers about readability of "!likely" and "!unlikely".

Will the influence of code variations become more interesting also for
the discussed SmPL script?

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, back to index

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-25 13:05 [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage Denis Efremov
2019-08-25 15:30 ` Markus Elfring
2019-08-25 21:06   ` Denis Efremov
2019-08-25 15:30 ` Markus Elfring
2019-08-25 16:37 ` Joe Perches
2019-08-25 18:59   ` Denis Efremov
2019-08-25 19:19     ` Julia Lawall
2019-08-28 11:33       ` Rasmus Villemoes
2019-08-28 11:59         ` Joe Perches
2019-08-28 12:33         ` Denis Efremov
2019-08-28 12:33         ` Julia Lawall
2019-08-28 12:41       ` Denis Efremov
2019-08-28 13:57         ` Denis Efremov
2019-08-25 21:19     ` Denis Efremov
     [not found]   ` <20190901172403.GA1047@bug>
2019-09-01 17:39     ` Denis Efremov
2019-08-29 17:10 ` [Cocci] [PATCH v2] " Denis Efremov
2019-08-29 17:13   ` Denis Efremov
2019-08-30  0:42     ` Julia Lawall
2019-08-30  6:56       ` Denis Efremov
2019-08-30  8:06         ` Rasmus Villemoes
2019-08-29 20:07   ` Markus Elfring
2019-08-30  7:55   ` Markus Elfring
2019-09-06 20:19   ` Julia Lawall
2019-09-06 20:55     ` Denis Efremov
2019-09-07  8:05       ` [Cocci] [v2] " Markus Elfring

Coccinelle archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr cocci@archiver.kernel.org
	public-inbox-index cocci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox