Messages in this thread Patch in this message | | | Date | Wed, 22 Jul 2020 12:26:40 +0530 | From | Gautham R Shenoy <> | Subject | Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain |
| |
Hello Srikar,
On Tue, Jul 21, 2020 at 05:08:10PM +0530, Srikar Dronamraju wrote: > Currently "CACHE" domain happens to be the 2nd sched domain as per > powerpc_topology. This domain will collapse if cpumask of l2-cache is > same as SMT domain. However we could generalize this domain such that it > could mean either be a "CACHE" domain or a "BIGCORE" domain. > > While setting up the "CACHE" domain, check if shared_cache is already > set. > > Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org> > Cc: LKML <linux-kernel@vger.kernel.org> > Cc: Michael Ellerman <michaele@au1.ibm.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Valentin Schneider <valentin.schneider@arm.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> > Cc: Jordan Niethe <jniethe5@gmail.com> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > --- > Changelog v1 -> v2: > powerpc/smp: Generalize 2nd sched domain > Moved shared_cache topology fixup to fixup_topology (Gautham) >
Just one comment below.
> arch/powerpc/kernel/smp.c | 49 ++++++++++++++++++++++++++++----------- > 1 file changed, 35 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index 57468877499a..933ebdf97432 100644 > --- a/arch/powerpc/kernel/smp.c > +++ b/arch/powerpc/kernel/smp.c > @@ -85,6 +85,14 @@ EXPORT_PER_CPU_SYMBOL(cpu_l2_cache_map); > EXPORT_PER_CPU_SYMBOL(cpu_core_map); > EXPORT_SYMBOL_GPL(has_big_cores); > > +enum { > +#ifdef CONFIG_SCHED_SMT > + smt_idx, > +#endif > + bigcore_idx, > + die_idx, > +}; > +
[..snip..]
> @@ -1339,14 +1345,20 @@ void start_secondary(void *unused) > /* Update topology CPU masks */ > add_cpu_to_masks(cpu); > > - if (has_big_cores) > - sibling_mask = cpu_smallcore_mask; > /* > * Check for any shared caches. Note that this must be done on a > * per-core basis because one core in the pair might be disabled. > */ > - if (!cpumask_equal(cpu_l2_cache_mask(cpu), sibling_mask(cpu))) > - shared_caches = true; > + if (!shared_caches) { > + struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask; > + struct cpumask *mask = cpu_l2_cache_mask(cpu); > + > + if (has_big_cores) > + sibling_mask = cpu_smallcore_mask; > + > + if (cpumask_weight(mask) > cpumask_weight(sibling_mask(cpu))) > + shared_caches = true;
At the risk of repeating my comment to the v1 version of the patch, we have shared caches only l2_cache_mask(cpu) is a strict superset of sibling_mask(cpu).
"cpumask_weight(mask) > cpumask_weight(sibling_mask(cpu))" does not capture this.
Could we please use
if (!cpumask_equal(sibling_mask(cpu), mask) && cpumask_subset(sibling_mask(cpu), mask) { }
?
> + } > > set_numa_node(numa_cpu_lookup_table[cpu]); > set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu])); > @@ -1374,10 +1386,19 @@ int setup_profiling_timer(unsigned int multiplier) > > static void fixup_topology(void) > { > + if (shared_caches) { > + pr_info("Using shared cache scheduler topology\n"); > + powerpc_topology[bigcore_idx].mask = shared_cache_mask; > +#ifdef CONFIG_SCHED_DEBUG > + powerpc_topology[bigcore_idx].name = "CACHE"; > +#endif > + powerpc_topology[bigcore_idx].sd_flags = powerpc_shared_cache_flags; > + } > + > #ifdef CONFIG_SCHED_SMT > if (has_big_cores) { > pr_info("Big cores detected but using small core scheduling\n"); > - powerpc_topology[0].mask = smallcore_smt_mask; > + powerpc_topology[smt_idx].mask = smallcore_smt_mask; > } > #endif
Otherwise the patch looks good to me.
-- Thanks and Regards gautham.
| |