Re: [PATCH 09/11] Powerpc/smp: Create coregroup domain

* Gautham R Shenoy [off-list ref] [2020-07-17 13:49:26]:

On Tue, Jul 14, 2020 at 10:06:22AM +0530, Srikar Dronamraju wrote:

quoted

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 <redacted>
Cc: Michael Ellerman <redacted>
Cc: Nick Piggin <redacted>
Cc: Oliver OHalloran <redacted>
Cc: Nathan Lynch <redacted>
Cc: Michael Neuling <redacted>
Cc: Anton Blanchard <redacted>
Cc: Gautham R Shenoy <redacted>
Cc: Vaidyanathan Srinivasan <redacted>
Signed-off-by: Srikar Dronamraju <redacted>
---
 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. 

quoted

 	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