Skip to contentSkip to conversation

x86/numa: Check for nonsensical topologies on real hw as well

tip:sched/core tip-bot for Ingo Molnar 5 participants 8 messages May 9, 2012 – May 11, 2012 x86

lore
Share & tools
lore
#0 lore raw
Commit-ID:  ad7687dde8780a0d618a3e3b5a62bb383696fc22
Gitweb:     http://git.kernel.org/tip/ad7687dde8780a0d618a3e3b5a62bb383696fc22
Author:     Ingo Molnar <[email protected]>
AuthorDate: Wed, 9 May 2012 13:31:47 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 9 May 2012 13:32:35 +0200

x86/numa: Check for nonsensical topologies on real hw as well

Instead of only checking nonsensical topologies on numa-emu, do it on real hardware as well, and print a warning.

---
 arch/x86/kernel/smpboot.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/kernel/smpboot.c +6 -6
index edfd03a..7c53d96 100644
@@ -337,10 +337,10 @@ void __cpuinit set_cpu_sibling_map(int cpu)
 		for_each_cpu(i, cpu_sibling_setup_mask) {
 			struct cpuinfo_x86 *o = &cpu_data(i);
 
-#ifdef CONFIG_NUMA_EMU
-			if (cpu_to_node(cpu) != cpu_to_node(i))
+			if (cpu_to_node(cpu) != cpu_to_node(i)) {
+				WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
 				continue;
-#endif
+			}
 
 			if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
 				if (c->phys_proc_id == o->phys_proc_id &&
@@ -365,10 +365,10 @@ void __cpuinit set_cpu_sibling_map(int cpu)
 	}
 
 	for_each_cpu(i, cpu_sibling_setup_mask) {
-#ifdef CONFIG_NUMA_EMU
-		if (cpu_to_node(cpu) != cpu_to_node(i))
+		if (cpu_to_node(cpu) != cpu_to_node(i)) {
+			WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
 			continue;
-#endif
+		}
 
 		if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
 		    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {
#1 lore raw
Quoted 52 lines · tip-bot for Ingo Molnar

Commit-ID:  ad7687dde8780a0d618a3e3b5a62bb383696fc22
Gitweb:     http://git.kernel.org/tip/ad7687dde8780a0d618a3e3b5a62bb383696fc22
Author:     Ingo Molnar <[email protected]>
AuthorDate: Wed, 9 May 2012 13:31:47 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 9 May 2012 13:32:35 +0200

x86/numa: Check for nonsensical topologies on real hw as well

Instead of only checking nonsensical topologies on numa-emu, do it on real hardware as well, and print a warning.

---
 arch/x86/kernel/smpboot.c |   12 ++++++------

 1 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/kernel/smpboot.c +3 -3
index edfd03a..7c53d96 100644
@@ -337,10 +337,10 @@ void __cpuinit set_cpu_sibling_map(int cpu)
                for_each_cpu(i, cpu_sibling_setup_mask) {
                        struct cpuinfo_x86 *o = &cpu_data(i);
 
-#ifdef CONFIG_NUMA_EMU
-                       if (cpu_to_node(cpu) != cpu_to_node(i))
+                       if (cpu_to_node(cpu) != cpu_to_node(i)) {
+                               WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
                                continue;
-#endif
+                       }
 
                        if (cpu_has(c, X86_FEATURE_TOPOEXT)) {

                               if (c->phys_proc_id == o->phys_proc_id &&

arch/x86/kernel/smpboot.c +3 -3
@@ -365,10 +365,10 @@ void __cpuinit set_cpu_sibling_map(int cpu)
        }
 
        for_each_cpu(i, cpu_sibling_setup_mask) {
-#ifdef CONFIG_NUMA_EMU
-               if (cpu_to_node(cpu) != cpu_to_node(i))
+               if (cpu_to_node(cpu) != cpu_to_node(i)) {
+                       WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
                        continue;
-#endif
+               }
 
Quoted 2 lines

               if (per_cpu(cpu_llc_id, cpu) != BAD_APICID && per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {

get wrong warning on system without using numa emu.

------------[ cut here ]------------
WARNING: at arch/x86/kernel/smpboot.c:320 set_cpu_sibling_map+0x9c/0x382()
Hardware name: Sun Blade X6270 M3
sched: CPU #8's thread-sibling CPU #0 not on the same node! [node 1 !=
0]. Ignoring sibling dependency.
Modules linked in:

Pid: 0, comm: swapper/8 Not tainted 3.4.0-rc6-yh-03506-gac97716-dirty #308

Call Trace:
 [<ffffffff8106a7d1>] warn_slowpath_common+0x83/0x9b
 [<ffffffff8106a88c>] warn_slowpath_fmt+0x46/0x48
 [<ffffffff81d78b78>] set_cpu_sibling_map+0x9c/0x382
 [<ffffffff81d77aca>] ? mcheck_cpu_init+0xcf/0x11c
 [<ffffffff81d760ce>] ? identify_cpu+0x24c/0x251
 [<ffffffff81d78f5c>] smp_callin+0xfe/0x114
 [<ffffffff81d78f8a>] start_secondary+0x18/0xc4
 [<ffffffff81d78f72>] ? smp_callin+0x114/0x114
---[ end trace b0a3493fd2ab781d ]---
------------[ cut here ]------------
WARNING: at arch/x86/kernel/smpboot.c:348 set_cpu_sibling_map+0x20f/0x382()
Hardware name: Sun Blade X6270 M3
sched: CPU #8's core-sibling CPU #0 not on the same node! [node 1 !=
0]. Ignoring sibling dependency.
Modules linked in:

Pid: 0, comm: swapper/8 Tainted: G W
3.4.0-rc6-yh-03506-gac97716-dirty #308

Call Trace:
 [<ffffffff8106a7d1>] warn_slowpath_common+0x83/0x9b
 [<ffffffff8106a88c>] warn_slowpath_fmt+0x46/0x48
 [<ffffffff81d78ceb>] set_cpu_sibling_map+0x20f/0x382
 [<ffffffff81d77aca>] ? mcheck_cpu_init+0xcf/0x11c
 [<ffffffff81d760ce>] ? identify_cpu+0x24c/0x251
 [<ffffffff81d78f5c>] smp_callin+0xfe/0x114
 [<ffffffff81d78f8a>] start_secondary+0x18/0xc4
 [<ffffffff81d78f72>] ? smp_callin+0x114/0x114
---[ end trace b0a3493fd2ab781e ]---

SRAT: PXM 0 -> APIC 0x00 -> Node 0
SRAT: PXM 0 -> APIC 0x02 -> Node 0
SRAT: PXM 0 -> APIC 0x04 -> Node 0
SRAT: PXM 0 -> APIC 0x06 -> Node 0
SRAT: PXM 0 -> APIC 0x08 -> Node 0
SRAT: PXM 0 -> APIC 0x0a -> Node 0
SRAT: PXM 0 -> APIC 0x0c -> Node 0
SRAT: PXM 0 -> APIC 0x0e -> Node 0
SRAT: PXM 1 -> APIC 0x20 -> Node 1
SRAT: PXM 1 -> APIC 0x22 -> Node 1
SRAT: PXM 1 -> APIC 0x24 -> Node 1
SRAT: PXM 1 -> APIC 0x26 -> Node 1
SRAT: PXM 1 -> APIC 0x28 -> Node 1
SRAT: PXM 1 -> APIC 0x2a -> Node 1
SRAT: PXM 1 -> APIC 0x2c -> Node 1
SRAT: PXM 1 -> APIC 0x2e -> Node 1
SRAT: PXM 0 -> APIC 0x01 -> Node 0
SRAT: PXM 0 -> APIC 0x03 -> Node 0
SRAT: PXM 0 -> APIC 0x05 -> Node 0
SRAT: PXM 0 -> APIC 0x07 -> Node 0
SRAT: PXM 0 -> APIC 0x09 -> Node 0
SRAT: PXM 0 -> APIC 0x0b -> Node 0
SRAT: PXM 0 -> APIC 0x0d -> Node 0
SRAT: PXM 0 -> APIC 0x0f -> Node 0
SRAT: PXM 1 -> APIC 0x21 -> Node 1
SRAT: PXM 1 -> APIC 0x23 -> Node 1
SRAT: PXM 1 -> APIC 0x25 -> Node 1
SRAT: PXM 1 -> APIC 0x27 -> Node 1
SRAT: PXM 1 -> APIC 0x29 -> Node 1
SRAT: PXM 1 -> APIC 0x2b -> Node 1
SRAT: PXM 1 -> APIC 0x2d -> Node 1
SRAT: PXM 1 -> APIC 0x2f -> Node 1

ACPI: LAPIC (acpi_id[0x00] lapic_id[0x00] enabled)
ACPI: LAPIC (acpi_id[0x02] lapic_id[0x02] enabled)
ACPI: LAPIC (acpi_id[0x04] lapic_id[0x04] enabled)
ACPI: LAPIC (acpi_id[0x06] lapic_id[0x06] enabled)
ACPI: LAPIC (acpi_id[0x08] lapic_id[0x08] enabled)
ACPI: LAPIC (acpi_id[0x0a] lapic_id[0x0a] enabled)
ACPI: LAPIC (acpi_id[0x0c] lapic_id[0x0c] enabled)
ACPI: LAPIC (acpi_id[0x0e] lapic_id[0x0e] enabled)
ACPI: LAPIC (acpi_id[0x10] lapic_id[0x20] enabled)
ACPI: LAPIC (acpi_id[0x12] lapic_id[0x22] enabled)
ACPI: LAPIC (acpi_id[0x14] lapic_id[0x24] enabled)
ACPI: LAPIC (acpi_id[0x16] lapic_id[0x26] enabled)
ACPI: LAPIC (acpi_id[0x18] lapic_id[0x28] enabled)
ACPI: LAPIC (acpi_id[0x1a] lapic_id[0x2a] enabled)
ACPI: LAPIC (acpi_id[0x1c] lapic_id[0x2c] enabled)
ACPI: LAPIC (acpi_id[0x1e] lapic_id[0x2e] enabled)
ACPI: LAPIC (acpi_id[0x01] lapic_id[0x01] enabled)
ACPI: LAPIC (acpi_id[0x03] lapic_id[0x03] enabled)
ACPI: LAPIC (acpi_id[0x05] lapic_id[0x05] enabled)
ACPI: LAPIC (acpi_id[0x07] lapic_id[0x07] enabled)
ACPI: LAPIC (acpi_id[0x09] lapic_id[0x09] enabled)
ACPI: LAPIC (acpi_id[0x0b] lapic_id[0x0b] enabled)
ACPI: LAPIC (acpi_id[0x0d] lapic_id[0x0d] enabled)
ACPI: LAPIC (acpi_id[0x0f] lapic_id[0x0f] enabled)
ACPI: LAPIC (acpi_id[0x11] lapic_id[0x21] enabled)
ACPI: LAPIC (acpi_id[0x13] lapic_id[0x23] enabled)
ACPI: LAPIC (acpi_id[0x15] lapic_id[0x25] enabled)
ACPI: LAPIC (acpi_id[0x17] lapic_id[0x27] enabled)
ACPI: LAPIC (acpi_id[0x19] lapic_id[0x29] enabled)
ACPI: LAPIC (acpi_id[0x1b] lapic_id[0x2b] enabled)
ACPI: LAPIC (acpi_id[0x1d] lapic_id[0x2d] enabled)
ACPI: LAPIC (acpi_id[0x1f] lapic_id[0x2f] enabled)

init_cpu_to_node:
cpu 0 -> apicid 0x0 -> node 0
cpu 1 -> apicid 0x2 -> node 0
cpu 2 -> apicid 0x4 -> node 0
cpu 3 -> apicid 0x6 -> node 0
cpu 4 -> apicid 0x8 -> node 0
cpu 5 -> apicid 0xa -> node 0
cpu 6 -> apicid 0xc -> node 0
cpu 7 -> apicid 0xe -> node 0
cpu 8 -> apicid 0x20 -> node 1
cpu 9 -> apicid 0x22 -> node 1
cpu 10 -> apicid 0x24 -> node 1
cpu 11 -> apicid 0x26 -> node 1
cpu 12 -> apicid 0x28 -> node 1
cpu 13 -> apicid 0x2a -> node 1
cpu 14 -> apicid 0x2c -> node 1
cpu 15 -> apicid 0x2e -> node 1
cpu 16 -> apicid 0x1 -> node 0
cpu 17 -> apicid 0x3 -> node 0
cpu 18 -> apicid 0x5 -> node 0
cpu 19 -> apicid 0x7 -> node 0
cpu 20 -> apicid 0x9 -> node 0
cpu 21 -> apicid 0xb -> node 0
cpu 22 -> apicid 0xd -> node 0
cpu 23 -> apicid 0xf -> node 0
cpu 24 -> apicid 0x21 -> node 1
cpu 25 -> apicid 0x23 -> node 1
cpu 26 -> apicid 0x25 -> node 1
cpu 27 -> apicid 0x27 -> node 1
cpu 28 -> apicid 0x29 -> node 1
cpu 29 -> apicid 0x2b -> node 1
cpu 30 -> apicid 0x2d -> node 1
cpu 31 -> apicid 0x2f -> node 1

#2 lore raw
Quoted 1 line · Yinghai Lu

get wrong warning on system without using numa emu.

Oops yeah, that condition is quite possible to hit, its just that with a sane topology the end result doesn't change..

How about we do something like this

---
 arch/x86/kernel/smpboot.c |  114 +++++++++++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 46 deletions(-)
arch/x86/kernel/smpboot.c +68 -46
index e543e02..2552463 100644
@@ -299,70 +299,92 @@ void __cpuinit smp_store_cpu_info(int id)
 		identify_secondary_cpu(c);
 }
 
-static void __cpuinit link_thread_siblings(int cpu1, int cpu2)
+static bool __cpuinit topology_sane(int cpu1, int cpu2, const char *name)
 {
-	cpumask_set_cpu(cpu1, cpu_sibling_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_sibling_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_core_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_core_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_llc_shared_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_llc_shared_mask(cpu1));
+	return WARN(cpu_to_node(cpu1) != cpu_to_node(cpu2),
+		"sched: CPU #%d's %s-sibling CPU #%d is not on the same node! "
+		"[node: %d != %d]. Ignoring siblings dependency.\n",
+		cpu1, name, cpu2, cpu_to_node(cpu1), cpu_to_node(cpu2));
 }
 
+#define link_mask(_m, c1, c2)						\
+do {									\
+	cpumask_set_cpu((c1), cpu_##_m##_mask(c2));			\
+	cpumask_set_cpu((c2), cpu_##_m##_mask(c1));			\
+} while (0)
 
-void __cpuinit set_cpu_sibling_map(int cpu)
+static bool __cpuinit match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
 {
-	int i;
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	if (!topology_sane(c->cpu_index, o->cpu_index, "smt"))
+		return false;
+
+	if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
+		if (c->phys_proc_id == o->phys_proc_id &&
+		    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i) &&
+		    c->compute_unit_id == o->compute_unit_id)
+			return true;
+
+	} else if (c->phys_proc_id == o->phys_proc_id &&
+		   c->cpu_core_id == o->cpu_core_id) {
+		return true;
+	}
 
-	cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
+	return false;
+}
 
-	if (smp_num_siblings > 1) {
-		for_each_cpu(i, cpu_sibling_setup_mask) {
-			struct cpuinfo_x86 *o = &cpu_data(i);
+static bool __cpuinit match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (!topology_sane(c->cpu_index, o->cpu_index, "llc"))
+		return false;
 
-			if (cpu_to_node(cpu) != cpu_to_node(i)) {
-				WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-				continue;
-			}
+	if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
+	    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i))
+		return true;
 
-			if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
-				if (c->phys_proc_id == o->phys_proc_id &&
-				    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i) &&
-				    c->compute_unit_id == o->compute_unit_id)
-					link_thread_siblings(cpu, i);
-			} else if (c->phys_proc_id == o->phys_proc_id &&
-				   c->cpu_core_id == o->cpu_core_id) {
-				link_thread_siblings(cpu, i);
-			}
-		}
-	} else {
-		cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
-	}
+	return false;
+}
+
+static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (!topology_sane(c->cpu_index, o->cpu_index, "mc"))
+		return false;
+
+	if (c->phys_proc_id == cpu_data(i).phys_proc_id)
+		return true;
+
+	return false;
+}
 
