| Message ID | 1566298572-12409-2-git-send-email-info@metux.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [1/2] fs: ntfs: drop unneeded likely() call around IS_ERR() | expand |
On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote: > From: Enrico Weigelt <info@metux.net> > > IS_ERR() already calls unlikely(), so this extra unlikely() call > around IS_ERR() is not needed. > > Signed-off-by: Enrico Weigelt <info@metux.net> > --- > drivers/input/mouse/alps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > index 34700ed..ed16614 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse, > /* On V2 devices the DualPoint Stick reports bare packets */ > dev = priv->dev2; > dev2 = psmouse->dev; > - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) { > + } else if (IS_ERR_OR_NULL(priv->dev3)) { > /* Register dev3 mouse if we received PS/2 packet first time */ > if (!IS_ERR(priv->dev3)) > psmouse_queue_work(psmouse, &priv->dev3_register_work, Hello! Two months ago I already saw this patch. See discussion: https://patchwork.kernel.org/patch/10977099/
On 20.08.19 13:17, Pali Rohár wrote: > On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote: >> From: Enrico Weigelt <info@metux.net> >> >> IS_ERR() already calls unlikely(), so this extra unlikely() call >> around IS_ERR() is not needed. >> >> Signed-off-by: Enrico Weigelt <info@metux.net> >> --- >> drivers/input/mouse/alps.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c >> index 34700ed..ed16614 100644 >> --- a/drivers/input/mouse/alps.c >> +++ b/drivers/input/mouse/alps.c >> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse, >> /* On V2 devices the DualPoint Stick reports bare packets */ >> dev = priv->dev2; >> dev2 = psmouse->dev; >> - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) { >> + } else if (IS_ERR_OR_NULL(priv->dev3)) { >> /* Register dev3 mouse if we received PS/2 packet first time */ >> if (!IS_ERR(priv->dev3)) >> psmouse_queue_work(psmouse, &priv->dev3_register_work, > > Hello! Two months ago I already saw this patch. See discussion: > https://patchwork.kernel.org/patch/10977099/ phuh, that's long chain of links to folow ;-) So, your primary argument is just *documenting* that this supposed to be a rare condition ? In that case, wouldn't a comment be more suitable for that ? It seems that this issue is coming up again and again ... when people run cocci scans (like I didn't), I'd suspect this to happen even more in the future. --mtx
On Tuesday 20 August 2019 14:21:33 Enrico Weigelt, metux IT consult wrote: > On 20.08.19 13:17, Pali Rohár wrote: > > On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote: > > > From: Enrico Weigelt <info@metux.net> > > > > > > IS_ERR() already calls unlikely(), so this extra unlikely() call > > > around IS_ERR() is not needed. > > > > > > Signed-off-by: Enrico Weigelt <info@metux.net> > > > --- > > > drivers/input/mouse/alps.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c > > > index 34700ed..ed16614 100644 > > > --- a/drivers/input/mouse/alps.c > > > +++ b/drivers/input/mouse/alps.c > > > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse, > > > /* On V2 devices the DualPoint Stick reports bare packets */ > > > dev = priv->dev2; > > > dev2 = psmouse->dev; > > > - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) { > > > + } else if (IS_ERR_OR_NULL(priv->dev3)) { > > > /* Register dev3 mouse if we received PS/2 packet first time */ > > > if (!IS_ERR(priv->dev3)) > > > psmouse_queue_work(psmouse, &priv->dev3_register_work, > > > > Hello! Two months ago I already saw this patch. See discussion: > > https://patchwork.kernel.org/patch/10977099/ > > phuh, that's long chain of links to folow ;-) > > So, your primary argument is just *documenting* that this supposed to > be a rare condition ? Yes. Also Dmitry said that prefer explicit unlikely. > In that case, wouldn't a comment be more suitable for that ? And why to add comment if current state of code is more-readable and does not need it? I do not see anything wrong on this code path. People normally add comments to code which is problematic to understand or is somehow tricky, no so obvious or document how should code behave. > It seems that this issue is coming up again and again ... when people > run cocci scans (like I didn't), I'd suspect this to happen even more > in the future. People first need to understand that static code analysis cannot work 100% reliably and always is needed to properly review changes. And if the only reason for this change is to satisfy some static code analysis program, would not it better to teach this program to no generate such false-positive results?
On 20.08.19 16:22, Pali Rohár wrote: Hi, >> In that case, wouldn't a comment be more suitable for that ? > > And why to add comment if current state of code is more-readable and > does not need it? Readability is probably a bit subjective :p With ongoing efforts of automatically identifying redundant code pathes, the current situation causes the same discussion coming up over and over again. Sooner or later somebody might get the idea to add a comment on that line, that it's exactly as intented :o OTOH, I'm unsure whether it's important to document that is particular error path is unlikely, while we don't do it in thousands of other places. IMHO, error pathes are supposed to be unlikely by nature, otherwise we wouldn't call it an error situation ;-) > People normally add comments to code which is problematic to understand > or is somehow tricky, no so obvious or document how should code behave. Yes, but isn't this case so obvious that it doesn't need any documentation at all ? Is it so important to never ever forget that this particular path is a rare situation ? I might be wrong, but IMHO the likely()/unlikely() macros have been introduced for optimization (better pipeline optimization for the frequent cases). > People first need to understand that static code analysis cannot work > 100% reliably and always is needed to properly review changes. Yes, but we see that this particular case is coming up again and again, from different people, who can't know the background of this (nobody can read the whole lkml and remember everything). This could be prevented by adding a comment on that line, but then the macro call just for documentation wouldn't be necessary anymore. > And if the only reason for this change is to satisfy some static code > analysis program, Actually, it's more for people who're using the tools for the purpose of tidying up the code and find potential problems. They need some information to act on properly, which is currently missing ... > would not it better to teach this program to no > generate such false-positive results? Okay, but how to do that practically ? There's no information in the code which neither some tool nor any human could recognize this false alarm. That information is just in your brain, now also in mine, and the other folks who previously proposed the same change (assuming they still recall it after years). Blacklisting doesn't seem to be a good idea in the long run. Once going that route, such blacklists grow quickly and get hard to maintain (we now have two entirely separately maintained data sources that need to be synced manually, when something changes). I believe the code should be kinda self-documenting, so the reader shouldn't need any detailed special knowledge to understand it. IMHO, when such discussions on actual implementation details come up, the code isn't expressive enough. How can we improve the situation ? Shall we consider introducing yet another macro (explicitly different name) for stating/documenting such cases ? --mtx
Hi, On Wed, Aug 21, 2019 at 01:37:09PM +0200, Enrico Weigelt, metux IT consult wrote: > On 20.08.19 16:22, Pali Rohár wrote: > > Hi, > > > > In that case, wouldn't a comment be more suitable for that ? > > > > And why to add comment if current state of code is more-readable and > > does not need it? > > Readability is probably a bit subjective :p > > With ongoing efforts of automatically identifying redundant code pathes, > the current situation causes the same discussion coming up over and over > again. Sooner or later somebody might get the idea to add a comment on > that line, that it's exactly as intented :o > > OTOH, I'm unsure whether it's important to document that is particular > error path is unlikely, while we don't do it in thousands of other > places. IMHO, error pathes are supposed to be unlikely by nature, > otherwise we wouldn't call it an error situation ;-) > > > People normally add comments to code which is problematic to understand > > or is somehow tricky, no so obvious or document how should code behave. > > Yes, but isn't this case so obvious that it doesn't need any > documentation at all ? Is it so important to never ever forget that this > particular path is a rare situation ? Because if I see "if (IS_ERR(...))" in an interrupt path I will try to see if it can be optimized out, but in this particular case we document it with explicit "unlikely" and I know that I do not need to bother. The fact that there is unlikely in IS_ERR is an implementation detail. It may be gone tomorrow. I do not want to have to remember all implementation details of all kernel APIs and readjust the code all the time as they are change underneath me. Thanks.
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 34700ed..ed16614 100644 --- a/drivers/input/mouse/alps.c +++ b/drivers/input/mouse/alps.c @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse, /* On V2 devices the DualPoint Stick reports bare packets */ dev = priv->dev2; dev2 = psmouse->dev; - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) { + } else if (IS_ERR_OR_NULL(priv->dev3)) { /* Register dev3 mouse if we received PS/2 packet first time */ if (!IS_ERR(priv->dev3)) psmouse_queue_work(psmouse, &priv->dev3_register_work,