Skip to contentSkip to conversation

x86, cpu: Fix node state for whether it contains CPU

PATCH Superseded Tim Chen 2 participants 3 messages Aug 24, 2016 – Aug 29, 2016 x86

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Tim Chen

author
#0 lore raw

PATCH 1/1 x86, cpu: Fix node state for whether it contains CPU

In current kernel code, we only call node_set_state(cpu_to_node(cpu), N_CPU) when a cpu is hot plugged. But we do not set the node state for N_CPU when the cpus are brought online during boot.

So this could lead to failure when we check to see if a node contains cpu with node_state(node_id, N_CPU).

One use case is in the node_reclaime function:

/*
* Only run node reclaim on the local node or on nodes that do
* not
* have associated processors. This will favor the local
* processor
* over remote processors and spread off node memory allocations
* as wide as possible.
*/
if (node_state(pgdat->node_id, N_CPU) && pgdat->node_id !=
numa_node_id())
return NODE_RECLAIM_NOSCAN;

I instrumented the kernel to call this function after boot and it always returns 0 on a x86 desktop machine until I apply the attached patch.

static int num_cpu_node(void)

{
       int i, nr_cpu_nodes = 0;

       for_each_node(i) {
               if (node_state(i, N_CPU))
                       ++ nr_cpu_nodes;
       }

       return nr_cpu_nodes;
}

I have not tested other architectues but they are likely to have similar issue.

---
 arch/x86/kernel/smpboot.c | 1 +
 1 file changed, 1 insertion(+)
arch/x86/kernel/smpboot.c +1 -0
index d8f7d01..04c0574 100644
@@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
 	lock_vector_lock();
 	setup_vector_irq(smp_processor_id());
 	set_cpu_online(smp_processor_id(), true);
+	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
 	unlock_vector_lock();
 	cpu_set_state_online(smp_processor_id());
 	x86_platform.nmi_init();
Signature · 1 line
2.5.5
#1 lore raw
Quoted 50 lines · Tim Chen

In current kernel code, we only call node_set_state(cpu_to_node(cpu), N_CPU) when a cpu is hot plugged. But we do not set the node state for N_CPU when the cpus are brought online during boot.

So this could lead to failure when we check to see if a node contains cpu with node_state(node_id, N_CPU).

One use case is in the node_reclaime function:

/*
* Only run node reclaim on the local node or on nodes that do
* not
* have associated processors. This will favor the local
* processor
* over remote processors and spread off node memory allocations
* as wide as possible.
*/
if (node_state(pgdat->node_id, N_CPU) && pgdat->node_id !=
numa_node_id())
return NODE_RECLAIM_NOSCAN;

I instrumented the kernel to call this function after boot and it always returns 0 on a x86 desktop machine until I apply the attached patch.

static int num_cpu_node(void)

{
       int i, nr_cpu_nodes = 0;

       for_each_node(i) {
               if (node_state(i, N_CPU))
                       ++ nr_cpu_nodes;
       }

       return nr_cpu_nodes;
}

I have not tested other architectues but they are likely to have similar issue.

---
 arch/x86/kernel/smpboot.c | 1 +
 1 file changed, 1 insertion(+)
arch/x86/kernel/smpboot.c +0 -0
index d8f7d01..04c0574 100644

arch/x86/kernel/smpboot.c static void notrace start_secondary(void *unused) · line 265

Quoted 7 lines
arch/x86/kernel/smpboot.c +1 -0
@@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
 	lock_vector_lock();
 	setup_vector_irq(smp_processor_id());
 	set_cpu_online(smp_processor_id(), true);
+	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
 	unlock_vector_lock();
 	cpu_set_state_online(smp_processor_id());
 	x86_platform.nmi_init();

Would it not be easier to register the vmstat_notifier earlier, before SMP bringup? Because with this change, we need to go fix all architectures.

author
#2 lore raw
Quoted 8 lines · Peter Zijlstra
Quoted 7 lines · Tim Chen

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

 1 file changed, 1 insertion(+)
Quoted 15 lines

arch/x86/kernel/smpboot.c static void notrace start_secondary(void *unused) · line 265

Quoted 12 lines
arch/x86/kernel/smpboot.c +1 -0
index d8f7d01..04c0574 100644
@@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
  	lock_vector_lock();
  	setup_vector_irq(smp_processor_id());
  	set_cpu_online(smp_processor_id(), true);
+	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
  	unlock_vector_lock();
  	cpu_set_state_online(smp_processor_id());
  	x86_platform.nmi_init();

Would it not be easier to register the vmstat_notifier earlier, before SMP bringup? Because with this change, we need to go fix all architectures.

I think checking all nodes for online cpus when we init vmstat is probably the more straightforward way to go. We can do this after SMP bring up. I'll update the patch for that.

Thanks.

Tim