Re: [PATCH 09/11] Powerpc/smp: Create coregroup domain
* Gautham R Shenoy [2020-07-17 13:49:26]:
> On Tue, Jul 14, 2020 at 10:06:22AM +0530, Srikar Dronamraju wrote:
> > Add percpu coregroup maps and masks to create coregroup domain.
> > If a coregroup doesn't exist, the coregroup domain will be degenerated
> > in favour of SMT/CACHE domain.
> >
> > Cc: linuxppc-dev
> > Cc: Michael Ellerman
> > Cc: Nick Piggin
> > Cc: Oliver OHalloran
> > Cc: Nathan Lynch
> > Cc: Michael Neuling
> > Cc: Anton Blanchard
> > Cc: Gautham R Shenoy
> > Cc: Vaidyanathan Srinivasan
> > Signed-off-by: Srikar Dronamraju
> > ---
> > arch/powerpc/include/asm/topology.h | 10
> > arch/powerpc/kernel/smp.c | 37 +
> > arch/powerpc/mm/numa.c | 5
> > 3 files changed, 52 insertions(+)
> >
> > @@ -950,6 +972,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> > cpumask_set_cpu(boot_cpuid, cpu_l2_cache_mask(boot_cpuid));
> > cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
> >
> > + if (has_coregroup_support())
> > + cpumask_set_cpu(boot_cpuid, cpu_coregroup_mask(boot_cpuid));
> > + else
> > + powerpc_topology[mc_idx].mask = cpu_bigcore_mask;
> > +
>
> The else part could be moved to the common function where we are
> modifying the other attributes of the topology array.
>
My intent is to make all the changes to the topology attributes in
smp_prepare_cpus. It makes more sense to change the attributes immediately
after we define / detect a particular topology change.
The only thing that is left out currently is shared_cache,
We should be able to detect shared_cache also around this time. Just that it
needs more cleanups. Once we do update the topology attributes even for
shared_cache here itself.
> > init_big_cores();
> > if (has_big_cores) {
> > cpumask_set_cpu(boot_cpuid,
> > @@ -1241,6 +1268,8 @@ static void remove_cpu_from_masks(int cpu)
> > set_cpus_unrelated(cpu, i, cpu_sibling_mask);
> > if (has_big_cores)
> > set_cpus_unrelated(cpu, i, cpu_smallcore_mask);
> > + if (has_coregroup_support())
> > + set_cpus_unrelated(cpu, i, cpu_coregroup_mask);
> > }
> > }
> > #endif
> > @@ -1301,6 +1330,14 @@ static void add_cpu_to_masks(int cpu)
> > add_cpu_to_smallcore_masks(cpu);
> > update_mask_by_l2(cpu, cpu_l2_cache_mask);
> >
> > + if (has_coregroup_support()) {
> > + cpumask_set_cpu(cpu, cpu_coregroup_mask(cpu));
> > + for_each_cpu(i, cpu_online_mask) {
> > + if (cpu_to_coregroup_id(cpu) == cpu_to_coregroup_id(i))
> > + set_cpus_related(cpu, i, cpu_coregroup_mask);
> > + }
> > + }
> > +
> > if (pkg_id == -1) {
> > struct cpumask *(*mask)(int) = cpu_sibling_mask;
> >
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > index a43eab455be4..d9ab9da85eab 100644
> > --- a/arch/powerpc/mm/numa.c
> > +++ b/arch/powerpc/mm/numa.c
> > @@ -1695,6 +1695,11 @@ static const struct proc_ops topology_proc_ops = {
> > .proc_release = single_release,
> > };
> >
> > +int cpu_to_coregroup_id(int cpu)
> > +{
> > + return cpu_to_core_id(cpu);
> > +}
>
>
> So, if has_coregroup_support() returns true, then since the core_group
> identification is currently done through the core-id, the
> coregroup_mask is going to be the same as the
> cpu_core_mask/cpu_cpu_mask. Thus, we will be degenerating the DIE
> domain. Right ? Instead we could have kept the core-group to be a
> single bigcore by default, so that those domains can get degenerated
> preserving the legacy SMT, DIE, NUMA hierarchy.
>
>
I think you have confused between cpu_core_mask and cpu_to_core_id.
cpu_to_core_id() returns a unique id for every SMT8 thread group.
If coregroup_support is true and the system doesn't support coregroup, then
We would not be degenerating the DIE but degenerating new MC domain only.
This is because the MC domain and the previous domain (SHARED_CACHE/BIGCORE/
SMT) would match the MC domain.
--
Thanks and Regards
Srikar Dronamraju
Re: [PATCH 09/11] Powerpc/smp: Create coregroup domain
On Fri, Jul 17, 2020 at 01:49:26PM +0530, Gautham R Shenoy wrote:
> > +int cpu_to_coregroup_id(int cpu)
> > +{
> > + return cpu_to_core_id(cpu);
> > +}
>
>
> So, if has_coregroup_support() returns true, then since the core_group
> identification is currently done through the core-id, the
> coregroup_mask is going to be the same as the
> cpu_core_mask/cpu_cpu_mask. Thus, we will be degenerating the DIE
> domain. Right ? Instead we could have kept the core-group to be a
> single bigcore by default, so that those domains can get degenerated
> preserving the legacy SMT, DIE, NUMA hierarchy.
Never mind, got confused with core_id and cpu_core_mask (which
corresponds to the cores of a chip). cpu_to_core_id() does exactly
what we have described above. Sorry for the noise.
I am ok with this patch, except that I would request if all the
changes to the topology structure be done within a single function for
ease of tracking it instead of distributing it all over the code.
>
>
>
>
> > +
> > static int topology_update_init(void)
> > {
> > start_topology_update();
> > --
> > 2.17.1
> >
Re: [PATCH 09/11] Powerpc/smp: Create coregroup domain
On Tue, Jul 14, 2020 at 10:06:22AM +0530, Srikar Dronamraju wrote:
> Add percpu coregroup maps and masks to create coregroup domain.
> If a coregroup doesn't exist, the coregroup domain will be degenerated
> in favour of SMT/CACHE domain.
>
> Cc: linuxppc-dev
> Cc: Michael Ellerman
> Cc: Nick Piggin
> Cc: Oliver OHalloran
> Cc: Nathan Lynch
> Cc: Michael Neuling
> Cc: Anton Blanchard
> Cc: Gautham R Shenoy
> Cc: Vaidyanathan Srinivasan
> Signed-off-by: Srikar Dronamraju
> ---
> arch/powerpc/include/asm/topology.h | 10
> arch/powerpc/kernel/smp.c | 37 +
> arch/powerpc/mm/numa.c | 5
> 3 files changed, 52 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/topology.h
> b/arch/powerpc/include/asm/topology.h
> index 2db7ba789720..34812c35018e 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -98,6 +98,7 @@ extern int stop_topology_update(void);
> extern int prrn_is_enabled(void);
> extern int find_and_online_cpu_nid(int cpu);
> extern int timed_topology_update(int nsecs);
> +extern int cpu_to_coregroup_id(int cpu);
> #else
> static inline int start_topology_update(void)
> {
> @@ -120,6 +121,15 @@ static inline int timed_topology_update(int nsecs)
> return 0;
> }
>
> +static inline int cpu_to_coregroup_id(int cpu)
> +{
> +#ifdef CONFIG_SMP
> + return cpu_to_core_id(cpu);
> +#else
> + return 0;
> +#endif
> +}
> +
> #endif /* CONFIG_NUMA && CONFIG_PPC_SPLPAR */
>
> #include
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index ef19eeccd21e..bb25c13bbb79 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -80,6 +80,7 @@ DEFINE_PER_CPU(cpumask_var_t, cpu_sibling_map);
> DEFINE_PER_CPU(cpumask_var_t, cpu_smallcore_map);
> DEFINE_PER_CPU(cpumask_var_t, cpu_l2_cache_map);
> DEFINE_PER_CPU(cpumask_var_t, cpu_core_map);
> +DEFINE_PER_CPU(cpumask_var_t, cpu_coregroup_map);
>
> EXPORT_PER_CPU_SYMBOL(cpu_sibling_map);
> EXPORT_PER_CPU_SYMBOL(cpu_l2_cache_map);
> @@ -91,6 +92,7 @@ enum {
> smt_idx,
> #endif
> bigcore_idx,
> + mc_idx,
> die_idx,
> };
>
> @@ -869,6 +871,21 @@ static const struct cpumask *smallcore_smt_mask(int cpu)
> }
> #endif
>
> +static struct cpumask *cpu_coregroup_mask(int cpu)
> +{
> + return per_cpu(cpu_coregroup_map, cpu);
> +}
> +
> +static bool has_coregroup_support(void)
> +{
> + return coregroup_enabled;
> +}
> +
> +static const struct cpumask *cpu_mc_mask(int cpu)
> +{
> + return cpu_coregroup_mask(cpu);
> +}
> +
> static const struct cpumask *cpu_bigcore_mask(int cpu)
> {
> return cpu_core_mask(cpu);
> @@ -879,6 +896,7 @@ static struct sched_domain_topology_level
> powerpc_topology[] = {
> { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
> #endif
> { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) },
> + { cpu_mc_mask, SD_INIT_NAME(MC) },
> { cpu_cpu_mask, SD_INIT_NAME(DIE) },
> { NULL, },
> };
> @@ -933,6 +951,10 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> GFP_KERNEL, node);
> zalloc_cpumask_var_node(_cpu(cpu_core_map, cpu),
> GFP_KERNEL, node);
> + if (has_coregroup_support())
> + zalloc_cpumask_var_node(_cpu(cpu_coregroup_map,
> cpu),
> + GFP_KERNEL, node);
> +
> #ifdef CONFIG_NEED_MULTIPLE_NODES
> /*
>* numa_node_id() works after this.
> @@ -950,6 +972,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> cpumask_set_cpu(boot_cpuid, cpu_l2_cache_mask(boot_cpuid));
> cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
>
> + if (has_coregroup_support())
> + cpumask_set_cpu(boot_cpuid, cpu_coregroup_mask(boot_cpuid));
> + else
> + powerpc_topology[mc_idx].mask = cpu_bigcore_mask;
> +
The else part could be moved to the common function where we are
modifying the other attributes of the topology array.
> init_big_cores();
> if (has_big_cores) {
> cpumask_set_cpu(boot_cpuid,
> @@ -1241,6 +1268,8 @@ static void remove_cpu_from_masks(int cpu)
> set_cpus_unrelated(cpu, i, cpu_sibling_mask);
> if (has_big_cores)
> set_cpus_unrelated(cpu, i, cpu_smallcore_mask);
> + if (has_coregroup_support())
> + set_cpus_unrelated(cpu, i, cpu_coregroup_mask);
> }
> }
> #endif
> @@ -1301,6 +1330,14 @@ static void add_cpu_to_masks(int cpu)
> add_cpu_to_smallcore_masks(cpu);
> update_mask_by_l2(cpu, cpu_l2_cache_mask);
>
> + if (has_coregroup_support()) {
> + cpumask_set_cpu(cpu, cpu_coregroup_mask(cpu));
> + for_each_cpu(i, cpu_online_mask) {
> + if (cpu_to_coregroup_id(cpu) ==
[PATCH 09/11] Powerpc/smp: Create coregroup domain
Add percpu coregroup maps and masks to create coregroup domain.
If a coregroup doesn't exist, the coregroup domain will be degenerated
in favour of SMT/CACHE domain.
Cc: linuxppc-dev
Cc: Michael Ellerman
Cc: Nick Piggin
Cc: Oliver OHalloran
Cc: Nathan Lynch
Cc: Michael Neuling
Cc: Anton Blanchard
Cc: Gautham R Shenoy
Cc: Vaidyanathan Srinivasan
Signed-off-by: Srikar Dronamraju
---
arch/powerpc/include/asm/topology.h | 10
arch/powerpc/kernel/smp.c | 37 +
arch/powerpc/mm/numa.c | 5
3 files changed, 52 insertions(+)
diff --git a/arch/powerpc/include/asm/topology.h
b/arch/powerpc/include/asm/topology.h
index 2db7ba789720..34812c35018e 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -98,6 +98,7 @@ extern int stop_topology_update(void);
extern int prrn_is_enabled(void);
extern int find_and_online_cpu_nid(int cpu);
extern int timed_topology_update(int nsecs);
+extern int cpu_to_coregroup_id(int cpu);
#else
static inline int start_topology_update(void)
{
@@ -120,6 +121,15 @@ static inline int timed_topology_update(int nsecs)
return 0;
}
+static inline int cpu_to_coregroup_id(int cpu)
+{
+#ifdef CONFIG_SMP
+ return cpu_to_core_id(cpu);
+#else
+ return 0;
+#endif
+}
+
#endif /* CONFIG_NUMA && CONFIG_PPC_SPLPAR */
#include
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index ef19eeccd21e..bb25c13bbb79 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -80,6 +80,7 @@ DEFINE_PER_CPU(cpumask_var_t, cpu_sibling_map);
DEFINE_PER_CPU(cpumask_var_t, cpu_smallcore_map);
DEFINE_PER_CPU(cpumask_var_t, cpu_l2_cache_map);
DEFINE_PER_CPU(cpumask_var_t, cpu_core_map);
+DEFINE_PER_CPU(cpumask_var_t, cpu_coregroup_map);
EXPORT_PER_CPU_SYMBOL(cpu_sibling_map);
EXPORT_PER_CPU_SYMBOL(cpu_l2_cache_map);
@@ -91,6 +92,7 @@ enum {
smt_idx,
#endif
bigcore_idx,
+ mc_idx,
die_idx,
};
@@ -869,6 +871,21 @@ static const struct cpumask *smallcore_smt_mask(int cpu)
}
#endif
+static struct cpumask *cpu_coregroup_mask(int cpu)
+{
+ return per_cpu(cpu_coregroup_map, cpu);
+}
+
+static bool has_coregroup_support(void)
+{
+ return coregroup_enabled;
+}
+
+static const struct cpumask *cpu_mc_mask(int cpu)
+{
+ return cpu_coregroup_mask(cpu);
+}
+
static const struct cpumask *cpu_bigcore_mask(int cpu)
{
return cpu_core_mask(cpu);
@@ -879,6 +896,7 @@ static struct sched_domain_topology_level
powerpc_topology[] = {
{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
#endif
{ cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) },
+ { cpu_mc_mask, SD_INIT_NAME(MC) },
{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
{ NULL, },
};
@@ -933,6 +951,10 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
GFP_KERNEL, node);
zalloc_cpumask_var_node(_cpu(cpu_core_map, cpu),
GFP_KERNEL, node);
+ if (has_coregroup_support())
+ zalloc_cpumask_var_node(_cpu(cpu_coregroup_map,
cpu),
+ GFP_KERNEL, node);
+
#ifdef CONFIG_NEED_MULTIPLE_NODES
/*
* numa_node_id() works after this.
@@ -950,6 +972,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
cpumask_set_cpu(boot_cpuid, cpu_l2_cache_mask(boot_cpuid));
cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
+ if (has_coregroup_support())
+ cpumask_set_cpu(boot_cpuid, cpu_coregroup_mask(boot_cpuid));
+ else
+ powerpc_topology[mc_idx].mask = cpu_bigcore_mask;
+
init_big_cores();
if (has_big_cores) {
cpumask_set_cpu(boot_cpuid,
@@ -1241,6 +1268,8 @@ static void remove_cpu_from_masks(int cpu)
set_cpus_unrelated(cpu, i, cpu_sibling_mask);
if (has_big_cores)
set_cpus_unrelated(cpu, i, cpu_smallcore_mask);
+ if (has_coregroup_support())
+ set_cpus_unrelated(cpu, i, cpu_coregroup_mask);
}
}
#endif
@@ -1301,6 +1330,14 @@ static void add_cpu_to_masks(int cpu)
add_cpu_to_smallcore_masks(cpu);
update_mask_by_l2(cpu, cpu_l2_cache_mask);
+ if (has_coregroup_support()) {
+ cpumask_set_cpu(cpu, cpu_coregroup_mask(cpu));
+ for_each_cpu(i, cpu_online_mask) {
+ if (cpu_to_coregroup_id(cpu) == cpu_to_coregroup_id(i))
+ set_cpus_related(cpu, i, cpu_coregroup_mask);
+ }
+ }
+
if (pkg_id == -1) {
struct cpumask *(*mask)(int) = cpu_sibling_mask;
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index a43eab455be4..d9ab9da85eab 100644
--- a/arch/powerpc/mm/numa.c
+++
