arm64: smp: Skip MC sched domain on SoCs with no LLC

PATCH In review Darren Hart 3 participants 9 messages Mar 1, 2022 – Mar 3, 2022

Subsystems (4)

Stable branch arm64 Driver core Generic architecture topology

lore

Share & tools

lore

Darren Hart

author

2022-03-01 00:35 UTC #0 lore raw

cover arm64: smp: Skip MC sched domain on SoCs with no LLC

After the feedback from Vincent and Barry, I wrote a version of this which performed the test in topology.c and added an ACPI pptt API to detect ancestry of cpu topology nodes. In the end, the new API felt forced and was inconsistent with the other APIs, and the detection loop was pretty ugly with a lot corner case cleanup. It seemed fragile and not very extensible.

This version follows the approach taken by other architectures: wait until smp cpus are online and take advantage of the stored cpu_topology structure to make sched domain topology decisions.

This adds a single point to test and decide on which sched domain topology should be used for an arm64 system without impacting the scheduler code, acpi code, or the construction of the cpu_topology structures. I think this is a cleaner and less invasive solution which properly encapsulates the sched domain topology decision in a way that is easy to maintain and extend over time.

Vincent did mention in an earlier version that by relying on cpumasks and removing the MC sched domain, I didn't address possible future architectures which may have clusters without shared cache and an MC level with shared cache. In this version, I limit the removal of the MC level to topologies where the coregroup weight is 1 (so it doesn't impact such future architectures). I also think that by encapsulating the sched domain topology decision into a single function after smp boot, it will be simple enough for someone to support such a topology if and when it exists, allowing us to create a minimal solution to the immediate problem of BUGing on the current topology.

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Vincent Guittot <[email protected]>

Cc Barry Song <[email protected]>

Cc Valentin Schneider <[email protected]>

Cc D. Scott Phillips <[email protected]>

Cc Ilkka Koskinen <[email protected]>

Darren Hart (1):
arm64: smp: Skip MC sched domain on SoCs with no LLC

 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Signature · 1 line

2.31.1

Darren Hart

author

2022-03-01 00:35 UTC #1 lore raw

PATCH 1/1 arm64: smp: Skip MC sched domain on SoCs with no LLC

Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.

cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.

As a result, build_sched_domain() will BUG() once per CPU with:

BUG: arch topology borken
     the CLS domain not a subset of the MC domain

The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.

Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.

Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.

The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:

For CPU0:

CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]

CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Vincent Guittot <[email protected]>

Cc Barry Song <[email protected]>

Cc Valentin Schneider <[email protected]>

Cc D. Scott Phillips <[email protected]>

Cc Ilkka Koskinen <[email protected]>

Cc <[email protected]> # 5.16.x

Signed-off-by Darren Hart <[email protected]>

---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

arch/arm64/kernel/smp.c +28 -0

index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
 	}
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+	{ cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+	{ cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+	{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
+	{ NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu_topology[cpu].llc_id != -1 &&
+		    cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+			return;
+	}
+
+	pr_info("No LLC siblings, using No MC sched domains topology\n");
+	set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
 	pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
 	hyp_mode_check();
 	apply_alternatives_all();
 	mark_linear_text_alias_ro();
+	update_sched_domain_topology();
 }
 
 void __init smp_prepare_boot_cpu(void)

Signature · 1 line

2.31.1

Vincent Guittot

2022-03-02 09:32 UTC #2 lore raw

Quoted 74 lines · Darren Hart

Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +9 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+

arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454

Quoted 10 lines

arch/arm64/kernel/smp.c +10 -0

+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&

Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system

Quoted 22 lines

arch/arm64/kernel/smp.c +9 -1

+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }
 
 void __init smp_prepare_boot_cpu(void)
--

2.31.1

Darren Hart

author

2022-03-03 02:18 UTC #3 lore raw

Quoted 80 lines · Vincent Guittot

Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)

Quoted 12 lines

arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }

Hi Vincent,

I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:

+ for_each_possible_cpu(cpu) {
+ if (cpu_topology[cpu].llc_id == -1 ||
+ cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+ return;
+ }

Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.

Thanks,

Quoted 19 lines

arch/arm64/kernel/smp.c +6 -1

+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }
 
 void __init smp_prepare_boot_cpu(void)
--

2.31.1

Signature · 2 lines

Darren Hart
Ampere Computing / OS and Kernel

Barry Song

2022-03-03 05:36 UTC #4 lore raw

Quoted 107 lines · Darren Hart

