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

[Julia Lawall]

> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@xxxxxxxxx> 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/@) {
>> 
>>