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

[Srikar Dronamraju]

* Gautham R Shenoy <ego at linux.vnet.ibm.com> [2020-07-20 14:40:25]:

> Hi Srikar,
> 
> 
> On Mon, Jul 20, 2020 at 11:18:16AM +0530, Srikar Dronamraju wrote:
> > * Gautham R Shenoy <ego at 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 at lists.ozlabs.org>
> > > > Cc: Michael Ellerman <michaele at au1.ibm.com>
> > > > Cc: Nick Piggin <npiggin at au1.ibm.com>
> > > > Cc: Oliver OHalloran <oliveroh at au1.ibm.com>
> > > > Cc: Nathan Lynch <nathanl at linux.ibm.com>
> > > > Cc: Michael Neuling <mikey at linux.ibm.com>
> > > > Cc: Anton Blanchard <anton at au1.ibm.com>
> > > > Cc: Gautham R Shenoy <ego at linux.vnet.ibm.com>
> > > > Cc: Vaidyanathan Srinivasan <svaidy at linux.ibm.com>
> > > > Signed-off-by: Srikar Dronamraju <srikar at 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.
> 
> I saw that. However, it would be better to assert within the function
> so that we don't call it from any other context without ascertaining
> first that core_groups are enabled. Or at least a comment in the
> function saying that we should call this only after ascertaining that
> core_groups are enabled.
> 
> 

Okay, I can move the coregroup_enabled check above.

> 
> > 
> > > 
> > > 
> > > > +	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

-- 
Thanks and Regards
Srikar Dronamraju