* [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 07/11] xfs: remove unlikely() from WARN_ON() 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; 9+ 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] 9+ messages in thread
* [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() 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-29 21:22 ` Darrick J. Wong
2019-08-30 5:37 ` Christoph Hellwig
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
1 sibling, 2 replies; 9+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, Darrick J. Wong, Joe Perches, Andrew Morton, linux-xfs
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-xfs@vger.kernel.org
---
fs/xfs/xfs_buf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ca0849043f54..4389d87ff0f0 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2096,7 +2096,7 @@ xfs_verify_magic(
int idx;
idx = xfs_sb_version_hascrc(&mp->m_sb);
- if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
+ if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
return false;
return dmagic == bp->b_ops->magic[idx];
}
@@ -2114,7 +2114,7 @@ xfs_verify_magic16(
int idx;
idx = xfs_sb_version_hascrc(&mp->m_sb);
- if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
+ if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
return false;
return dmagic == bp->b_ops->magic16[idx];
}
--
2.21.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition
2019-08-29 16:50 ` [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition Denis Efremov
@ 2019-08-29 21:22 ` Darrick J. Wong
2019-08-30 5:37 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2019-08-29 21:22 UTC (permalink / raw)
To: Denis Efremov; +Cc: linux-kernel, Joe Perches, Andrew Morton, linux-xfs
On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-xfs@vger.kernel.org
LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_buf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ca0849043f54..4389d87ff0f0 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2096,7 +2096,7 @@ xfs_verify_magic(
> int idx;
>
> idx = xfs_sb_version_hascrc(&mp->m_sb);
> - if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
> + if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
> return false;
> return dmagic == bp->b_ops->magic[idx];
> }
> @@ -2114,7 +2114,7 @@ xfs_verify_magic16(
> int idx;
>
> idx = xfs_sb_version_hascrc(&mp->m_sb);
> - if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
> + if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
> return false;
> return dmagic == bp->b_ops->magic16[idx];
> }
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition
2019-08-29 16:50 ` [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition Denis Efremov
2019-08-29 21:22 ` Darrick J. Wong
@ 2019-08-30 5:37 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-08-30 5:37 UTC (permalink / raw)
To: Denis Efremov
Cc: linux-kernel, Darrick J. Wong, Joe Perches, Andrew Morton, linux-xfs
On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ 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 07/11] xfs: remove unlikely() from WARN_ON() condition Denis Efremov
@ 2019-08-31 9:15 ` Markus Elfring
2019-08-31 15:54 ` Denis Efremov
1 sibling, 1 reply; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2019-08-31 17:27 UTC | newest]
Thread overview: 9+ 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 07/11] xfs: remove unlikely() from WARN_ON() condition Denis Efremov
2019-08-29 21:22 ` Darrick J. Wong
2019-08-30 5:37 ` Christoph Hellwig
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).