Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 fbmov%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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 fbmov%rdi,%rbx
> u64 time, delta;
>
> - if (!likely(tsk->mm))
> + if (unlikely(tsk->mm))
Are you sure this is equivalent?
Pavel
>18: 4c 8d ab 28 02 00 00lea0x228(%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 efmov%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 00mov0x228(%rbx),%r15
> - 33: 4d 85 fftest %r15,%r15
> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
> + 2c: 48 83 bb 28 02 00 00cmpq $0x0,0x228(%rbx)
> + 33: 00
> + 34: 75 34 jne6a <__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/@) {
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
> > 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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 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.
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
On 25/08/2019 21.19, Julia Lawall wrote: > > >> On 26 Aug 2019, at 02:59, Denis Efremov 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
Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
> 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 12771285 +8 > get_sigframe.isra.constprop 16571665 +8 > csum_partial_copy_from_user 605 603 -2 > wait_consider_task 38073797 -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 13381320 -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 15131521 +8 wait_consider_task 21512157 +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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 >
Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage
> On 26 Aug 2019, at 02:59, Denis Efremov 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 fbmov%rdi,%rbx
>>u64 time, delta;
>>
>> - if (!likely(tsk->mm))
>> + if (unlikely(tsk->mm))
>> 18: 4c 8d ab 28 02 00 00lea0x228(%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 efmov%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 00mov0x228(%rbx),%r15
>> - 33: 4d 85 fftest %r15,%r15
>> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
>> + 2c: 48 83 bb 28 02 00 00cmpq $0x0,0x228(%rbx)
>> + 33: 00
>> + 34: 75 34 jne6a <__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 12771285 +8
> get_sigframe.isra.constprop 16571665 +8
> csum_partial_copy_from_user 605 603 -2
> wait_consider_task 38073797 -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
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 fbmov%rdi,%rbx
> u64 time, delta;
>
> - if (!likely(tsk->mm))
> + if (unlikely(tsk->mm))
>18: 4c 8d ab 28 02 00 00lea0x228(%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 efmov%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 00mov0x228(%rbx),%r15
> - 33: 4d 85 fftest %r15,%r15
> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
> + 2c: 48 83 bb 28 02 00 00cmpq $0x0,0x228(%rbx)
> + 33: 00
> + 34: 75 34 jne6a <__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 12771285 +8
get_sigframe.isra.constprop 16571665 +8
csum_partial_copy_from_user 605 603 -2
wait_consider_task 38073797 -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/@) {
>
>
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
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 fbmov%rdi,%rbx
u64 time, delta;
- if (!likely(tsk->mm))
+ if (unlikely(tsk->mm))
18: 4c 8d ab 28 02 00 00lea0x228(%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 efmov%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 00mov0x228(%rbx),%r15
- 33: 4d 85 fftest %r15,%r15
- 36: 74 34 je 6c <__acct_update_integrals+0x6c>
+ 2c: 48 83 bb 28 02 00 00cmpq $0x0,0x228(%rbx)
+ 33: 00
+ 34: 75 34 jne6a <__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/@) {
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
> +( > +* !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
Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage
> +( > +* !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
[PATCH] scripts: coccinelle: check for !(un)?likely usage
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 --- 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 ..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
