Skip to contentSkip to conversation

x86, sched: allow topolgies where NUMA nodes share an LLC

PATCH In review Dave Hansen 3 participants 4 messages Jun 8, 2017 x86

lore
Share & tools
lore

Dave Hansen

author maintainer
#0 lore raw

PATCH 1/1 x86, sched: allow topolgies where NUMA nodes share an LLC

From: Dave Hansen <[email protected]>

Our SMP boot code has a series of assumptions about what NUMA nodes are that are enforced via topology_sane(). Once upon a time, we verified that a CPU package only contained a single node (fixed in cebf15eb0). Today, we verify that SMT siblings and LLCs do not span nodes.

The SMT siblings assumption is safe, but the LLC is violated on current hardware.

Remove the "sanity" check on LLC spanning NUMA nodes. Also make sure to set 'x86_has_numa_in_package = true' which ensures that we use the x86_numa_in_package_topology[]. The default topology layers NUMA "outside" of the cache, which is wrong when the cache spans multiple nodes.

This fixes the warnings, but it does theoretically throw away the LLC from being consulted in scheduling decisions, if the LLC is shared at a boundary that is not also a NUMA node.

Cc Luck, Tony <[email protected]>
Cc Tim Chen <[email protected]>
Cc Peter Zijlstra (Intel) <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc David Rientjes <[email protected]>
Cc Igor Mammedov <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Prarit Bhargava <[email protected]>
Cc Toshi Kani <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>

---

 b/arch/x86/kernel/smpboot.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
arch/x86/kernel/smpboot.c renamed from arch/x86/kernel/smpboot.c~x86-numa-nodes-share-llc +10 -5
diff -puN arch/x86/kernel/smpboot.c~x86-numa-nodes-share-llc arch/x86/kernel/smpboot.c
@@ -460,7 +460,7 @@ static bool match_llc(struct cpuinfo_x86
 
 	if (per_cpu(cpu_llc_id, cpu1) != BAD_APICID &&
 	    per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2))
-		return topology_sane(c, o, "llc");
+		return true;
 
 	return false;
 }
@@ -520,7 +520,8 @@ static struct sched_domain_topology_leve
 
 /*
  * Set if a package/die has multiple NUMA nodes inside.
- * AMD Magny-Cours and Intel Cluster-on-Die have this.
+ * AMD Magny-Cours, Intel Cluster-on-Die, and Intel
+ * Sub-NUMA Clustering have this.
  */
 static bool x86_has_numa_in_package;
 
@@ -548,9 +549,13 @@ void set_cpu_sibling_map(int cpu)
 		if ((i == cpu) || (has_smt && match_smt(c, o)))
 			link_mask(topology_sibling_cpumask, cpu, i);
 
-		if ((i == cpu) || (has_mp && match_llc(c, o)))
-			link_mask(cpu_llc_shared_mask, cpu, i);
-
+		if ((i == cpu) || (has_mp && match_llc(c, o))) {
+			/* LLC may be shared across NUMA nodes */
+			if (topology_same_node(c, o))
+				link_mask(cpu_llc_shared_mask, cpu, i);
+			else
+				x86_has_numa_in_package = true;
+		}
 	}
 
 	/*

_

#1 lore raw
Quoted 11 lines · Dave Hansen

From: Dave Hansen <[email protected]>

Our SMP boot code has a series of assumptions about what NUMA nodes are that are enforced via topology_sane(). Once upon a time, we verified that a CPU package only contained a single node (fixed in cebf15eb0). Today, we verify that SMT siblings and LLCs do not span nodes.

The SMT siblings assumption is safe, but the LLC is violated on current hardware.

What does? That does sound broken. How can a cache domain sanely span memory controllers?

This needs far more explanation.

#2 lore raw
Quoted 2 lines

What does? That does sound broken. How can a cache domain sanely span memory controllers?

Think "cluster on die" with cores on the socket split into two clusters, but still sharing LLC.

-Tony

#3 lore raw
Quoted 4 lines · Tony
Quoted 2 lines

What does? That does sound broken. How can a cache domain sanely span memory controllers?

Think "cluster on die" with cores on the socket split into two clusters, but still sharing LLC.

The thing is, cluster-on-die works with the current code, and therefore seems to modify the SRAT an CPUID information in a consistent manner.

Which in turn seems to suggest the LLC really is split for cluster-on-die.

This is something new, and the Changelog is absolute crap for not explaining _anything_.

So while SRAT seems to invent new nodes, the CPUID topology bits still describes the full LLC, now shared across nodes.

Is this accurate?, do these nodes, as described by SRAT, actually have a memory controller each? And is the LLC still fully integrated across the nodes? If so, we need to go fix the scheduler domain topology to put a cache domain across nodes (which is going to be painful).

Just making the warning go away and not explaining things sucks.