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

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

Hi Srikar,


On Mon, Jul 20, 2020 at 11:18:16AM +0530, Srikar Dronamraju wrote:
> * 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.

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.



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