diff mbox series

[v2] NUMA: Early use of cpu_to_node() returns 0 instead of the correct node id

Message ID 20240123045843.75969-1-shijie@os.amperecomputing.com
State New
Headers
Series [v2] NUMA: Early use of cpu_to_node() returns 0 instead of the correct node id |

Commit Message

Huang Shijie Jan. 23, 2024, 4:58 a.m. UTC 
  During the kernel booting, the generic cpu_to_node() is called too early in
arm64, powerpc and riscv when CONFIG_NUMA is enabled.

For arm64/powerpc/riscv, there are at least four places in the common code
where the generic cpu_to_node() is called before it is initialized:
	   1.) early_trace_init()         in kernel/trace/trace.c
	   2.) sched_init()               in kernel/sched/core.c
	   3.) init_sched_fair_class()    in kernel/sched/fair.c
	   4.) workqueue_init_early()     in kernel/workqueue.c

In order to fix the bug, the patch changes generic cpu_to_node to
function pointer, and export it for kernel modules.
Introduce smp_prepare_boot_cpu_start() to wrap the original
smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
and set the cpu_to_node to formal _cpu_to_node().

Signed-off-by: Huang Shijie <shijie@os.amperecomputing.com>
---
v1 --> v2:
	In order to fix the x86 compiling error, move the cpu_to_node()
       	from driver/base/arch_numa.c to driver/base/node.c.

	v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2024-January/896160.html

	An old different title patch:
	http://lists.infradead.org/pipermail/linux-arm-kernel/2024-January/895963.html

---
 drivers/base/node.c      | 11 +++++++++++
 include/linux/topology.h |  6 ++----
 init/main.c              | 29 +++++++++++++++++++++++++++--
 3 files changed, 40 insertions(+), 6 deletions(-)

Comments

Lameter, Christopher Jan. 24, 2024, 5:19 p.m. UTC | #1 
On Tue, 23 Jan 2024, Huang Shijie wrote:

> During the kernel booting, the generic cpu_to_node() is called too early in
> arm64, powerpc and riscv when CONFIG_NUMA is enabled.
>
> For arm64/powerpc/riscv, there are at least four places in the common code
> where the generic cpu_to_node() is called before it is initialized:
> 	   1.) early_trace_init()         in kernel/trace/trace.c
> 	   2.) sched_init()               in kernel/sched/core.c
> 	   3.) init_sched_fair_class()    in kernel/sched/fair.c
> 	   4.) workqueue_init_early()     in kernel/workqueue.c
>
> In order to fix the bug, the patch changes generic cpu_to_node to
> function pointer, and export it for kernel modules.
> Introduce smp_prepare_boot_cpu_start() to wrap the original
> smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
> Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
> and set the cpu_to_node to formal _cpu_to_node().

Would  you please fix this cleanly without a function pointer?

What I think needs to be done is a patch series.

1. Instrument cpu_to_node so that some warning is issued if it is used too 
early. Preloading the array with NUMA_NO_NODE would allow us to do that.

2. Implement early_cpu_to_node on platforms that currently do not have it.

3. A series of patches that fix each place where cpu_to_node is used too 
early.
Yury Norov Jan. 24, 2024, 5:41 p.m. UTC | #2 
On Wed, Jan 24, 2024 at 09:19:00AM -0800, Lameter, Christopher wrote:
> On Tue, 23 Jan 2024, Huang Shijie wrote:
> 
> > During the kernel booting, the generic cpu_to_node() is called too early in
> > arm64, powerpc and riscv when CONFIG_NUMA is enabled.
> > 
> > For arm64/powerpc/riscv, there are at least four places in the common code
> > where the generic cpu_to_node() is called before it is initialized:
> > 	   1.) early_trace_init()         in kernel/trace/trace.c
> > 	   2.) sched_init()               in kernel/sched/core.c
> > 	   3.) init_sched_fair_class()    in kernel/sched/fair.c
> > 	   4.) workqueue_init_early()     in kernel/workqueue.c
> > 
> > In order to fix the bug, the patch changes generic cpu_to_node to
> > function pointer, and export it for kernel modules.
> > Introduce smp_prepare_boot_cpu_start() to wrap the original
> > smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
> > Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
> > and set the cpu_to_node to formal _cpu_to_node().
> 
> Would  you please fix this cleanly without a function pointer?
> 
> What I think needs to be done is a patch series.
> 
> 1. Instrument cpu_to_node so that some warning is issued if it is used too
> early. Preloading the array with NUMA_NO_NODE would allow us to do that.