Quoted 80 lines · Vincent Guittot
Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
Quoted 12 lines
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Hi Vincent,
I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:
(quoted diff) +5 -0
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id == -1 ||
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }

Quoted 3 lines

Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.

I wonder if we can fix it by this

drivers/base/arch_topology.c +7 -0

index 976154140f0b..551655ccd0eb 100644
@@ -627,6 +627,13 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
                if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
                        core_mask = &cpu_topology[cpu].llc_sibling;
        }
+       /*
+        * Some machines have no LLC but have clusters, we let MC = CLUSTER
+        * as MC should always be after CLUSTER. But anyway, the MC domain
+        * will be removed
+        */
+       if (cpumask_subset(core_mask, &cpu_topology[cpu].cluster_sibling))
+               core_mask = &cpu_topology[cpu].cluster_sibling;
 
        return core_mask;
 }

as it can make all kinds of topologies happy - symmetric and asymmetric.

Quoted 18 lines

Thanks,

Quoted 15 lines

arch/arm64/kernel/smp.c +6 -0

+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/smp.c +0 -1
 void __init smp_prepare_boot_cpu(void)
--
2.31.1
--
Darren Hart
Ampere Computing / OS and Kernel

Thanks
Barry

Darren Hart

author

2022-03-03 16:35 UTC #5 lore raw

Quoted 134 lines · Barry Song

Quoted 107 lines · Darren Hart
Quoted 80 lines · Vincent Guittot
Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
Quoted 12 lines
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Hi Vincent,
I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:
(quoted diff) +5 -0
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id == -1 ||
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Quoted 3 lines
Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.
I wonder if we can fix it by this
drivers/base/arch_topology.c +7 -0
index 976154140f0b..551655ccd0eb 100644
@@ -627,6 +627,13 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
                if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
                        core_mask = &cpu_topology[cpu].llc_sibling;
        }
+       /*
+        * Some machines have no LLC but have clusters, we let MC = CLUSTER
+        * as MC should always be after CLUSTER. But anyway, the MC domain
+        * will be removed
+        */
+       if (cpumask_subset(core_mask, &cpu_topology[cpu].cluster_sibling))
+               core_mask = &cpu_topology[cpu].cluster_sibling;
 
        return core_mask;
 }

Quoted 2 lines

as it can make all kinds of topologies happy - symmetric and asymmetric.

Hah. Full circle. Yes, this works, and it's basically what we'd started with internally. I ended up exploring various paths here to avoid a "band aid" and to target the fix and minimize impact. That said, after digging through the acpi, topology, smp, and sched domains code... I don't think this approach is a band aid and it's a very minimal solution. The only downside I can think of is masking a potential topology bug and not catching it in the scheduler - that seems very unlikely. I'm perfectly happy with this solution as well.

Will D, would you prefer this approach?

+Sudeep, Greg, and Rafael,

Are you OK with this approach?

If so, we can drop my arm64 specific new topology patch and I can send a version of this one out (suggested-by Barry of course), unless you'd prefer to send it Barry?

Thanks,

Quoted 29 lines

Quoted 18 lines

Thanks,

Quoted 15 lines

arch/arm64/kernel/smp.c +6 -0

+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/smp.c +0 -1
 void __init smp_prepare_boot_cpu(void)
--
2.31.1
--
Darren Hart
Ampere Computing / OS and Kernel

Thanks
Barry

Signature · 2 lines

Darren Hart
Ampere Computing / OS and Kernel

Barry Song

2022-03-03 21:43 UTC #6 lore raw

Quoted 150 lines · Darren Hart

Quoted 134 lines · Barry Song
Quoted 107 lines · Darren Hart
Quoted 80 lines · Vincent Guittot
Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
Quoted 12 lines
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Hi Vincent,
I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:
(quoted diff) +5 -0
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id == -1 ||
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Quoted 3 lines
Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.
I wonder if we can fix it by this
drivers/base/arch_topology.c +7 -0
index 976154140f0b..551655ccd0eb 100644
@@ -627,6 +627,13 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
                if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
                        core_mask = &cpu_topology[cpu].llc_sibling;
        }
+       /*
+        * Some machines have no LLC but have clusters, we let MC = CLUSTER
+        * as MC should always be after CLUSTER. But anyway, the MC domain
+        * will be removed
+        */
+       if (cpumask_subset(core_mask, &cpu_topology[cpu].cluster_sibling))
+               core_mask = &cpu_topology[cpu].cluster_sibling;
 
        return core_mask;
 }
