Hi Srikar,
On Mon, Jul 20, 2020 at 11:18:16AM +0530, Srikar Dronamraju wrote:
> * Gautham R Shenoy <e...@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 <micha...@au1.ibm.com>
> > > Cc: Nick Piggin <npig...@au1.ibm.com>
> > > Cc: Oliver OHalloran <olive...@au1.ibm.com>
> > > Cc: Nathan Lynch <nath...@linux.ibm.com>
> > > Cc: Michael Neuling <mi...@linux.ibm.com>
> > > Cc: Anton Blanchard <an...@au1.ibm.com>
> > > Cc: Gautham R Shenoy <e...@linux.vnet.ibm.com>
> > > Cc: Vaidyanathan Srinivasan <sva...@linux.ibm.com>
> > > Signed-off-by: Srikar Dronamraju <sri...@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
