* [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
@ 2019-08-29 16:50 Denis Efremov
2019-08-29 16:50 ` [PATCH v3 08/11] IB/hfi1: Remove unlikely() from IS_ERR*() condition Denis Efremov
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
0 siblings, 2 replies; 7+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, Alexander Viro, Anton Altaparmakov,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov,
Inaky Perez-Gonzalez, Juergen Gross, Leon Romanovsky,
Mike Marciniszyn, Pali Rohár, Rob Clark, Saeed Mahameed,
Sean Paul, linux-arm-msm, linux-fsdevel, linux-input,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs, xen-devel,
netdev, dri-devel, Joe Perches, Andrew Morton, Andy Whitcroft
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 08/11] IB/hfi1: Remove unlikely() from IS_ERR*() condition
2019-08-29 16:50 [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Denis Efremov
@ 2019-08-29 16:50 ` Denis Efremov
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
1 sibling, 0 replies; 7+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, Mike Marciniszyn, Dennis Dalessandro, Joe Perches,
Andrew Morton, linux-rdma
"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: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-rdma@vger.kernel.org
---
drivers/infiniband/hw/hfi1/verbs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 646f61545ed6..1c52b19dd0f8 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1040,7 +1040,7 @@ int hfi1_verbs_send_pio(struct rvt_qp *qp, struct hfi1_pkt_state *ps,
if (cb)
iowait_pio_inc(&priv->s_iowait);
pbuf = sc_buffer_alloc(sc, plen, cb, qp);
- if (unlikely(IS_ERR_OR_NULL(pbuf))) {
+ if (IS_ERR_OR_NULL(pbuf)) {
if (cb)
verbs_pio_complete(qp, 0);
if (IS_ERR(pbuf)) {
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
2019-08-29 16:50 [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Denis Efremov
2019-08-29 16:50 ` [PATCH v3 08/11] IB/hfi1: Remove unlikely() from IS_ERR*() condition Denis Efremov
@ 2019-08-31 9:15 ` Markus Elfring
2019-08-31 15:54 ` Denis Efremov
1 sibling, 1 reply; 7+ messages in thread
From: Markus Elfring @ 2019-08-31 9:15 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß,
Leon Romanovsky, linux-arm-msm, linux-fsdevel, linux-input,
linux-kernel, linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
> +# 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
@ 2019-08-31 15:54 ` Denis Efremov
2019-08-31 16:45 ` Markus Elfring
0 siblings, 1 reply; 7+ messages in thread
From: Denis Efremov @ 2019-08-31 15:54 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß,
Leon Romanovsky, linux-arm-msm, linux-fsdevel, linux-input,
linux-kernel, linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
2019-08-31 15:54 ` Denis Efremov
@ 2019-08-31 16:45 ` Markus Elfring
2019-08-31 17:07 ` Denis Efremov
0 siblings, 1 reply; 7+ messages in thread
From: Markus Elfring @ 2019-08-31 16:45 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß,
Leon Romanovsky, linux-arm-msm, linux-fsdevel, linux-input,
linux-kernel, linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
>>> +# 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
2019-08-31 16:45 ` Markus Elfring
@ 2019-08-31 17:07 ` Denis Efremov
2019-08-31 17:26 ` Markus Elfring
0 siblings, 1 reply; 7+ messages in thread
From: Denis Efremov @ 2019-08-31 17:07 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß,
Leon Romanovsky, linux-arm-msm, linux-fsdevel, linux-input,
linux-kernel, linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
2019-08-31 17:07 ` Denis Efremov
@ 2019-08-31 17:26 ` Markus Elfring
0 siblings, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2019-08-31 17:26 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong,
David S. Miller, Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß,
Leon Romanovsky, linux-arm-msm, linux-fsdevel, linux-input,
linux-kernel, linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
>>>>> +# 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-31 17:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-29 16:50 [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Denis Efremov
2019-08-29 16:50 ` [PATCH v3 08/11] IB/hfi1: Remove unlikely() from IS_ERR*() condition Denis Efremov
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
2019-08-31 15:54 ` Denis Efremov
2019-08-31 16:45 ` Markus Elfring
2019-08-31 17:07 ` Denis Efremov
2019-08-31 17:26 ` Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).