Re: [PATCH 10/11] powerpc/smp: Implement cpu_to_coregroup_id

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-17 13:56:53]:

> On Tue, Jul 14, 2020 at 10:06:23AM +0530, Srikar Dronamraju wrote:
> > Lookup the coregroup id from the associativity array.
> > 
> > If unable to detect the coregroup id, fallback on the core id.
> > This way, ensure sched_domain degenerates and an extra sched domain is
> > not created.
> > 
> > Ideally this function should have been implemented in
> > arch/powerpc/kernel/smp.c. However if its implemented in mm/numa.c, we
> > don't need to find the primary domain again.
> > 
> > If the device-tree mentions more than one coregroup, then kernel
> > implements only the last or the smallest coregroup, which currently
> > corresponds to the penultimate domain in the device-tree.
> > 
> > Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> > Cc: Michael Ellerman <michaele@au1.ibm.com>
> > Cc: Nick Piggin <npiggin@au1.ibm.com>
> > Cc: Oliver OHalloran <oliveroh@au1.ibm.com>
> > Cc: Nathan Lynch <nathanl@linux.ibm.com>
> > Cc: Michael Neuling <mikey@linux.ibm.com>
> > Cc: Anton Blanchard <anton@au1.ibm.com>
> > Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> > Cc: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/mm/numa.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > index d9ab9da85eab..4e85564ef62a 100644
> > --- a/arch/powerpc/mm/numa.c
> > +++ b/arch/powerpc/mm/numa.c
> > @@ -1697,6 +1697,23 @@ static const struct proc_ops topology_proc_ops = {
> > 
> >  int cpu_to_coregroup_id(int cpu)
> >  {
> > +	__be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
> > +	int index;
> > +
> 
> It would be good to have an assert here to ensure that we are calling
> this function only when coregroups are enabled.
> 
> Else, we may end up returning the penultimate index which maps to the
> chip-id.
> 

We have a check below exactly for the same reason. Please look below.

> 
> 
> > +	if (cpu < 0 || cpu > nr_cpu_ids)
> > +		return -1;
> > +
> > +	if (!firmware_has_feature(FW_FEATURE_VPHN))
> > +		goto out;
> > +
> > +	if (vphn_get_associativity(cpu, associativity))
> > +		goto out;
> > +
> > +	index = of_read_number(associativity, 1);
> > +	if ((index > min_common_depth + 1) && coregroup_enabled)
> > +		return of_read_number(&associativity[index - 1], 1);

See ^above.

index would be the all the domains in the associativity array, 
min_common_depth would be where the primary domain or the chip-id is
defined. So we are reading the penultimate domain if and only if the
min_common_depth isn't the primary domain aka chip-id. 

What other check /assertions can we add?


> > +
> > +out:
> >  	return cpu_to_core_id(cpu);
> >  }
> > 
> > -- 
> > 2.17.1
> > 

-- 
Thanks and Regards
Srikar Dronamraju