-	cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+void __cpuinit set_cpu_sibling_map(int cpu)
+{
+	bool has_mc = boot_cpu_data.x86_max_cores > 1;
+	bool has_smt = smp_num_siblings > 1;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	struct cpuinfo_x86 *o;
+	int i;
 
-	if (__this_cpu_read(cpu_info.x86_max_cores) == 1) {
-		cpumask_copy(cpu_core_mask(cpu), cpu_sibling_mask(cpu));
+	cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
+
+	if (!has_smt && !has_mc) {
+		cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_core_mask(cpu));
 		c->booted_cores = 1;
 		return;
 	}
 
 	for_each_cpu(i, cpu_sibling_setup_mask) {
-		if (cpu_to_node(cpu) != cpu_to_node(i)) {
-			WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-			continue;
-		}
+		o = &cpu_data(i);
 
-		if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
-		    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {
-			cpumask_set_cpu(i, cpu_llc_shared_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_llc_shared_mask(i));
-		}
+		if ((i == cpu) || (has_smt && match_smt(c, o)))
+			link_mask(siblings, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_llc(c, o)))
+			link_mask(llc_shared, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_cpu(c, o))) {
+			link_mask(core, cpu, i);
 
-		if (c->phys_proc_id == cpu_data(i).phys_proc_id) {
-			cpumask_set_cpu(i, cpu_core_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_core_mask(i));
 			/*
 			 *  Does this new cpu bringup a new core?
 			 */
#3 lore raw

arch/x86/kernel/smpboot.c void __cpuinit smp_store_cpu_info(int id) · line 356

Quoted 10 lines · Peter Zijlstra
arch/x86/kernel/smpboot.c +10 -0
+static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+       if (!topology_sane(c->cpu_index, o->cpu_index, "mc"))
+               return false;
+
+       if (c->phys_proc_id == cpu_data(i).phys_proc_id)
+               return true;
+
+       return false;
+} 

Argh, that has the very same problem.. it (and the same for the other two match funcions).

Something like:

  if (c->phys_proc_id == cpu_data(i).phys_proc_id && topology_sane())

might do, since then we only verify the topology after we would otherwise have already accepted it.

#4 lore raw

OK here's one that compiles and boots without silly warnings on a WSM-EP

---
 arch/x86/kernel/smpboot.c |  112 ++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 46 deletions(-)
arch/x86/kernel/smpboot.c +66 -46
index e543e02..e8bf636 100644
@@ -299,70 +299,90 @@ void __cpuinit smp_store_cpu_info(int id)
 		identify_secondary_cpu(c);
 }
 
