* [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 02/11] drm/msm: remove unlikely() from WARN_ON() conditions Denis Efremov
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
0 siblings, 2 replies; 8+ 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] 8+ messages in thread
* [PATCH v3 02/11] drm/msm: remove unlikely() from WARN_ON() conditions
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-09-04 4:13 ` Bjorn Andersson
2019-08-31 9:15 ` [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls Markus Elfring
1 sibling, 1 reply; 8+ messages in thread
From: Denis Efremov @ 2019-08-29 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Denis Efremov, Rob Clark, Sean Paul, Joe Perches, Andrew Morton,
linux-arm-msm, dri-devel
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
index 4804cf40de14..030279d7b64b 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
@@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
u32 blend_cfg;
struct mdp5_hw_mixer *mixer = pipeline->mixer;
- if (unlikely(WARN_ON(!mixer))) {
+ if (WARN_ON(!mixer)) {
DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
ctl->id);
return -EINVAL;
@@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
goto fail;
}
- if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
+ if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
ctl_cfg->count);
ret = -ENOSPC;
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
index 8afb0f9c04bb..5495d8b3f5b9 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
{
- if (unlikely(WARN_ON(type >= CSC_MAX)))
+ if (WARN_ON(type >= CSC_MAX))
return NULL;
return &csc_convert[type];
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ 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 02/11] drm/msm: remove unlikely() from WARN_ON() conditions Denis Efremov
@ 2019-08-31 9:15 ` Markus Elfring
2019-08-31 15:54 ` Denis Efremov
1 sibling, 1 reply; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ messages in thread
* Re: [PATCH v3 02/11] drm/msm: remove unlikely() from WARN_ON() conditions
2019-08-29 16:50 ` [PATCH v3 02/11] drm/msm: remove unlikely() from WARN_ON() conditions Denis Efremov
@ 2019-09-04 4:13 ` Bjorn Andersson
0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Andersson @ 2019-09-04 4:13 UTC (permalink / raw)
To: Denis Efremov
Cc: linux-kernel, Rob Clark, Sean Paul, Joe Perches, Andrew Morton,
linux-arm-msm, dri-devel
On Thu 29 Aug 09:50 PDT 2019, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
> drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> index 4804cf40de14..030279d7b64b 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> @@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
> u32 blend_cfg;
> struct mdp5_hw_mixer *mixer = pipeline->mixer;
>
> - if (unlikely(WARN_ON(!mixer))) {
> + if (WARN_ON(!mixer)) {
> DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
> ctl->id);
> return -EINVAL;
> @@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
> goto fail;
> }
>
> - if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
> + if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
> DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
> ctl_cfg->count);
> ret = -ENOSPC;
> diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
> index 8afb0f9c04bb..5495d8b3f5b9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp_format.c
> +++ b/drivers/gpu/drm/msm/disp/mdp_format.c
> @@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
>
> struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
> {
> - if (unlikely(WARN_ON(type >= CSC_MAX)))
> + if (WARN_ON(type >= CSC_MAX))
> return NULL;
>
> return &csc_convert[type];
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-04 4:13 UTC | newest]
Thread overview: 8+ 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 02/11] drm/msm: remove unlikely() from WARN_ON() conditions Denis Efremov
2019-09-04 4:13 ` Bjorn Andersson
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).