Input: alps: Drop unlikely before IS_ERR(_OR_NULL)
diff mbox series

Message ID 20190605142428.84784-5-wangkefeng.wang@huawei.com
State New
Headers show
Series
  • Input: alps: Drop unlikely before IS_ERR(_OR_NULL)
Related show

Commit Message

Kefeng Wang June 5, 2019, 2:24 p.m. UTC
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(-)

Comments

Pali Rohár June 5, 2019, 2:42 p.m. UTC | #1
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,
Kefeng Wang June 6, 2019, 1:08 a.m. UTC | #2
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,
Joe Perches June 6, 2019, 2:28 a.m. UTC | #3
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,
Dmitry Torokhov June 12, 2019, 12:59 a.m. UTC | #4
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.
Pali Rohár June 12, 2019, 7:14 a.m. UTC | #5
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.
Joe Perches June 12, 2019, 8:35 a.m. UTC | #6
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.

Patch
diff mbox series

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,