Re: Linux 2.6.39-rc4 (regression: NUMA on multi-node CPUs broken)

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

> Following patch breaks real NUMA on multi-node CPUs like AMD
> Magny-Cours and should be reverted (or changed to just take effect in
> case of numa=fake):
> 
>   commit 7d6b46707f2491a94f4bd3b4329d2d7f809e9368
>   Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>   Date:   Fri Apr 15 20:39:01 2011 +0900
> 
>     x86, NUMA: Fix fakenuma boot failure
> 
>     ...
> 
>     Thus, this patch implements a reassignment of node-ids if buggy firmware
>     or numa emulation makes wrong cpu node map. Tt enforce all logical cpus
>     in the same physical cpu share the same node.
> 
>     ...
> 
>   +static void __cpuinit check_cpu_siblings_on_same_node(int cpu1, int cpu2)
>   +{
>   +       int node1 = early_cpu_to_node(cpu1);
>   +       int node2 = early_cpu_to_node(cpu2);
>   +
>   +       /*
>   +        * Our CPU scheduler assumes all logical cpus in the same physical cpu
>   +        * share the same node. But, buggy ACPI or NUMA emulation might assign
>   +        * them to different node. Fix it.
>   +        */
> 
>    ...
> 
> This is a false assumption. Magny-Cours has two nodes in the same
> physical package. The scheduler was (kind of) fixed to work around
> this boot problem for multi-node CPUs (with 2.6.32). 

I agree we have to fix this ASAP. I also think we have to avoid reintroduce 
the same again. Can you please tell me the commit-id of this one? 

> If this is also
> an issue with wrong cpu node maps in case of NUMA emulation this might
> be fixed similar or this quirk should only be applied in case of NUMA
> emulation.

Indeed.

Tejun, Do you remember I sent numa emulation specific patch at first. now
I'm beside with Andreas. Because I bet current numa fallback code (you 
pointed out one) has no user. 

Or, please let us know if you have an alternative patch.



Attached revert and fakenuma spefic fix patches.

