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

[Srikar Dronamraju]

* Gautham R Shenoy <ego at linux.vnet.ibm.com> [2020-07-17 11:30:11]:

> Hi Srikar,
> 
> On Tue, Jul 14, 2020 at 10:06:18AM +0530, Srikar Dronamraju 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.
> > 
> > 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/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));
> 
> It would be good to comment why do we need to do set the CPU in the
> l2-mask if we don't have a l2cache domain.
> 

Good Catch, 
We should move this after the cpu_to_l2cache.

> >  	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);
> >  	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.
> > -	 */
> > -	for_each_cpu(i, cpu_l2_cache_mask(cpu))
> > -		set_cpus_related(cpu, i, cpu_core_mask);
> > +	if (pkg_id == -1) {
> > +		struct cpumask *(*mask)(int) = cpu_sibling_mask;
> > +
> > +		/*
> > +		 * Copy the sibling mask into core sibling mask and
> > +		 * mark any CPUs on the same chip as this CPU.
> > +		 */
> > +		if (shared_caches)
> > +			mask = cpu_l2_cache_mask;
> > +
> 
> 
> Now that we decoupling the containment relationship between
> sibling_mask and l2-cache mask, should we set all the CPUs that are
> both in cpu_sibling_mask(cpu) as well as cpu_l2_mask(cpu) in
> cpu_core_mask ? 
> 

Are you saying instead of setting this cpu in this cpu_core_mask, can we set
all the cpus in the mask in cpu_core_mask?
Currently we dont know if any of the cpus of the mask were already set or
not. Plus we need to anyway update cpumask of all other cpus to says they
are related. So setting a mask instead of cpu at a time will not change
anything for our side.

> > +		for_each_cpu(i, mask(cpu))
> > +			set_cpus_related(cpu, i, cpu_core_mask);
> > 
> > -	if (pkg_id == -1)
> >  		return;
> > +	}
> > 
> >  	for_each_cpu(i, cpu_online_mask)
> >  		if (get_physical_package_id(i) == pkg_id)
> > -- 
> > 2.17.1
> > 
> --
> Thanks and Regards
> gautham.

-- 
Thanks and Regards
Srikar Dronamraju