Re: [PATCH 05/11] powerpc/smp: Dont assume l2-cache to be superset of sibling

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

* Oliver O'Halloran <oohall@gmail.com> [2020-07-14 15:40:09]:

> On Tue, Jul 14, 2020 at 2:45 PM Srikar Dronamraju
> <srikar@linux.vnet.ibm.com> wrote:
> >
> > Current code assumes that cpumask of cpus sharing a l2-cache mask will
> > always be a superset of cpu_sibling_mask.
> >
> > Lets stop that assumption.
> 
> It's been a while since I looked, but I'm pretty sure the scheduler
> requires child domains to be subsets of their parents. Why is this
> necessary or a good idea?

Thanks for looking into the patches.

Yes the scheduler requires the child domains to be subsets of their parents.

Current code assumes that the l2_cache is always a superset of sibling mask.
However there may be processors in future whose sibling mask maynot be a
superset. 

Lets for example we have a chip with 16 threads and 8 threads share
l2-cache, i.e 8 threads are acting like a small core and 16 threads are
acting like a big core. Then the assumption that l2-cache mask is a superset
of cpu_sibling mask would be wrong.

> 
> > 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/kernel/smp.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 7d430fc536cc..875f57e41355 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1198,6 +1198,7 @@ static bool update_mask_by_l2(int cpu, struct cpumask *(*mask_fn)(int))
> >         struct device_node *l2_cache, *np;
> >         int i;
> >
> > +       cpumask_set_cpu(cpu, mask_fn(cpu));
> 
> ?

At the time the cpumasks are updated, the cpu is not yet part of the
cpu_online_mask. So when we online/offline the cpus, the masks will end up
not having itself and causes the scheduler to bork.

Previously (as we can note in code below thats removed), we were doing as
part of updating all cpus that were part of the cpu_sibling_mask before
calling update_mask_by_l2.

> 
> >         l2_cache = cpu_to_l2cache(cpu);
> >         if (!l2_cache)
> >                 return false;
> > @@ -1284,29 +1285,30 @@ static void add_cpu_to_masks(int cpu)
> >          * add it to it's own thread sibling mask.
> >          */
> >         cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
> > +       cpumask_set_cpu(cpu, cpu_core_mask(cpu));
> >
> >         for (i = first_thread; i < first_thread + threads_per_core; i++)
> >                 if (cpu_online(i))
> >                         set_cpus_related(i, cpu, cpu_sibling_mask);
> >
> >         add_cpu_to_smallcore_masks(cpu);
> > -       /*
> > -        * Copy the thread sibling mask into the cache sibling mask
> > -        * and mark any CPUs that share an L2 with this CPU.
> > -        */
> > -       for_each_cpu(i, cpu_sibling_mask(cpu))
> > -               set_cpus_related(cpu, i, cpu_l2_cache_mask);

I am referring to this code above. This would have updated the self in its
cpumask. For the rest of the cpus in the cpu_sibling_mask, they get updated
correctly in the update_mask_by_l2.

> >         update_mask_by_l2(cpu, cpu_l2_cache_mask);
> >
> > -       /*
> > -        * Copy the cache sibling mask into core sibling mask and mark
> > -        * any CPUs on the same chip as this CPU.
> > -        */

-- 
Thanks and Regards
Srikar Dronamraju