[-- Attachment #2: 0001-Revert-x86-NUMA-Fix-fakenuma-boot-failure.patch --]
[-- Type: application/octet-stream, Size: 4840 bytes --]

From 8183833bb4b48fdb150f64905b80fd21045946ec Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Thu, 21 Apr 2011 10:32:57 +0900
Subject: [PATCH 1/2] Revert "x86, NUMA: Fix fakenuma boot failure"

This reverts commit 7d6b46707f2491a94f4bd3b4329d2d7f809e9368.
Andreas Herrmann reported the patch breaks AMD Mangy-Cours because
Magny-Cours has two nodes in the same physical package.

He said,
:With this patch Linux shows
:
:   root # numactl  --hardware
:   available: 8 nodes (0-7)
:   node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11
:   node 0 size: 8189 MB
:   node 0 free: 7937 MB
:   node 1 cpus:
:   node 1 size: 16384 MB
:   node 1 free: 16129 MB
:   node 2 cpus: 12 13 14 15 16 17 18 19 20 21 22 23
:   node 2 size: 8192 MB
:   node 2 free: 8024 MB
:   node 3 cpus:
:   node 3 size: 16384 MB
:   node 3 free: 16129 MB
:   node 4 cpus: 24 25 26 27 28 29 30 31 32 33 34 35
:   node 4 size: 8192 MB
:   node 4 free: 8013 MB
:   node 5 cpus:
:   node 5 size: 16384 MB
:   node 5 free: 16129 MB
:   node 6 cpus: 36 37 38 39 40 41 42 43 44 45 46 47
:   node 6 size: 8192 MB
:   node 6 free: 8025 MB
:   node 7 cpus:
:   node 7 size: 16384 MB
:   node 7 free: 16128 MB
:   node distances:
:   node   0   1   2   3   4   5   6   7
:     0:  10  16  16  22  16  22  16  22
:     1:  16  10  22  16  16  22  22  16
:     2:  16  22  10  16  16  16  16  16
:     3:  22  16  16  10  16  16  22  22
:     4:  16  16  16  16  10  16  16  22
:     5:  22  22  16  16  16  10  22  16
:     6:  16  22  16  22  16  22  10  16
:     7:  22  16  16  22  22  16  16  10
:
:which is bogus. The correct NUMA-information (based on SRAT) (w/o this
:patch) is
:
:    linux # numactl --hardware
:   available: 8 nodes (0-7)
:   node 0 cpus: 0 1 2 3 4 5
:   node 0 size: 8189 MB
:   node 0 free: 7947 MB
:   node 1 cpus: 6 7 8 9 10 11
:   node 1 size: 16384 MB
:   node 1 free: 16114 MB
:   node 2 cpus: 12 13 14 15 16 17
:   node 2 size: 8192 MB
:   node 2 free: 7941 MB
:   node 3 cpus: 18 19 20 21 22 23
:   node 3 size: 16384 MB
:   node 3 free: 16120 MB
:   node 4 cpus: 24 25 26 27 28 29
:   node 4 size: 8192 MB
:   node 4 free: 8028 MB
:   node 5 cpus: 30 31 32 33 34 35
:   node 5 size: 16384 MB
:   node 5 free: 16116 MB
:   node 6 cpus: 36 37 38 39 40 41
:   node 6 size: 8192 MB
:   node 6 free: 8033 MB
:   node 7 cpus: 42 43 44 45 46 47
:   node 7 size: 16384 MB
:   node 7 free: 16120 MB
:   node distances:
:   node   0   1   2   3   4   5   6   7
:     0:  10  16  16  22  16  22  16  22
:     1:  16  10  22  16  16  22  22  16
:     2:  16  22  10  16  16  16  16  16
:     3:  22  16  16  10  16  16  22  22
:     4:  16  16  16  16  10  16  16  22
:     5:  22  22  16  16  16  10  22  16
:     6:  16  22  16  22  16  22  10  16
:     7:  22  16  16  22  22  16  16  10

We need another and less intrusive patch.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 arch/x86/kernel/smpboot.c |   23 -----------------------
 1 files changed, 0 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8ed8908..c2871d3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -312,26 +312,6 @@ void __cpuinit smp_store_cpu_info(int id)
 		identify_secondary_cpu(c);
 }
 
-static void __cpuinit check_cpu_siblings_on_same_node(int cpu1, int cpu2)
-{
-	int node1 = early_cpu_to_node(cpu1);
-	int node2 = early_cpu_to_node(cpu2);
-
-	/*
-	 * Our CPU scheduler assumes all logical cpus in the same physical cpu
-	 * share the same node. But, buggy ACPI or NUMA emulation might assign
-	 * them to different node. Fix it.
-	 */
-	if (node1 != node2) {
-		pr_warning("CPU %d in node %d and CPU %d in node %d are in the same physical CPU. forcing same node %d\n",
-			   cpu1, node1, cpu2, node2, node2);
-
-		numa_remove_cpu(cpu1);
-		numa_set_node(cpu1, node2);
-		numa_add_cpu(cpu1);
-	}
-}
-
 static void __cpuinit link_thread_siblings(int cpu1, int cpu2)
 {
 	cpumask_set_cpu(cpu1, cpu_sibling_mask(cpu2));
@@ -340,7 +320,6 @@ static void __cpuinit link_thread_siblings(int cpu1, int 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));
-	check_cpu_siblings_on_same_node(cpu1, cpu2);
 }
 
 
@@ -382,12 +361,10 @@ void __cpuinit set_cpu_sibling_map(int cpu)
 		    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));
-			check_cpu_siblings_on_same_node(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));
-			check_cpu_siblings_on_same_node(cpu, i);
 			/*
 			 *  Does this new cpu bringup a new core?
 			 */
-- 
1.7.3.1