By preloading do you mean compile-time initialization?
 
> 2. Implement early_cpu_to_node on platforms that currently do not have it.
> 
> 3. A series of patches that fix each place where cpu_to_node is used too
> early.

Agree. This is the right way to go. And pretty well all of it was discussed
in v1, isn't?

Thanks,
Yury
Shijie Huang Jan. 25, 2024, 2:42 a.m. UTC | #3 
在 2024/1/25 1:19, Lameter, Christopher 写道:
> On Tue, 23 Jan 2024, Huang Shijie wrote:
>
>> During the kernel booting, the generic cpu_to_node() is called too 
>> early in
>> arm64, powerpc and riscv when CONFIG_NUMA is enabled.
>>
>> For arm64/powerpc/riscv, there are at least four places in the common 
>> code
>> where the generic cpu_to_node() is called before it is initialized:
>>        1.) early_trace_init()         in kernel/trace/trace.c
>>        2.) sched_init()               in kernel/sched/core.c
>>        3.) init_sched_fair_class()    in kernel/sched/fair.c
>>        4.) workqueue_init_early()     in kernel/workqueue.c
>>
>> In order to fix the bug, the patch changes generic cpu_to_node to
>> function pointer, and export it for kernel modules.
>> Introduce smp_prepare_boot_cpu_start() to wrap the original
>> smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
>> Introduce smp_prepare_cpus_done() to wrap the original 
>> smp_prepare_cpus(),
>> and set the cpu_to_node to formal _cpu_to_node().
>
> Would  you please fix this cleanly without a function pointer?
>
> What I think needs to be done is a patch series.
>
> 1. Instrument cpu_to_node so that some warning is issued if it is used 
> too early. Preloading the array with NUMA_NO_NODE would allow us to do 
> that.
>
> 2. Implement early_cpu_to_node on platforms that currently do not have 
> it.
>
> 3. A series of patches that fix each place where cpu_to_node is used 
> too early.

okay, I will try to do it.


Thanks

Huang Shijie
Mike Rapoport Jan. 25, 2024, 7:31 a.m. UTC | #4 
On Wed, Jan 24, 2024 at 09:19:00AM -0800, Lameter, Christopher wrote:
> On Tue, 23 Jan 2024, Huang Shijie wrote:
> 
> > During the kernel booting, the generic cpu_to_node() is called too early in
> > arm64, powerpc and riscv when CONFIG_NUMA is enabled.
> > 
> > For arm64/powerpc/riscv, there are at least four places in the common code
> > where the generic cpu_to_node() is called before it is initialized:
> > 	   1.) early_trace_init()         in kernel/trace/trace.c
> > 	   2.) sched_init()               in kernel/sched/core.c
> > 	   3.) init_sched_fair_class()    in kernel/sched/fair.c
> > 	   4.) workqueue_init_early()     in kernel/workqueue.c
> > 
> > In order to fix the bug, the patch changes generic cpu_to_node to
> > function pointer, and export it for kernel modules.
> > Introduce smp_prepare_boot_cpu_start() to wrap the original
> > smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
> > Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
> > and set the cpu_to_node to formal _cpu_to_node().
> 
> Would  you please fix this cleanly without a function pointer?
> 
> What I think needs to be done is a patch series.
> 
> 1. Instrument cpu_to_node so that some warning is issued if it is used too
> early. Preloading the array with NUMA_NO_NODE would allow us to do that.
> 
> 2. Implement early_cpu_to_node on platforms that currently do not have it.
> 
> 3. A series of patches that fix each place where cpu_to_node is used too
> early.

I think step 3 can be simplified with a generic function that sets
per_cpu(numa_node) using early_cpu_to_node(). It can be called right after
setup_per_cpu_areas().
Shijie Huang Jan. 25, 2024, 9:15 a.m. UTC | #5 
在 2024/1/25 15:31, Mike Rapoport 写道:
> On Wed, Jan 24, 2024 at 09:19:00AM -0800, Lameter, Christopher wrote:
>> On Tue, 23 Jan 2024, Huang Shijie wrote:
>>
>>> During the kernel booting, the generic cpu_to_node() is called too early in
>>> arm64, powerpc and riscv when CONFIG_NUMA is enabled.
>>>
>>> For arm64/powerpc/riscv, there are at least four places in the common code
>>> where the generic cpu_to_node() is called before it is initialized:
>>> 	   1.) early_trace_init()         in kernel/trace/trace.c
>>> 	   2.) sched_init()               in kernel/sched/core.c
>>> 	   3.) init_sched_fair_class()    in kernel/sched/fair.c
>>> 	   4.) workqueue_init_early()     in kernel/workqueue.c
>>>
>>> In order to fix the bug, the patch changes generic cpu_to_node to
>>> function pointer, and export it for kernel modules.
>>> Introduce smp_prepare_boot_cpu_start() to wrap the original
>>> smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
>>> Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
>>> and set the cpu_to_node to formal _cpu_to_node().
>> Would  you please fix this cleanly without a function pointer?
>>
>> What I think needs to be done is a patch series.
>>
>> 1. Instrument cpu_to_node so that some warning is issued if it is used too
>> early. Preloading the array with NUMA_NO_NODE would allow us to do that.
>>
>> 2. Implement early_cpu_to_node on platforms that currently do not have it.
>>
>> 3. A series of patches that fix each place where cpu_to_node is used too
>> early.

