[PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

IS_ERR(), IS_ERR_OR_NULL(), IS_ERR_VALUE() and WARN*() already contain
unlikely() optimization internally. Thus, there is no point in calling
these functions and defines under likely()/unlikely().

This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..56969ce06df4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,12 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
+# nested likely/unlikely calls
+		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+			WARN("LIKELY_MISUSE",
+			     "nested (un)?likely() calls, $1 already uses unlikely() internally\n" . $herecurr);
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {
-- 
2.21.0

[PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Denis Efremov]

"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-input@vger.kernel.org
---
 drivers/input/mouse/alps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 34700eda0429..ed1661434899 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 		/* On V2 devices the DualPoint Stick reports bare packets */
 		dev = priv->dev2;
 		dev2 = psmouse->dev;
-	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+	} else if (IS_ERR_OR_NULL(priv->dev3)) {
 		/* Register dev3 mouse if we received PS/2 packet first time */
 		if (!IS_ERR(priv->dev3))
 			psmouse_queue_work(psmouse, &priv->dev3_register_work,
-- 
2.21.0

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Dmitry Torokhov]

On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> unlikely() internally.

The keyword here is _internally_.

https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/

So please no.

> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: "Pali Rohár" <pali.rohar@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-input@vger.kernel.org
> ---
>  drivers/input/mouse/alps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 34700eda0429..ed1661434899 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>  		/* On V2 devices the DualPoint Stick reports bare packets */
>  		dev = priv->dev2;
>  		dev2 = psmouse->dev;
> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>  		/* Register dev3 mouse if we received PS/2 packet first time */
>  		if (!IS_ERR(priv->dev3))
>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> -- 
> 2.21.0
> 

-- 
Dmitry

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

> +# nested likely/unlikely calls
> +		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> +			WARN("LIKELY_MISUSE",

How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
in this regular expression?

Regards,
Markus

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > unlikely() internally.
> 
> The keyword here is _internally_.
> 
> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> 
> So please no.

Dmitry and I already rejected this patch, see also linked-list:
https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/

> > 
> > Signed-off-by: Denis Efremov <efremov@linux.com>
> > Cc: "Pali Rohár" <pali.rohar@gmail.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Joe Perches <joe@perches.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  drivers/input/mouse/alps.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 34700eda0429..ed1661434899 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> >  		/* On V2 devices the DualPoint Stick reports bare packets */
> >  		dev = priv->dev2;
> >  		dev2 = psmouse->dev;
> > -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> > +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
> >  		/* Register dev3 mouse if we received PS/2 packet first time */
> >  		if (!IS_ERR(priv->dev3))
> >  			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> > -- 
> > 2.21.0
> > 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Denis Efremov]

On 31.08.2019 18:25, Pali Rohár wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
>> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
>>> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
>>> unlikely() internally.
>>
>> The keyword here is _internally_.
>>
>> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
>>
>> So please no.
> 
> Dmitry and I already rejected this patch, see also linked-list:
> https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/
>

Looks like this is a very long recurring story with this patch.
Thanks, for the clarification.

Regards,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> +            WARN("LIKELY_MISUSE",
> 
> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?

Hmm, 
(?:   <- Catch group is required here, since it is used in diagnostic message,
         see $1
   IS_ERR
   (?:_ <- Another atomic group just to show that '_' is a common prefix?
           I'm not sure about this. Usually, Perl interpreter is very good at optimizing such things.
           You could see this optimization if you run perl with -Mre=debug.
     (?:OR_NULL|VALUE))?|WARN)

Regards,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

>>> +# nested likely/unlikely calls
>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> +            WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
>    IS_ERR
>    (?:_ <- Another atomic group just to show that '_' is a common prefix?

Yes. - I hope that this specification detail can help a bit.


>            Usually, Perl interpreter is very good at optimizing such things.

Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?

\b(?:un)?likely\s*


Regards,
Markus

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

On 31.08.2019 19:45, Markus Elfring wrote:
>>>> +# nested likely/unlikely calls
>>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>> +            WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>>    IS_ERR
>>    (?:_ <- Another atomic group just to show that '_' is a common prefix?
> 
> Yes. - I hope that this specification detail can help a bit.

I'm not sure that another pair of brackets for a single char worth it.

>>            Usually, Perl interpreter is very good at optimizing such things.

The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
   1: EXACT <IS_ERR> (4)
   4: CURLYX[0]{0,1} (16)
   6:   EXACT <_> (8)      <-- common prefix
   8:   TRIE-EXACT[OV] (15)
        <OR_NULL> 
        <VALUE>
...
> 
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
> 
> \b(?:un)?likely\s*

This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".

Regards,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

>>>>> +# nested likely/unlikely calls
>>>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>>> +            WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".

I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing parentheses
at the front (from the version which you suggested).

Regards,
Markus

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Joe Perches]

On Sat, 2019-08-31 at 17:25 +0200, Pali Rohár wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > unlikely() internally.
> > 
> > The keyword here is _internally_.
> > 
> > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> > 
> > So please no.

I think it poor form not to simply restate your original
objection from 4 message levels below this link

https://lists.gt.net/linux/kernel/2269724

   Hm... I do not like this change. If I read code 
    
    if (unlikely(IS_ERR_OR_NULL(priv->dev3))) 
    
   then I know that it is really unlikely that condition will be truth and 
   so this is some case of error/exception or something that normally does 
   not happen too much. 
    
   But if I read code 
    
    if (IS_ERR_OR_NULL(priv->dev3)) 
    
   I know nothing about chance that this condition will be truth. Explicit 
   unlikely in previous example give me more information. 
    
I alslo think this argument is dubious as it also applies
to any IS_ERR and all the unlikely uses have been removed
from those.

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Dmitry Torokhov]

On Sat, Aug 31, 2019 at 01:32:02PM -0700, Joe Perches wrote:
> On Sat, 2019-08-31 at 17:25 +0200, Pali Rohár wrote:
> > On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > > unlikely() internally.
> > > 
> > > The keyword here is _internally_.
> > > 
> > > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> > > 
> > > So please no.
> 
> I think it poor form not to simply restate your original
> objection from 4 message levels below this link

Thank you for the lesson in etiquette, but I posted reference to the
very message I wanted.

> 
> https://lists.gt.net/linux/kernel/2269724
> 
>    Hm... I do not like this change. If I read code 
>     
>     if (unlikely(IS_ERR_OR_NULL(priv->dev3))) 
>     
>    then I know that it is really unlikely that condition will be truth and 
>    so this is some case of error/exception or something that normally does 
>    not happen too much. 
>     
>    But if I read code 
>     
>     if (IS_ERR_OR_NULL(priv->dev3)) 
>     
>    I know nothing about chance that this condition will be truth. Explicit 
>    unlikely in previous example give me more information. 
>     
> I alslo think this argument is dubious as it also applies
> to any IS_ERR and all the unlikely uses have been removed
> from those.

No, if you read the reference I posted, the argument does not apply to
all IS_ERR() instances. Majority of them are in probe() paths where we
do not really care about likely/unlikely. Here we are dealing with
IS_ERR in a [fairly] hot path.

Thanks.

-- 
Dmitry