Messages in this thread | | | Date | Wed, 22 Jul 2020 13:09:36 +0530 | From | Srikar Dronamraju <> | Subject | Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain |
| |
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 12:26:40]:
> 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. > > @@ -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.
Why would it not? We are setting shared_caches if and only if l2_cache_mask is a strict superset of sibling/smallcore mask.
> Could we please use > > if (!cpumask_equal(sibling_mask(cpu), mask) && > cpumask_subset(sibling_mask(cpu), mask) { > } >
Scheduler mandates that two cpumasks for the same CPU would either have to be equal or one of them has to be a strict superset of the other. If not the scheduler would mark our domains as broken. That being the case, cpumask_weight will correctly capture what we are looking for. That said your condition is also correct but slightly heavier and doesn't provide us with any more information or correctness.
> > Otherwise the patch looks good to me. > > -- > Thanks and Regards > gautham.
-- Thanks and Regards Srikar Dronamraju
| |