| Message ID | 20190605142428.84784-5-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: alps: Drop unlikely before IS_ERR(_OR_NULL) | expand |
On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: > IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, > so no need to do that again from its callers. Drop it. Hi! I already reviewed this patch and rejected it, see: https://patchwork.kernel.org/patch/10817475/ > Cc: "Pali Rohár" <pali.rohar@gmail.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: linux-input@vger.kernel.org > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > 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 0a6f7ca883e7..791ef0f826c5 100644 > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -1478,7 +1478,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,
On 2019/6/5 22:42, Pali Rohár wrote: > On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: >> IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, >> so no need to do that again from its callers. Drop it. > Hi! I already reviewed this patch and rejected it, see: > https://patchwork.kernel.org/patch/10817475/ OK, please ignore it. >> Cc: "Pali Rohár" <pali.rohar@gmail.com> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: linux-input@vger.kernel.org >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> 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 0a6f7ca883e7..791ef0f826c5 100644 >> --- a/drivers/input/mouse/alps.c >> +++ b/drivers/input/mouse/alps.c >> @@ -1478,7 +1478,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,
On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote: > On 2019/6/5 22:42, Pali Rohár wrote: > > On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: > > > IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, > > > so no need to do that again from its callers. Drop it. > > Hi! I already reviewed this patch and rejected it, see: > > https://patchwork.kernel.org/patch/10817475/ > OK, please ignore it. I think the stated reason of better readability isn't particularly sensible as the object code produced is actually slightly larger. x86-64 defconfig (gcc 8.3.0) $ size drivers/input/mouse/alps.o* text data bss dec hex filename 29416 56 0 29472 7320 drivers/input/mouse/alps.o.new 29432 56 0 29488 7330 drivers/input/mouse/alps.o.old Also if this unlikely is _really_ useful, perhaps the !IS_ERR immediately after could also use likely as the test seems only done for an OOM condition. > > > Cc: "Pali Rohár" <pali.rohar@gmail.com> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Cc: linux-input@vger.kernel.org > > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > > --- > > > 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 0a6f7ca883e7..791ef0f826c5 100644 > > > --- a/drivers/input/mouse/alps.c > > > +++ b/drivers/input/mouse/alps.c > > > @@ -1478,7 +1478,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,
Hi Joe, On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote: > On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote: > > On 2019/6/5 22:42, Pali Rohár wrote: > > > On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: > > > > IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, > > > > so no need to do that again from its callers. Drop it. > > > Hi! I already reviewed this patch and rejected it, see: > > > https://patchwork.kernel.org/patch/10817475/ > > OK, please ignore it. > > I think the stated reason of better readability isn't > particularly sensible as the object code produced is > actually slightly larger. > > x86-64 defconfig (gcc 8.3.0) > > $ size drivers/input/mouse/alps.o* > text data bss dec hex filename > 29416 56 0 29472 7320 drivers/input/mouse/alps.o.new > 29432 56 0 29488 7330 drivers/input/mouse/alps.o.old If gcc produces worse code for double unlikely, you should probably report it to gcc folks, no? Or double unlikely turns into likely? > > Also if this unlikely is _really_ useful, perhaps the > !IS_ERR immediately after could also use likely as the > test seems only done for an OOM condition. No, once you take the IS_ERR_OR_NULL(priv->dev3) == true branch it stops being hot path and additional annotations are completely unneeded. And if we failed to create and register priv->dev3 device - that's an error and system is degraded. Can't do much here. Thanks.
On Tuesday 11 June 2019 17:59:13 Dmitry Torokhov wrote: > Hi Joe, > > On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote: > > On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote: > > > On 2019/6/5 22:42, Pali Rohár wrote: > > > > On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: > > > > > IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, > > > > > so no need to do that again from its callers. Drop it. > > > > Hi! I already reviewed this patch and rejected it, see: > > > > https://patchwork.kernel.org/patch/10817475/ > > > OK, please ignore it. > > > > I think the stated reason of better readability isn't > > particularly sensible as the object code produced is > > actually slightly larger. > > > > x86-64 defconfig (gcc 8.3.0) > > > > $ size drivers/input/mouse/alps.o* > > text data bss dec hex filename > > 29416 56 0 29472 7320 drivers/input/mouse/alps.o.new > > 29432 56 0 29488 7330 drivers/input/mouse/alps.o.old > > If gcc produces worse code for double unlikely, you should probably > report it to gcc folks, no? Or double unlikely turns into likely? Is measured size of stripped or unstripped binary? Plus with or without debug symbols? Double unlikely version should have more debug symbols and therefore also larger size. But if unstripped version with double unlikely is larger then it is for sure compiler bug.
On Wed, 2019-06-12 at 09:14 +0200, Pali Rohár wrote: > On Tuesday 11 June 2019 17:59:13 Dmitry Torokhov wrote: > > Hi Joe, > > > > On Wed, Jun 05, 2019 at 07:28:53PM -0700, Joe Perches wrote: > > > On Thu, 2019-06-06 at 09:08 +0800, Kefeng Wang wrote: > > > > On 2019/6/5 22:42, Pali Rohár wrote: > > > > > On Wednesday 05 June 2019 22:24:28 Kefeng Wang wrote: > > > > > > IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, > > > > > > so no need to do that again from its callers. Drop it. > > > > > Hi! I already reviewed this patch and rejected it, see: > > > > > https://patchwork.kernel.org/patch/10817475/ > > > > OK, please ignore it. > > > > > > I think the stated reason of better readability isn't > > > particularly sensible as the object code produced is > > > actually slightly larger. > > > > > > x86-64 defconfig (gcc 8.3.0) > > > > > > $ size drivers/input/mouse/alps.o* > > > text data bss dec hex filename > > > 29416 56 0 29472 7320 drivers/input/mouse/alps.o.new > > > 29432 56 0 29488 7330 drivers/input/mouse/alps.o.old > > > > If gcc produces worse code for double unlikely, you should probably > > report it to gcc folks, no? Or double unlikely turns into likely? > > Is measured size of stripped or unstripped binary? Plus with or without > debug symbols? Double unlikely version should have more debug symbols > and therefore also larger size. > > But if unstripped version with double unlikely is larger then it is for > sure compiler bug. defconfig so no debug symbols. It's not necessarily a gcc bug as gcc doesn't guarantee compiler repeatability.
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c index 0a6f7ca883e7..791ef0f826c5 100644 --- a/drivers/input/mouse/alps.c +++ b/drivers/input/mouse/alps.c @@ -1478,7 +1478,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,
IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag, so no need to do that again from its callers. Drop it. Cc: "Pali Rohár" <pali.rohar@gmail.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: linux-input@vger.kernel.org Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- drivers/input/mouse/alps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)