For step 3, I find it it hard to change the cpu_to_node() to 
early_cpu_to_node() for early_trace_init().

In early_trace_init(), the __ring_buffer_alloc() calls the cpu_to_node().

In order to fix the bug, we should use early_cpu_to_node() for 
__ring_buffer_alloc().

But __ring_buffer_alloc() is also used by the kernel after the booting 
finished.

After the booting finishes, we should use the cpu_to_node(), not the 
early_cpu_to_node().

> I think step 3 can be simplified with a generic function that sets
> per_cpu(numa_node) using early_cpu_to_node(). It can be called right after
> setup_per_cpu_areas().

I think this method maybe better..

I will try this too.


Thanks

Huang Shijie
diff mbox series

Patch

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 1c05640461dd..477d58c12886 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -976,3 +976,14 @@  void __init node_dev_init(void)
 			panic("%s() failed to add node: %d\n", __func__, ret);
 	}
 }
+
+#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
+#ifndef cpu_to_node
+int _cpu_to_node(int cpu)
+{
+	return per_cpu(numa_node, cpu);
+}
+int (*cpu_to_node)(int cpu);
+EXPORT_SYMBOL(cpu_to_node);
+#endif
+#endif
diff --git a/include/linux/topology.h b/include/linux/topology.h
index 52f5850730b3..e7ce2bae11dd 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -91,10 +91,8 @@  static inline int numa_node_id(void)
 #endif
 
 #ifndef cpu_to_node
-static inline int cpu_to_node(int cpu)
-{
-	return per_cpu(numa_node, cpu);
-}
+extern int (*cpu_to_node)(int cpu);
+extern int _cpu_to_node(int cpu);
 #endif
 
 #ifndef set_numa_node
diff --git a/init/main.c b/init/main.c
index e24b0780fdff..b142e9c51161 100644
--- a/init/main.c
+++ b/init/main.c
@@ -870,6 +870,18 @@  static void __init print_unknown_bootoptions(void)
 	memblock_free(unknown_options, len);
 }
 
+static void __init smp_prepare_boot_cpu_start(void)
+{
+	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
+
+#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
+#ifndef cpu_to_node
+	/* The early_cpu_to_node should be ready now. */
+	cpu_to_node = early_cpu_to_node;
+#endif
+#endif
+}
+
 asmlinkage __visible __init __no_sanitize_address __noreturn __no_stack_protector
 void start_kernel(void)
 {
@@ -899,7 +911,7 @@  void start_kernel(void)
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
 	setup_per_cpu_areas();
-	smp_prepare_boot_cpu();	/* arch-specific boot-cpu hooks */
+	smp_prepare_boot_cpu_start();
 	boot_cpu_hotplug_init();
 
 	pr_notice("Kernel command line: %s\n", saved_command_line);
@@ -1519,6 +1531,19 @@  void __init console_on_rootfs(void)
 	fput(file);
 }
 
+static void __init smp_prepare_cpus_done(unsigned int setup_max_cpus)
+{
+	/* Different ARCHs may override smp_prepare_cpus() */
+	smp_prepare_cpus(setup_max_cpus);
+
+#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
+#ifndef cpu_to_node
+	/* Change to the formal function. */
+	cpu_to_node = _cpu_to_node;
+#endif
+#endif
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/* Now the scheduler is fully set up and can do blocking allocations */
@@ -1531,7 +1556,7 @@  static noinline void __init kernel_init_freeable(void)
 
 	cad_pid = get_pid(task_pid(current));
 
-	smp_prepare_cpus(setup_max_cpus);
+	smp_prepare_cpus_done(setup_max_cpus);
 
 	workqueue_init();