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.ne...
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/@) {
"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];
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
+# 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
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
+# 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
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
+# 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
dri-devel@lists.freedesktop.org
-
Bjorn Andersson -
Denis Efremov -
Markus Elfring