* [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 03/11] net/mlx5e: Remove unlikely() from WARN*() condition Denis Efremov
` (2 more replies)
0 siblings, 3 replies; 10+ 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] 10+ messages in thread
* [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() 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:23 ` Saeed Mahameed
2019-08-29 16:50 ` [PATCH v3 10/11] udp: 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
2 siblings, 1 reply; 10+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, Boris Pismenny, Saeed Mahameed, Leon Romanovsky,
Joe Perches, Andrew Morton, netdev
"unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Boris Pismenny <borisp@mellanox.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: netdev@vger.kernel.org
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
index 7833ddef0427..e5222d17df35 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
@@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct net_device *netdev,
goto out;
tls_ctx = tls_get_ctx(skb->sk);
- if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
+ if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
goto err_out;
priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 10/11] udp: 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 ` [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition 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
2 siblings, 0 replies; 10+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, David S. Miller, Joe Perches, Andrew Morton, netdev
"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: "David S. Miller" <davem@davemloft.net>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: netdev@vger.kernel.org
---
include/net/udp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/udp.h b/include/net/udp.h
index 79d141d2103b..bad74f780831 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -480,7 +480,7 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
* CB fragment
*/
segs = __skb_gso_segment(skb, features, false);
- if (unlikely(IS_ERR_OR_NULL(segs))) {
+ if (IS_ERR_OR_NULL(segs)) {
int segs_nr = skb_shinfo(skb)->gso_segs;
atomic_add(segs_nr, &sk->sk_drops);
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition
2019-08-29 16:50 ` [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition Denis Efremov
@ 2019-08-29 21:23 ` Saeed Mahameed
2019-08-31 2:50 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Saeed Mahameed @ 2019-08-29 21:23 UTC (permalink / raw)
To: efremov, linux-kernel, davem; +Cc: joe, Boris Pismenny, netdev, leon, akpm
On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
> unlikely() internally.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: Boris Pismenny <borisp@mellanox.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: netdev@vger.kernel.org
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> index 7833ddef0427..e5222d17df35 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
> net_device *netdev,
> goto out;
>
> tls_ctx = tls_get_ctx(skb->sk);
> - if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
> + if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
> goto err_out;
>
> priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
Dave, you can take this one.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition
2019-08-29 21:23 ` Saeed Mahameed
@ 2019-08-31 2:50 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2019-08-31 2:50 UTC (permalink / raw)
To: saeedm; +Cc: efremov, linux-kernel, joe, borisp, netdev, leon, akpm
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Thu, 29 Aug 2019 21:23:30 +0000
> On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
>> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
>> unlikely() internally.
>>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> Cc: Boris Pismenny <borisp@mellanox.com>
>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>> Cc: Leon Romanovsky <leon@kernel.org>
>> Cc: Joe Perches <joe@perches.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: netdev@vger.kernel.org
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git
>> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> index 7833ddef0427..e5222d17df35 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
>> net_device *netdev,
>> goto out;
>>
>> tls_ctx = tls_get_ctx(skb->sk);
>> - if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
>> + if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
>> goto err_out;
>>
>> priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
>
> Acked-by: Saeed Mahameed <saeedm@mellanox.com>
>
> Dave, you can take this one.
Ok, applied to net-next as well as the UDP patch.
^ permalink raw reply [flat|nested] 10+ 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 03/11] net/mlx5e: Remove unlikely() from WARN*() condition Denis Efremov
2019-08-29 16:50 ` [PATCH v3 10/11] udp: Remove unlikely() from IS_ERR*() condition Denis Efremov
@ 2019-08-31 9:15 ` Markus Elfring
2019-08-31 15:54 ` Denis Efremov
2 siblings, 1 reply; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2019-08-31 17:27 UTC | newest]
Thread overview: 10+ 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 03/11] net/mlx5e: Remove unlikely() from WARN*() condition Denis Efremov
2019-08-29 21:23 ` Saeed Mahameed
2019-08-31 2:50 ` David Miller
2019-08-29 16:50 ` [PATCH v3 10/11] udp: 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).