Re: [PATCH v4 1/2] powerpc: cleanup APIs for cpu/thread/core mappings

[Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> [2010-08-03 14:44:13]:

> On Thu, 2010-07-22 at 06:27 +0530, Vaidyanathan Srinivasan wrote:
> > These APIs take logical cpu number as input
> > Change cpu_first_thread_in_core() to cpu_leftmost_thread_sibling()
> > Change cpu_last_thread_in_core() to cpu_rightmost_thread_sibling()
> 
> I'm still not happy here.
> 
> I don't find leftmost/rightmost to be a "progress" from the previous
> naming. If you really want to change to avoid in_core() at the end, then
> call them first_thread_sibling() and last_thread_sibling().

Hi Ben,

The naming is based on our discussion during the first RFC:
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081610.html

I am fine with first_thread_sibling() name instead of
cpu_{left,right}most_thread_sibling().  The goal is to distinguish
between APIs returning logical cpu numbers and core indexes.

first_thread_sibling() implies that the parameter and return value are
logical cpu (thread) number.

I will change the name as per your suggestion and post an update.

> Then you change:
> 
> > -static inline int cpu_thread_to_core(int cpu)
> > -{
> > -	return cpu >> threads_shift;
> > -}
> 
>   .../... to:
> 
> > -/* Must be called when no change can occur to cpu_present_mask,
> > +/* Helper routines for cpu to core mapping */
> > +int cpu_core_of_thread(int cpu)
> > +{
> > +	return cpu >> threads_shift;
> > +}
> > +EXPORT_SYMBOL_GPL(cpu_core_of_thread);

The cpu_core_of_thread() name implies that the return type is a core index
and not a logical cpu number.

cpu_core_of_thread(5) = 1
cpu_first_thread_of_core(1) = 4

Do you think cpu_core_number_of_thread() will explain the return type
better?

> Any reason you are making it out of line other than gratuituous
> bloat ? :-)
> 
> > +int cpu_first_thread_of_core(int core)
> > +{
> > +	return core << threads_shift;
> > +}
> > +EXPORT_SYMBOL_GPL(cpu_first_thread_of_core);
> 
> Same.

The reason behind out of line is to avoid exporting threads_shift or
other variables like threads_per_core and have this API exported so
that modules can read them.  If we make these wrapper inline, then we
will have to export the variables themselves which is not a good idea.

Thanks for the review.  

--Vaidy