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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 25 Aug 2019 21:59:10 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Joe Perches <joe@...ches.com>, cocci@...teme.lip6.fr
Cc:     linux-kernel@...r.kernel.org, Julia Lawall <Julia.Lawall@...6.fr>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <michal.lkml@...kovi.net>
Subject: Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage



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@...ivers/@) {
> 
>