Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741503 > unrolled thread

Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison

Started byDan Carpenter <dan.carpenter@oracle.com>
First post2017-09-28 14:50 +0200
Last post2017-09-28 15:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Dan Carpenter <dan.carpenter@oracle.com> - 2017-09-28 14:50 +0200
    Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Dan Carpenter <dan.carpenter@oracle.com> - 2017-09-28 15:00 +0200
    RE: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com> - 2017-09-28 15:10 +0200
      Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org> - 2017-09-28 15:20 +0200
        RE: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com> - 2017-09-28 15:30 +0200

#1741503 — Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-09-28 14:50 +0200
SubjectRe: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
Message-ID<uuGqm-C7-13@gated-at.bofh.it>
On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> index f809682..26922fc 100644
> --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> @@ -76,7 +76,7 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
>  	if (d)
>  		return d;
>  
> -	if (unlikely(cpu >= num_possible_cpus()))
> +	if (unlikely(cpu >= (int)num_possible_cpus()))


Drivers shouldn't use likely/unlikley.  Please write it more explicitly
like this:

	if (cpu != -1 && cpu >= num_possible_cpus())
		return NULL;

Same for the other one as well.

regards,
dan carpenter

[toc] | [next] | [standalone]

#1741511

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-09-28 15:00 +0200
Message-ID<uuGA1-Fm-21@gated-at.bofh.it>
In reply to#1741503
On Thu, Sep 28, 2017 at 03:48:36PM +0300, Dan Carpenter wrote:
> On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > index f809682..26922fc 100644
> > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > @@ -76,7 +76,7 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
> >  	if (d)
> >  		return d;
> >  
> > -	if (unlikely(cpu >= num_possible_cpus()))
> > +	if (unlikely(cpu >= (int)num_possible_cpus()))
> 
> 
> Drivers shouldn't use likely/unlikley.  Please write it more explicitly
> like this:
> 
> 	if (cpu != -1 && cpu >= num_possible_cpus())

This would probably be more readable as a define.

	if (cpu != DPAA_ANY_CPU && cpu >= num_possible_cpus())
		return NULL;

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1741524

FromRuxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
Date2017-09-28 15:10 +0200
Message-ID<uuGJI-XA-31@gated-at.bofh.it>
In reply to#1741503
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, September 28, 2017 3:49 PM
> To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> arnd@arndb.de; stuyoder@gmail.com; Roy Pledge <roy.pledge@nxp.com>;
> linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>
> Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> 
> On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > index f809682..26922fc 100644
> > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> *service_select_by_cpu(struct dpaa2_io *d,
> >  	if (d)
> >  		return d;
> >
> > -	if (unlikely(cpu >= num_possible_cpus()))
> > +	if (unlikely(cpu >= (int)num_possible_cpus()))
> 
> 
> Drivers shouldn't use likely/unlikley.
 
I was under the impression it's ok to use them on hotpath
(and while not entirely obvious, this function is called on
other drivers' hotpath).

> Please write it more explicitly like this:
> 
> 	if (cpu != -1 && cpu >= num_possible_cpus())
> 		return NULL;
> 
> Same for the other one as well.

Will rewrite as you suggested in the second email and send a v2.

Thanks,
Ioana

[toc] | [prev] | [next] | [standalone]

#1741528

From"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Date2017-09-28 15:20 +0200
Message-ID<uuGTo-10S-13@gated-at.bofh.it>
In reply to#1741524
On Thu, Sep 28, 2017 at 01:07:48PM +0000, Ruxandra Ioana Radulescu wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Thursday, September 28, 2017 3:49 PM
> > To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> > Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> > arnd@arndb.de; stuyoder@gmail.com; Roy Pledge <roy.pledge@nxp.com>;
> > linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> > <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> > <laurentiu.tudor@nxp.com>
> > Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> > 
> > On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > index f809682..26922fc 100644
> > > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> > *service_select_by_cpu(struct dpaa2_io *d,
> > >  	if (d)
> > >  		return d;
> > >
> > > -	if (unlikely(cpu >= num_possible_cpus()))
> > > +	if (unlikely(cpu >= (int)num_possible_cpus()))
> > 
> > 
> > Drivers shouldn't use likely/unlikley.
>  
> I was under the impression it's ok to use them on hotpath
> (and while not entirely obvious, this function is called on
> other drivers' hotpath).

Only use it if you can measure the difference.  If you can not, then do
not use it as the compiler and the CPU will guess it better than you
will.

This has been proven many times, something like 80% of our
likely/unlikely usage in the kernel is wrong because of this, see the
work from Andi Kleen many years ago in this area.

So please remove it.  Unless you can prove it matters, and if so,
document that.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1741557

FromRuxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
Date2017-09-28 15:30 +0200
Message-ID<uuH34-13Q-15@gated-at.bofh.it>
In reply to#1741528
> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, September 28, 2017 4:18 PM
> To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>;
> devel@driverdev.osuosl.org; arnd@arndb.de; stuyoder@gmail.com; Roy
> Pledge <roy.pledge@nxp.com>; linux-kernel@vger.kernel.org;
> agraf@suse.de; Bogdan Purcareata <bogdan.purcareata@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> 
> On Thu, Sep 28, 2017 at 01:07:48PM +0000, Ruxandra Ioana Radulescu wrote:
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Thursday, September 28, 2017 3:49 PM
> > > To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> > > Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> > > arnd@arndb.de; stuyoder@gmail.com; Roy Pledge
> <roy.pledge@nxp.com>;
> > > linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> > > <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> > > <laurentiu.tudor@nxp.com>
> > > Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> > >
> > > On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > > > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > index f809682..26922fc 100644
> > > > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> > > *service_select_by_cpu(struct dpaa2_io *d,
> > > >  	if (d)
> > > >  		return d;
> > > >
> > > > -	if (unlikely(cpu >= num_possible_cpus()))
> > > > +	if (unlikely(cpu >= (int)num_possible_cpus()))
> > >
> > >
> > > Drivers shouldn't use likely/unlikley.
> >
> > I was under the impression it's ok to use them on hotpath
> > (and while not entirely obvious, this function is called on
> > other drivers' hotpath).
> 
> Only use it if you can measure the difference.  If you can not, then do
> not use it as the compiler and the CPU will guess it better than you
> will.
> 
> This has been proven many times, something like 80% of our
> likely/unlikely usage in the kernel is wrong because of this, see the
> work from Andi Kleen many years ago in this area.
> 
> So please remove it.  Unless you can prove it matters, and if so,
> document that.
 
Greg, thanks for the explanation. Will remove it in v2.

Ioana

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web