Quoted 2 lines
as it can make all kinds of topologies happy - symmetric and asymmetric.
Hah. Full circle. Yes, this works, and it's basically what we'd started with internally. I ended up exploring various paths here to avoid a "band aid" and to target the fix and minimize impact. That said, after digging through the acpi, topology, smp, and sched domains code... I don't think this approach is a band aid and it's a very minimal solution. The only downside I can think of is masking a potential topology bug and not catching it in the scheduler - that seems very unlikely. I'm perfectly happy with this solution as well.
Will D, would you prefer this approach?

Quoted 7 lines

+Sudeep, Greg, and Rafael,
Are you OK with this approach?
If so, we can drop my arm64 specific new topology patch and I can send a version of this one out (suggested-by Barry of course), unless you'd prefer to send it Barry?

i prefer this solution as anyway, cpu_coregroup_mask() is a combination of a couple of masks. we are just putting cluster_mask into consideration.

const struct cpumask *cpu_coregroup_mask(int cpu)

{
        const cpumask_t *core_mask = cpumask_of_node(cpu_to_node(cpu));

        /* Find the smaller of NUMA, core or LLC siblings */
        if (cpumask_subset(&cpu_topology[cpu].core_sibling, core_mask)) {
                /* not numa in package, lets use the package siblings */
                core_mask = &cpu_topology[cpu].core_sibling;
        }
        if (cpu_topology[cpu].llc_id != -1) {
                if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
                        core_mask = &cpu_topology[cpu].llc_sibling;
        }

        return core_mask;
}

we are also making this machine happy:

CPU0-CPU1 have cluster but have no LLC.
CPU2-CPU3 have both cluster and LLC.

for cpu0 and cpu1, they are getting cluster level only, mc will be dropped. for cpu2 and cpu3, they are getting both cluster and mc.

Please feel free to send, Darren. And i feel wake_affine series will finally have to handle your exception.

Quoted 30 lines

Thanks,

Quoted 27 lines

Quoted 26 lines

Thanks,

Quoted 15 lines

arch/arm64/kernel/smp.c +6 -0

+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());

@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }

Quoted 4 lines

Quoted 4 lines
arch/arm64/kernel/smp.c +0 -1
 void __init smp_prepare_boot_cpu(void)
--
2.31.1

--
Darren Hart
Ampere Computing / OS and Kernel

Quoted 6 lines

Quoted 2 lines
Thanks
Barry
--
Darren Hart
Ampere Computing / OS and Kernel

Thanks
Barry

Vincent Guittot

2022-03-03 08:09 UTC #7 lore raw

In reply to Darren Hart

Quoted 101 lines · Darren Hart

Quoted 80 lines · Vincent Guittot
Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
Quoted 12 lines
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Hi Vincent,
I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:

arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 457

Quoted 5 lines

arch/arm64/kernel/smp.c +5 -0

+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id == -1 ||
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }

This works.
Also , do you really need to loop on all possible cpus ? Would it be enough to check only the 1st cpu ?
You won't be able to support a mixed topology so all cpus have the same kind of topology i.e either cluster before or cluster before the MC level

Quoted 30 lines

Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.
Thanks,
Quoted 15 lines
Quoted 15 lines
arch/arm64/kernel/smp.c +6 -0
+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());
@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }
 
Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/smp.c +0 -1
 void __init smp_prepare_boot_cpu(void)
--
2.31.1
--
Darren Hart
Ampere Computing / OS and Kernel

Darren Hart

author

2022-03-03 16:02 UTC #8 lore raw

Quoted 108 lines · Vincent Guittot

Quoted 101 lines · Darren Hart
Quoted 80 lines · Vincent Guittot
Quoted 79 lines · Darren Hart
Ampere Altra defines CPU clusters in the ACPI PPTT. They share a Snoop Control Unit, but have no shared CPU-side last level cache.
cpu_coregroup_mask() will return a cpumask with weight 1, while cpu_clustergroup_mask() will return a cpumask with weight 2.
As a result, build_sched_domain() will BUG() once per CPU with:
BUG: arch topology borken
     the CLS domain not a subset of the MC domain
