Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
Srikar Dronamraju writes: > * Michael Ellerman [2020-07-31 17:45:37]: > >> Srikar Dronamraju writes: >> > 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. >> >> PeterZ asked for some overview of what you're doing and why, you >> responded to his mail, but I was expecting to see that text incorporated >> here somewhere. >> > > Okay, do you want that as part of the code or documentation dir or the > changelog? I guess a comment is best, as that's most likely to be seen by people looking at the code in future. A little bit of overview in the change log is also good. >> He also asked for some comments, which I would also like to see. >> >> >> I'm also not clear why we want to rename it to "bigcore", that's not a >> commonly understood term, I don't think it's clear to new readers what >> it means. >> >> Leaving it as the shared cache domain, and having a comment mentioning >> that "bigcores" share a cache, would be clearer I think. >> > > Today, Shared cache is equal to Big Core. However in not too distant future, > Shared cache domain and Big Core may not be the same. For example lets > assume that L2 cache were to Shrink per small core with the firmware > exposing the core as a bigcore. Then with the current design, we have a SMT > == SHARED CACHE, and a DIE. We would not have any domain at the publicised 8 > thread level. Keeping the Bigcore as a domain and mapping the shared > cache, (I am resetting the domain name as CACHE if BIGCORE==SHARED_CACHE), > helps us in this scenario. Yeah OK. In that scenario it's not really clear what the 8 thread level domain expresses, if the two "halves" of the bigcore have separate L2s. But presumably there is still some benefit to exposing it. cheers
Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
* Michael Ellerman [2020-07-31 17:45:37]: > Srikar Dronamraju writes: > > 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. > > PeterZ asked for some overview of what you're doing and why, you > responded to his mail, but I was expecting to see that text incorporated > here somewhere. > Okay, do you want that as part of the code or documentation dir or the changelog? > He also asked for some comments, which I would also like to see. > > > I'm also not clear why we want to rename it to "bigcore", that's not a > commonly understood term, I don't think it's clear to new readers what > it means. > > Leaving it as the shared cache domain, and having a comment mentioning > that "bigcores" share a cache, would be clearer I think. > Today, Shared cache is equal to Big Core. However in not too distant future, Shared cache domain and Big Core may not be the same. For example lets assume that L2 cache were to Shrink per small core with the firmware exposing the core as a bigcore. Then with the current design, we have a SMT == SHARED CACHE, and a DIE. We would not have any domain at the publicised 8 thread level. Keeping the Bigcore as a domain and mapping the shared cache, (I am resetting the domain name as CACHE if BIGCORE==SHARED_CACHE), helps us in this scenario. > cheers > -- Thanks and Regards Srikar Dronamraju
Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
Srikar Dronamraju writes: > 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. PeterZ asked for some overview of what you're doing and why, you responded to his mail, but I was expecting to see that text incorporated here somewhere. He also asked for some comments, which I would also like to see. I'm also not clear why we want to rename it to "bigcore", that's not a commonly understood term, I don't think it's clear to new readers what it means. Leaving it as the shared cache domain, and having a comment mentioning that "bigcores" share a cache, would be clearer I think. cheers > Cc: linuxppc-dev > Cc: LKML > Cc: Michael Ellerman > Cc: Nicholas Piggin > Cc: Anton Blanchard > Cc: Oliver O'Halloran > Cc: Nathan Lynch > Cc: Michael Neuling > Cc: Gautham R Shenoy > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Valentin Schneider > Cc: Jordan Niethe > Signed-off-by: Srikar Dronamraju > --- > Changelog v1 -> v2: > Moved shared_cache topology fixup to fixup_topology (Gautham) > > arch/powerpc/kernel/smp.c | 48 +++ > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index d997c7411664..3c5ccf6d2b1c 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, > +}; > + > #define MAX_THREAD_LIST_SIZE 8 > #define THREAD_GROUP_SHARE_L1 1 > struct thread_groups { > @@ -851,13 +859,7 @@ static int powerpc_shared_cache_flags(void) > */ > static const struct cpumask *shared_cache_mask(int cpu) > { > - if (shared_caches) > - return cpu_l2_cache_mask(cpu); > - > - if (has_big_cores) > - return cpu_smallcore_mask(cpu); > - > - return per_cpu(cpu_sibling_map, cpu); > + return per_cpu(cpu_l2_cache_map, cpu); > } > > #ifdef CONFIG_SCHED_SMT > @@ -867,11 +869,16 @@ static const struct cpumask *smallcore_smt_mask(int cpu) > } > #endif > > +static const struct cpumask *cpu_bigcore_mask(int cpu) > +{ > + return per_cpu(cpu_sibling_map, cpu); > +} > + > static struct sched_domain_topology_level powerpc_topology[] = { > #ifdef CONFIG_SCHED_SMT > { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) }, > #endif > - { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) }, > + { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) }, > { cpu_cpu_mask, SD_INIT_NAME(DIE) }, > { NULL, }, > }; > @@ -1311,7 +1318,6 @@ static void add_cpu_to_masks(int cpu) > void start_secondary(void *unused) > { > unsigned int cpu = smp_processor_id(); > - struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask; > > mmgrab(_mm); > current->active_mm = _mm; > @@ -1337,14 +1343,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; > + } > > set_numa_node(numa_cpu_lookup_table[cpu]); > set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu])); > @@ -1375,9 +1387,17 @@ static void fixup_topology(void) > #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 > + if (shared_caches) { > + pr_info("Using shared cache scheduler topology\n"); > + powerpc_topology[bigcore_idx].mask = shared_cache_mask; > + powerpc_topology[bigcore_idx].sd_flags = > powerpc_shared_cache_flags; > +#ifdef CONFIG_SCHED_DEBUG > + powerpc_topology[bigcore_idx].name = "CACHE"; > +#endif > +
Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
On Mon, Jul 27, 2020 at 11:02:26AM +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 > Cc: LKML > Cc: Michael Ellerman > Cc: Nicholas Piggin > Cc: Anton Blanchard > Cc: Oliver O'Halloran > Cc: Nathan Lynch > Cc: Michael Neuling > Cc: Gautham R Shenoy > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Valentin Schneider > Cc: Jordan Niethe > Signed-off-by: Srikar Dronamraju Reviewed-by: Gautham R. Shenoy > --- > Changelog v1 -> v2: > Moved shared_cache topology fixup to fixup_topology (Gautham) > > arch/powerpc/kernel/smp.c | 48 +++ > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index d997c7411664..3c5ccf6d2b1c 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, > +}; > + > #define MAX_THREAD_LIST_SIZE 8 > #define THREAD_GROUP_SHARE_L1 1 > struct thread_groups { > @@ -851,13 +859,7 @@ static int powerpc_shared_cache_flags(void) > */ > static const struct cpumask *shared_cache_mask(int cpu) > { > - if (shared_caches) > - return cpu_l2_cache_mask(cpu); > - > - if (has_big_cores) > - return cpu_smallcore_mask(cpu); > - > - return per_cpu(cpu_sibling_map, cpu); > + return per_cpu(cpu_l2_cache_map, cpu); > } > > #ifdef CONFIG_SCHED_SMT > @@ -867,11 +869,16 @@ static const struct cpumask *smallcore_smt_mask(int cpu) > } > #endif > > +static const struct cpumask *cpu_bigcore_mask(int cpu) > +{ > + return per_cpu(cpu_sibling_map, cpu); > +} > + > static struct sched_domain_topology_level powerpc_topology[] = { > #ifdef CONFIG_SCHED_SMT > { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) }, > #endif > - { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) }, > + { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) }, > { cpu_cpu_mask, SD_INIT_NAME(DIE) }, > { NULL, }, > }; > @@ -1311,7 +1318,6 @@ static void add_cpu_to_masks(int cpu) > void start_secondary(void *unused) > { > unsigned int cpu = smp_processor_id(); > - struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask; > > mmgrab(_mm); > current->active_mm = _mm; > @@ -1337,14 +1343,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; > + } > > set_numa_node(numa_cpu_lookup_table[cpu]); > set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu])); > @@ -1375,9 +1387,17 @@ static void fixup_topology(void) > #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 > + if (shared_caches) { > + pr_info("Using shared cache scheduler topology\n"); > + powerpc_topology[bigcore_idx].mask = shared_cache_mask; > + powerpc_topology[bigcore_idx].sd_flags = > powerpc_shared_cache_flags; > +#ifdef CONFIG_SCHED_DEBUG > + powerpc_topology[bigcore_idx].name = "CACHE"; > +#endif > + } > } > > void __init smp_cpus_done(unsigned int max_cpus) > -- > 2.17.1 >
[PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
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 Cc: LKML Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Anton Blanchard Cc: Oliver O'Halloran Cc: Nathan Lynch Cc: Michael Neuling Cc: Gautham R Shenoy Cc: Ingo Molnar Cc: Peter Zijlstra Cc: Valentin Schneider Cc: Jordan Niethe Signed-off-by: Srikar Dronamraju --- Changelog v1 -> v2: Moved shared_cache topology fixup to fixup_topology (Gautham) arch/powerpc/kernel/smp.c | 48 +++ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index d997c7411664..3c5ccf6d2b1c 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, +}; + #define MAX_THREAD_LIST_SIZE 8 #define THREAD_GROUP_SHARE_L1 1 struct thread_groups { @@ -851,13 +859,7 @@ static int powerpc_shared_cache_flags(void) */ static const struct cpumask *shared_cache_mask(int cpu) { - if (shared_caches) - return cpu_l2_cache_mask(cpu); - - if (has_big_cores) - return cpu_smallcore_mask(cpu); - - return per_cpu(cpu_sibling_map, cpu); + return per_cpu(cpu_l2_cache_map, cpu); } #ifdef CONFIG_SCHED_SMT @@ -867,11 +869,16 @@ static const struct cpumask *smallcore_smt_mask(int cpu) } #endif +static const struct cpumask *cpu_bigcore_mask(int cpu) +{ + return per_cpu(cpu_sibling_map, cpu); +} + static struct sched_domain_topology_level powerpc_topology[] = { #ifdef CONFIG_SCHED_SMT { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) }, #endif - { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) }, + { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) }, { cpu_cpu_mask, SD_INIT_NAME(DIE) }, { NULL, }, }; @@ -1311,7 +1318,6 @@ static void add_cpu_to_masks(int cpu) void start_secondary(void *unused) { unsigned int cpu = smp_processor_id(); - struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask; mmgrab(_mm); current->active_mm = _mm; @@ -1337,14 +1343,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; + } set_numa_node(numa_cpu_lookup_table[cpu]); set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu])); @@ -1375,9 +1387,17 @@ static void fixup_topology(void) #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 + if (shared_caches) { + pr_info("Using shared cache scheduler topology\n"); + powerpc_topology[bigcore_idx].mask = shared_cache_mask; + powerpc_topology[bigcore_idx].sd_flags = powerpc_shared_cache_flags; +#ifdef CONFIG_SCHED_DEBUG + powerpc_topology[bigcore_idx].name = "CACHE"; +#endif + } } void __init smp_cpus_done(unsigned int max_cpus) -- 2.17.1
[PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
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 Cc: LKML Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Anton Blanchard Cc: Oliver O'Halloran Cc: Nathan Lynch Cc: Michael Neuling Cc: Gautham R Shenoy Cc: Ingo Molnar Cc: Peter Zijlstra Cc: Valentin Schneider Cc: Jordan Niethe Signed-off-by: Srikar Dronamraju --- Changelog v1 -> v2: Moved shared_cache topology fixup to fixup_topology (Gautham) 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 d997c7411664..3c5ccf6d2b1c 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, +}; + #define MAX_THREAD_LIST_SIZE 8 #define THREAD_GROUP_SHARE_L1 1 struct thread_groups { @@ -851,13 +859,7 @@ static int powerpc_shared_cache_flags(void) */ static const struct cpumask *shared_cache_mask(int cpu) { - if (shared_caches) - return cpu_l2_cache_mask(cpu); - - if (has_big_cores) - return cpu_smallcore_mask(cpu); - - return per_cpu(cpu_sibling_map, cpu); + return per_cpu(cpu_l2_cache_map, cpu); } #ifdef CONFIG_SCHED_SMT @@ -867,11 +869,16 @@ static const struct cpumask *smallcore_smt_mask(int cpu) } #endif +static const struct cpumask *cpu_bigcore_mask(int cpu) +{ + return per_cpu(cpu_sibling_map, cpu); +} + static struct sched_domain_topology_level powerpc_topology[] = { #ifdef CONFIG_SCHED_SMT { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) }, #endif - { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) }, + { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) }, { cpu_cpu_mask, SD_INIT_NAME(DIE) }, { NULL, }, }; @@ -1311,7 +1318,6 @@ static void add_cpu_to_masks(int cpu) void start_secondary(void *unused) { unsigned int cpu = smp_processor_id(); - struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask; mmgrab(_mm); current->active_mm = _mm; @@ -1337,14 +1343,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; + } set_numa_node(numa_cpu_lookup_table[cpu]); set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu])); @@ -1375,9 +1387,17 @@ static void fixup_topology(void) #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 + if (shared_caches) { + pr_info("Using shared cache scheduler topology\n"); + powerpc_topology[bigcore_idx].mask = shared_cache_mask; + powerpc_topology[bigcore_idx].sd_flags = powerpc_shared_cache_flags; +#ifdef CONFIG_SCHED_DEBUG + powerpc_topology[bigcore_idx].name = "CACHE"; +#endif + } } void __init smp_cpus_done(unsigned int max_cpus) -- 2.17.1