[-- Attachment #3: 0002-x86-64-NUMA-reimplement-cpu-node-map-initialization-.patch --]
[-- Type: application/octet-stream, Size: 3777 bytes --]

From 209e31ed67190c82f33c00769095b241dde11a6b Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Fri, 8 Apr 2011 23:23:50 +0900
Subject: [PATCH 2/2] x86-64, NUMA: reimplement cpu node map initialization for fake numa.

Currently, numa=fake boot parameter is broken. If it's used, kernel
may panic due to devide by zero error depending on CPU configuration

Call Trace:
 [<ffffffff8104ad4c>] find_busiest_group+0x38c/0xd30
 [<ffffffff81086aff>] ? local_clock+0x6f/0x80
 [<ffffffff81050533>] load_balance+0xa3/0x600
 [<ffffffff81050f53>] idle_balance+0xf3/0x180
 [<ffffffff81550092>] schedule+0x722/0x7d0
 [<ffffffff81550538>] ? wait_for_common+0x128/0x190
 [<ffffffff81550a65>] schedule_timeout+0x265/0x320
 [<ffffffff81095815>] ? lock_release_holdtime+0x35/0x1a0
 [<ffffffff81550538>] ? wait_for_common+0x128/0x190
 [<ffffffff8109bb6c>] ? __lock_release+0x9c/0x1d0
 [<ffffffff815534e0>] ? _raw_spin_unlock_irq+0x30/0x40
 [<ffffffff815534e0>] ? _raw_spin_unlock_irq+0x30/0x40
 [<ffffffff81550540>] wait_for_common+0x130/0x190
 [<ffffffff81051920>] ? try_to_wake_up+0x510/0x510
 [<ffffffff8155067d>] wait_for_completion+0x1d/0x20
 [<ffffffff8107f36c>] kthread_create_on_node+0xac/0x150
 [<ffffffff81077bb0>] ? process_scheduled_works+0x40/0x40
 [<ffffffff8155045f>] ? wait_for_common+0x4f/0x190
 [<ffffffff8107a283>] __alloc_workqueue_key+0x1a3/0x590
 [<ffffffff81e0cce2>] cpuset_init_smp+0x6b/0x7b
 [<ffffffff81df3d07>] kernel_init+0xc3/0x182
 [<ffffffff8155d5e4>] kernel_thread_helper+0x4/0x10
 [<ffffffff81553cd4>] ? retint_restore_args+0x13/0x13
 [<ffffffff81df3c44>] ? start_kernel+0x400/0x400
 [<ffffffff8155d5e0>] ? gs_change+0x13/0x13

The divede by zero is caused following line. (ie group->cpu_power==0)

kernel/sched_fair.c::update_sg_lb_stats()
        /* Adjust by relative CPU power of the group */
        sgs->avg_load = (sgs->group_load * SCHED_LOAD_SCALE) /
group->cpu_power;

This is regression by commit e23bba6044 (x86-64, NUMA: Unify emulated
distance mapping) because it changes cpu -> node mapping in the process
of dropping fake_physnodes().

  old) all cpus are assinged node 0
  now) cpus are assigned round robin
       (the logic is implemented by numa_init_array())

  Note: The change is heppen only if the system doesn't have neigher
    ACPI srat table nor AMD northbridge NUMA information.

Why round robin assignment doesn't work? Because init_numa_sched_groups_power()
assumes all logical cpus in the same physical cpu share the same node
(Then it only accounts for group_first_cpu()), and the simple round
robin breaks the above assumption.

This patch enforce all cpus use node 0 if fake numa is enabled as same
as v2.6.38 or priorer.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Shaohui Zheng <shaohui.zheng@intel.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/mm/numa_emulation.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
index ad091e4..4fda351 100644
--- a/arch/x86/mm/numa_emulation.c
+++ b/arch/x86/mm/numa_emulation.c
@@ -419,6 +419,14 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
 	/* free the copied physical distance table */
 	if (phys_dist)
 		memblock_x86_free_range(__pa(phys_dist), __pa(phys_dist) + phys_size);
+
+	/* Setup cpu node map. */
+	for (i = 0; i < nr_cpu_ids; i++) {
+		if (early_cpu_to_node(i) != NUMA_NO_NODE)
+			continue;
+		numa_set_node(i, 0);
+	}
+
 	return;
 
 no_emu:
-- 
1.7.3.1