-static void __cpuinit link_thread_siblings(int cpu1, int cpu2)
+static bool __cpuinit 
+topology_sane(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o, const char *name)
 {
-	cpumask_set_cpu(cpu1, cpu_sibling_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_sibling_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_core_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_core_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_llc_shared_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_llc_shared_mask(cpu1));
+	int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+	return !WARN(cpu_to_node(cpu1) != cpu_to_node(cpu2),
+		"sched: CPU #%d's %s-sibling CPU #%d is not on the same node! "
+		"[node: %d != %d]. Ignoring dependency.\n",
+		cpu1, name, cpu2, cpu_to_node(cpu1), cpu_to_node(cpu2));
 }
 
+#define link_mask(_m, c1, c2)						\
+do {									\
+	cpumask_set_cpu((c1), cpu_##_m##_mask(c2));			\
+	cpumask_set_cpu((c2), cpu_##_m##_mask(c1));			\
+} while (0)
 
-void __cpuinit set_cpu_sibling_map(int cpu)
+static bool __cpuinit match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
 {
-	int i;
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
+		int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
 
-	cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
+		if (c->phys_proc_id == o->phys_proc_id &&
+		    per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2) &&
+		    c->compute_unit_id == o->compute_unit_id)
+			return topology_sane(c, o, "smt");
 
-	if (smp_num_siblings > 1) {
-		for_each_cpu(i, cpu_sibling_setup_mask) {
-			struct cpuinfo_x86 *o = &cpu_data(i);
+	} else if (c->phys_proc_id == o->phys_proc_id &&
+		   c->cpu_core_id == o->cpu_core_id) {
+		return topology_sane(c, o, "smt");
+	}
 
-			if (cpu_to_node(cpu) != cpu_to_node(i)) {
-				WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-				continue;
-			}
+	return false;
+}
 
-			if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
-				if (c->phys_proc_id == o->phys_proc_id &&
-				    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i) &&
-				    c->compute_unit_id == o->compute_unit_id)
-					link_thread_siblings(cpu, i);
-			} else if (c->phys_proc_id == o->phys_proc_id &&
-				   c->cpu_core_id == o->cpu_core_id) {
-				link_thread_siblings(cpu, i);
-			}
-		}
-	} else {
-		cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
-	}
+static bool __cpuinit match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+	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 false;
+}
+
+static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (c->phys_proc_id == o->phys_proc_id)
+		return topology_sane(c, o, "mc");
+
+	return false;
+}
+
+void __cpuinit set_cpu_sibling_map(int cpu)
+{
+	bool has_mc = boot_cpu_data.x86_max_cores > 1;
+	bool has_smt = smp_num_siblings > 1;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	struct cpuinfo_x86 *o;
+	int i;
 
-	cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+	cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
 
-	if (__this_cpu_read(cpu_info.x86_max_cores) == 1) {
-		cpumask_copy(cpu_core_mask(cpu), cpu_sibling_mask(cpu));
+	if (!has_smt && !has_mc) {
+		cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_core_mask(cpu));
 		c->booted_cores = 1;
 		return;
 	}
 
 	for_each_cpu(i, cpu_sibling_setup_mask) {
-		if (cpu_to_node(cpu) != cpu_to_node(i)) {
-			WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-			continue;
-		}
+		o = &cpu_data(i);
 
-		if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
-		    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {
-			cpumask_set_cpu(i, cpu_llc_shared_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_llc_shared_mask(i));
-		}
+		if ((i == cpu) || (has_smt && match_smt(c, o)))
+			link_mask(sibling, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_llc(c, o)))
+			link_mask(llc_shared, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_mc(c, o))) {
+			link_mask(core, cpu, i);
 
-		if (c->phys_proc_id == cpu_data(i).phys_proc_id) {
-			cpumask_set_cpu(i, cpu_core_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_core_mask(i));
 			/*
 			 *  Does this new cpu bringup a new core?
 			 */
#5 lore raw
Quoted 140 lines · Peter Zijlstra

OK here's one that compiles and boots without silly warnings on a WSM-EP

---
 arch/x86/kernel/smpboot.c |  112 ++++++++++++++++++++++++++-------------------

 1 file changed, 66 insertions(+), 46 deletions(-)
arch/x86/kernel/smpboot.c +63 -43
index e543e02..e8bf636 100644
@@ -299,70 +299,90 @@ void __cpuinit smp_store_cpu_info(int id)
                identify_secondary_cpu(c);
  }
 
-static void __cpuinit link_thread_siblings(int cpu1, int cpu2)
+static bool __cpuinit
+topology_sane(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o, const char *name)
  {
-       cpumask_set_cpu(cpu1, cpu_sibling_mask(cpu2));
-       cpumask_set_cpu(cpu2, cpu_sibling_mask(cpu1));
-       cpumask_set_cpu(cpu1, cpu_core_mask(cpu2));
-       cpumask_set_cpu(cpu2, cpu_core_mask(cpu1));
-       cpumask_set_cpu(cpu1, cpu_llc_shared_mask(cpu2));
-       cpumask_set_cpu(cpu2, cpu_llc_shared_mask(cpu1));
+       int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+       return !WARN(cpu_to_node(cpu1) != cpu_to_node(cpu2),
+               "sched: CPU #%d's %s-sibling CPU #%d is not on the same node! "
+               "[node: %d != %d]. Ignoring dependency.\n",
+               cpu1, name, cpu2, cpu_to_node(cpu1), cpu_to_node(cpu2));
  }
 
+#define link_mask(_m, c1, c2)                                          \
+do {                                                                   \
+       cpumask_set_cpu((c1), cpu_##_m##_mask(c2));                     \
+       cpumask_set_cpu((c2), cpu_##_m##_mask(c1));                     \
+} while (0)
 
-void __cpuinit set_cpu_sibling_map(int cpu)
+static bool __cpuinit match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
  {
-       int i;
-       struct cpuinfo_x86 *c = &cpu_data(cpu);
+       if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
+               int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
 
-       cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
+               if (c->phys_proc_id == o->phys_proc_id &&
+                   per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2) &&
+                   c->compute_unit_id == o->compute_unit_id)
+                       return topology_sane(c, o, "smt");
 
-       if (smp_num_siblings > 1) {
-               for_each_cpu(i, cpu_sibling_setup_mask) {
-                       struct cpuinfo_x86 *o = &cpu_data(i);
+       } else if (c->phys_proc_id == o->phys_proc_id &&
+                  c->cpu_core_id == o->cpu_core_id) {
+               return topology_sane(c, o, "smt");
+       }
 
-                       if (cpu_to_node(cpu) != cpu_to_node(i)) {
-                               WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-                               continue;
-                       }
+       return false;
+}
 
-                       if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
-                               if (c->phys_proc_id == o->phys_proc_id &&
-                                   per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i) &&
-                                   c->compute_unit_id == o->compute_unit_id)
-                                       link_thread_siblings(cpu, i);
-                       } else if (c->phys_proc_id == o->phys_proc_id &&
-                                  c->cpu_core_id == o->cpu_core_id) {
-                               link_thread_siblings(cpu, i);
-                       }
-               }
-       } else {
-               cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
-       }
+static bool __cpuinit match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+       int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+       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 false;
+}
+
+static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+       if (c->phys_proc_id == o->phys_proc_id)
+               return topology_sane(c, o, "mc");
+
+       return false;
+}
+
+void __cpuinit set_cpu_sibling_map(int cpu)
+{
+       bool has_mc = boot_cpu_data.x86_max_cores > 1;
+       bool has_smt = smp_num_siblings > 1;
+       struct cpuinfo_x86 *c = &cpu_data(cpu);
+       struct cpuinfo_x86 *o;
+       int i;
 
-       cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+       cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
 
-       if (__this_cpu_read(cpu_info.x86_max_cores) == 1) {
-               cpumask_copy(cpu_core_mask(cpu), cpu_sibling_mask(cpu));
+       if (!has_smt && !has_mc) {
+               cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
+               cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+               cpumask_set_cpu(cpu, cpu_core_mask(cpu));
                c->booted_cores = 1;
                return;
        }
 
        for_each_cpu(i, cpu_sibling_setup_mask) {
-               if (cpu_to_node(cpu) != cpu_to_node(i)) {
-                       WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-                       continue;
-               }
+               o = &cpu_data(i);
 
-               if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
-                   per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {
-                       cpumask_set_cpu(i, cpu_llc_shared_mask(cpu));
-                       cpumask_set_cpu(cpu, cpu_llc_shared_mask(i));
-               }
+               if ((i == cpu) || (has_smt && match_smt(c, o)))
+                       link_mask(sibling, cpu, i);
+
+               if ((i == cpu) || (has_mc && match_llc(c, o)))
+                       link_mask(llc_shared, cpu, i);

arch/x86/kernel/smpboot.c void __cpuinit smp_store_cpu_info(int id) · line 388

Quoted 10 lines
arch/x86/kernel/smpboot.c +3 -3
+
+               if ((i == cpu) || (has_mc && match_mc(c, o))) {
+                       link_mask(core, cpu, i);
 
-               if (c->phys_proc_id == cpu_data(i).phys_proc_id) {
-                       cpumask_set_cpu(i, cpu_core_mask(cpu));
-                       cpumask_set_cpu(cpu, cpu_core_mask(i));
                        /*
                         *  Does this new cpu bringup a new core?
                         */

Yes, this fixes the wrong warning go away.

Thanks

Yinghai Lu

#6 lore raw
In reply to Peter Zijlstra
Quoted 5 lines · Peter Zijlstra

OK here's one that compiles and boots without silly warnings on a WSM-EP

---

I've tried Ingo's original patch on a KVM guest that has 'numa=fake=10', and saw two of these warnings mentioned above show up.

I've tried applying Peter's patch on top, but now I get 24 such warnings instead.

#7 lore raw
Quoted 5 lines · Sasha Levin
Quoted 4 lines · Peter Zijlstra

OK here's one that compiles and boots without silly warnings on a WSM-EP

Quoted 7 lines
Quoted 1 line

---

I've tried Ingo's original patch on a KVM guest that has 'numa=fake=10', and saw two of these warnings mentioned above show up.

I've tried applying Peter's patch on top, but now I get 24 such warnings instead.

Yeah, I should probably make that WARN_ONCE()

---
Subject: x86: Rewrite set_cpu_sibling_map
From: Peter Zijlstra <[email protected]>
Date: Fri May 11 13:05:59 CEST 2012

Commit ad7687dde ("x86/numa: Check for nonsensical topologies on real hw as well") is broken in that the condition can trigger for valid setups but only changes the end result for invalid setups with no real means of discerning between those.

Rewrite set_cpu_sibling_map() to make the code clearer and make sure to only warn when the check changes the end result.

---
 arch/x86/kernel/smpboot.c |  116 ++++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 48 deletions(-)
arch/x86/kernel/smpboot.c +68 -48
@@ -299,70 +299,90 @@ void __cpuinit smp_store_cpu_info(int id
 		identify_secondary_cpu(c);
 }
 
-static void __cpuinit link_thread_siblings(int cpu1, int cpu2)
+static bool __cpuinit
+topology_sane(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o, const char *name)
 {
-	cpumask_set_cpu(cpu1, cpu_sibling_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_sibling_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_core_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_core_mask(cpu1));
-	cpumask_set_cpu(cpu1, cpu_llc_shared_mask(cpu2));
-	cpumask_set_cpu(cpu2, cpu_llc_shared_mask(cpu1));
+	int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+	return !WARN_ONCE(cpu_to_node(cpu1) != cpu_to_node(cpu2),
+		"sched: CPU #%d's %s-sibling CPU #%d is not on the same node! "
+		"[node: %d != %d]. Ignoring dependency.\n",
+		cpu1, name, cpu2, cpu_to_node(cpu1), cpu_to_node(cpu2));
 }
 
+#define link_mask(_m, c1, c2)						\
+do {									\
+	cpumask_set_cpu((c1), cpu_##_m##_mask(c2));			\
+	cpumask_set_cpu((c2), cpu_##_m##_mask(c1));			\
+} while (0)
+
+static bool __cpuinit match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
+		int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+		if (c->phys_proc_id == o->phys_proc_id &&
+		    per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2) &&
+		    c->compute_unit_id == o->compute_unit_id)
+			return topology_sane(c, o, "smt");
+
+	} else if (c->phys_proc_id == o->phys_proc_id &&
+		   c->cpu_core_id == o->cpu_core_id) {
+		return topology_sane(c, o, "smt");
+	}
+
+	return false;
+}
+
+static bool __cpuinit match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+
+	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 false;
+}
+
+static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+	if (c->phys_proc_id == o->phys_proc_id)
+		return topology_sane(c, o, "mc");
+
+	return false;
+}
 
 void __cpuinit set_cpu_sibling_map(int cpu)
 {
-	int i;
+	bool has_mc = boot_cpu_data.x86_max_cores > 1;
+	bool has_smt = smp_num_siblings > 1;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	struct cpuinfo_x86 *o;
+	int i;
 
 	cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
 
-	if (smp_num_siblings > 1) {
-		for_each_cpu(i, cpu_sibling_setup_mask) {
-			struct cpuinfo_x86 *o = &cpu_data(i);
-
-			if (cpu_to_node(cpu) != cpu_to_node(i)) {
-				WARN_ONCE(1, "sched: CPU #%d's thread-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-				continue;
-			}
-
-			if (cpu_has(c, X86_FEATURE_TOPOEXT)) {
-				if (c->phys_proc_id == o->phys_proc_id &&
-				    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i) &&
-				    c->compute_unit_id == o->compute_unit_id)
-					link_thread_siblings(cpu, i);
-			} else if (c->phys_proc_id == o->phys_proc_id &&
-				   c->cpu_core_id == o->cpu_core_id) {
-				link_thread_siblings(cpu, i);
-			}
-		}
-	} else {
+	if (!has_smt && !has_mc) {
 		cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
-	}
-
-	cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
-
-	if (__this_cpu_read(cpu_info.x86_max_cores) == 1) {
-		cpumask_copy(cpu_core_mask(cpu), cpu_sibling_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
+		cpumask_set_cpu(cpu, cpu_core_mask(cpu));
 		c->booted_cores = 1;
 		return;
 	}
 
 	for_each_cpu(i, cpu_sibling_setup_mask) {
-		if (cpu_to_node(cpu) != cpu_to_node(i)) {
-			WARN_ONCE(1, "sched: CPU #%d's core-sibling CPU #%d not on the same node! [node %d != %d]. Ignoring sibling dependency.\n", cpu, i, cpu_to_node(cpu), cpu_to_node(i));
-			continue;
-		}
-
-		if (per_cpu(cpu_llc_id, cpu) != BAD_APICID &&
-		    per_cpu(cpu_llc_id, cpu) == per_cpu(cpu_llc_id, i)) {
-			cpumask_set_cpu(i, cpu_llc_shared_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_llc_shared_mask(i));
-		}
-
-		if (c->phys_proc_id == cpu_data(i).phys_proc_id) {
-			cpumask_set_cpu(i, cpu_core_mask(cpu));
-			cpumask_set_cpu(cpu, cpu_core_mask(i));
+		o = &cpu_data(i);
+
+		if ((i == cpu) || (has_smt && match_smt(c, o)))
+			link_mask(sibling, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_llc(c, o)))
+			link_mask(llc_shared, cpu, i);
+
+		if ((i == cpu) || (has_mc && match_mc(c, o))) {
+			link_mask(core, cpu, i);
+
 			/*
 			 *  Does this new cpu bringup a new core?
 			 */