The MC level cpumask is then extended to that of the CLS child, and is later removed entirely as redundant. This sched domain topology is an improvement over previous topologies, or those built without SCHED_CLUSTER, particularly for certain latency sensitive workloads. With the current scheduler model and heuristics, this is a desirable default topology for Ampere Altra and Altra Max system.
Introduce an alternate sched domain topology for arm64 without the MC level and test for llc_sibling weight 1 across all CPUs to enable it.
Do this in arch/arm64/kernel/smp.c (as opposed to arch/arm64/kernel/topology.c) as all the CPU sibling maps are now populated and we avoid needing to extend the drivers/acpi/pptt.c API to detect the cluster level being above the cpu llc level. This is consistent with other architectures and provides a readily extensible mechanism for other alternate topologies.
The final sched domain topology for a 2 socket Ampere Altra system is unchanged with or without CONFIG_SCHED_CLUSTER, and the BUG is avoided:
For CPU0:
CONFIG_SCHED_CLUSTER=y
CLS [0-1]
DIE [0-79]
NUMA [0-159]
CONFIG_SCHED_CLUSTER is not set
DIE [0-79]
NUMA [0-159]
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Vincent Guittot <[email protected]>
Cc Barry Song <[email protected]>
Cc Valentin Schneider <[email protected]>
Cc D. Scott Phillips <[email protected]>
Cc Ilkka Koskinen <[email protected]>
Cc <[email protected]> # 5.16.x
Signed-off-by Darren Hart <[email protected]>
---
 arch/arm64/kernel/smp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/kernel/smp.c +14 -0
index 27df5c1e6baa..3597e75645e1 100644
@@ -433,6 +433,33 @@ static void __init hyp_mode_check(void)
        }
 }
 
+static struct sched_domain_topology_level arm64_no_mc_topology[] = {
+#ifdef CONFIG_SCHED_SMT
+       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+#endif
+
+#ifdef CONFIG_SCHED_CLUSTER
+       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+#endif
+
+       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
+       { NULL, },
+};
+
+static void __init update_sched_domain_topology(void)
Quoted 12 lines
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 454
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+{
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id != -1 &&
Have you tested it with a non-acpi system ? AFAICT, llc_id is only set by ACPI system and llc_id == -1 for others like DT based system
Quoted 3 lines
arch/arm64/kernel/smp.c +3 -0
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }
Hi Vincent,
I did not have a non-acpi system to test, no. You're right of course, llc_id is only set by ACPI systems on arm64. We could wrap this in a CONFIG_ACPI ifdef (or IS_ENABLED), but I think this would be preferable:
arch/arm64/kernel/smp.c static void __init hyp_mode_check(void) · line 457
Quoted 5 lines
arch/arm64/kernel/smp.c +5 -0
+       for_each_possible_cpu(cpu) {
+               if (cpu_topology[cpu].llc_id == -1 ||
+                   cpumask_weight(&cpu_topology[cpu].llc_sibling) > 1)
+                       return;
+       }

Quoted 6 lines

This works.
Also , do you really need to loop on all possible cpus ? Would it be enough to check only the 1st cpu ?
You won't be able to support a mixed topology so all cpus have the same kind of topology i.e either cluster before or cluster before the MC level

My intention here is to restrict the use of of the new topology to a very specific architecture where the problem is known to manifest, and avoid introducing any unexpected change to other systems.

For other systems, they will break on the first loop, so the loop is also minimal impact.

As for supporting a mixed topology, my intention was again to not make any statement about the existance or viability of such systems. If they would break before, they would still break. If a new topology is needed for them, this provides a easily modifiable location to do that.

If the consensus is we don't need the loop, this simplifies my specific use case at the cost of applying to a broader set (but only hypothetically I believe) of topologies. So no objection to dropping the loop.

Will, do you have a preference? Lean toward targeted change and minimal impact, or lean toward simpler implementation with slightly broader impact?

Thanks,

Quoted 32 lines

Quoted 32 lines
Quickly tested on Altra successfully. Would appreciate anyone with non-acpi arm64 systems who can test and verify this behaves as intended. I will ask around tomorrow as well to see what I may have access to.
Thanks,
Quoted 15 lines
Quoted 15 lines
arch/arm64/kernel/smp.c +6 -0
+
+       pr_info("No LLC siblings, using No MC sched domains topology\n");
+       set_sched_topology(arm64_no_mc_topology);
+}
+
 void __init smp_cpus_done(unsigned int max_cpus)
 {
        pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());
@@ -440,6 +467,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
        hyp_mode_check();
        apply_alternatives_all();
        mark_linear_text_alias_ro();
+       update_sched_domain_topology();
 }
 
Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/smp.c +0 -1
 void __init smp_prepare_boot_cpu(void)
--
2.31.1
--
Darren Hart
Ampere Computing / OS and Kernel

Signature · 2 lines

Darren Hart
Ampere Computing / OS and Kernel