LinuxLists.cc - [PATCH] init: refactor the generic cpu_to

2024-01-18 03:16:07

Subject: [PATCH] init: refactor the generic cpu_to_node for NUMA

(0) We list the ARCHs which support the NUMA:
arm64, loongarch, powerpc, riscv,
sparc, mips, s390, x86,

(1) Some ARCHs in (0) override the generic cpu_to_node(), such as:
sparc, mips, s390, x86.

Since these ARCHs have their own cpu_to_node(), we do not care
about them.

(2) The ARCHs enable NUMA and use the generic cpu_to_node.
From (0) and (1), we can know that four ARCHs support NUMA and
use the generic cpu_to_node:
arm64, loongarch, powerpc, riscv,

The generic cpu_to_node depends on percpu "numa_node".

(2.1) The loongarch sets "numa_node" in:
start_kernel --> smp_prepare_boot_cpu()

(2.2) The arm64, powerpc, riscv set "numa_node" in:
start_kernel --> arch_call_rest_init() --> rest_init()
--> kernel_init() --> kernel_init_freeable()
--> smp_prepare_cpus()

(2.3) The first place calling the cpu_to_node() is early_trace_init():
start_kernel --> early_trace_init()--> __ring_buffer_alloc()
--> rb_allocate_cpu_buffer()

(2.4) So it safe for loongarch. But for arm64, powerpc and riscv,
there are at least four places in the common code where
the cpu_to_node() is called before it is initialized:
a.) early_trace_init() in kernel/trace/trace.c
b.) sched_init() in kernel/sched/core.c
c.) init_sched_fair_class() in kernel/sched/fair.c
d.) workqueue_init_early() in kernel/workqueue.c

(3) In order to fix the issue, the patch refactors the generic cpu_to_node:
(3.1) change cpu_to_node to function pointer,
and export it for kernel modules.

(3.2) introduce _cpu_to_node() which is the original cpu_to_node().

(3.3) introduce smp_prepare_boot_cpu_start() to wrap the original
smp_prepare_boot_cpu(), and set cpu_to_node with
early_cpu_to_node which works fine for arm64, powerpc,
riscv and loongarch.

(3.4) introduce smp_prepare_cpus_done() to wrap the original
smp_prepare_cpus().
The "numa_node" is ready after smp_prepare_cpus(),
then set cpu_to_node with _cpu_to_node().

Signed-off-by: Huang Shijie <[email protected]>
---
drivers/base/arch_numa.c | 11 +++++++++++
include/linux/topology.h | 6 ++----
init/main.c | 29 +++++++++++++++++++++++++++--
3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
index 5b59d133b6af..867a477fa975 100644
--- a/drivers/base/arch_numa.c
+++ b/drivers/base/arch_numa.c
@@ -61,6 +61,17 @@ EXPORT_SYMBOL(cpumask_of_node);

#endif

+#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
+
static void numa_update_cpu(unsigned int cpu, bool remove)
{
int nid = cpu_to_node(cpu);
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();

--
2.40.1

2024-01-18 09:36:54

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] init: refactor the generic cpu_to_node for NUMA

On Thu, Jan 18, 2024 at 11:14:12AM +0800, Huang Shijie wrote:
> (0) We list the ARCHs which support the NUMA:
> arm64, loongarch, powerpc, riscv,
> sparc, mips, s390, x86,

I do not understand this format, what are you saying here?

Have you read the kernel documentation for how to write changelog texts?
It doesn't say "list a bunch of things", it's a bit more descriptive.

>
> (1) Some ARCHs in (0) override the generic cpu_to_node(), such as:
> sparc, mips, s390, x86.
>
> Since these ARCHs have their own cpu_to_node(), we do not care
> about them.
>
> (2) The ARCHs enable NUMA and use the generic cpu_to_node.
> From (0) and (1), we can know that four ARCHs support NUMA and
> use the generic cpu_to_node:
> arm64, loongarch, powerpc, riscv,
>
> The generic cpu_to_node depends on percpu "numa_node".
>
> (2.1) The loongarch sets "numa_node" in:
> start_kernel --> smp_prepare_boot_cpu()
>
> (2.2) The arm64, powerpc, riscv set "numa_node" in:
> start_kernel --> arch_call_rest_init() --> rest_init()
> --> kernel_init() --> kernel_init_freeable()
> --> smp_prepare_cpus()
>
> (2.3) The first place calling the cpu_to_node() is early_trace_init():
> start_kernel --> early_trace_init()--> __ring_buffer_alloc()
> --> rb_allocate_cpu_buffer()
>
> (2.4) So it safe for loongarch. But for arm64, powerpc and riscv,
> there are at least four places in the common code where
> the cpu_to_node() is called before it is initialized:
> a.) early_trace_init() in kernel/trace/trace.c
> b.) sched_init() in kernel/sched/core.c
> c.) init_sched_fair_class() in kernel/sched/fair.c
> d.) workqueue_init_early() in kernel/workqueue.c
>
> (3) In order to fix the issue, the patch refactors the generic cpu_to_node:
> (3.1) change cpu_to_node to function pointer,
> and export it for kernel modules.
>
> (3.2) introduce _cpu_to_node() which is the original cpu_to_node().
>
> (3.3) introduce smp_prepare_boot_cpu_start() to wrap the original
> smp_prepare_boot_cpu(), and set cpu_to_node with
> early_cpu_to_node which works fine for arm64, powerpc,
> riscv and loongarch.
>
> (3.4) introduce smp_prepare_cpus_done() to wrap the original
> smp_prepare_cpus().
> The "numa_node" is ready after smp_prepare_cpus(),
> then set cpu_to_node with _cpu_to_node().

When you start listing different things in a changelog, that's a hint to
the reviewer to say "please break this up" as patches need to do only
one thing at a time. As I can't follow the above text at all, that's
all the review comments I'm able to give here, sorry.

But as-is, this isn't acceptable :(

thanks,

greg k-h

2024-01-18 09:44:12

by Shijie Huang

[permalink] [raw]

Subject: Re: [PATCH] init: refactor the generic cpu_to_node for NUMA

Hi Greg,

在 2024/1/18 17:27, Greg KH 写道:
> On Thu, Jan 18, 2024 at 11:14:12AM +0800, Huang Shijie wrote:
>> (0) We list the ARCHs which support the NUMA:
>> arm64, loongarch, powerpc, riscv,
>> sparc, mips, s390, x86,
> I do not understand this format, what are you saying here?

Sorry for the confusing.

I should put the conclusion at the beginning:

The generic cpu_to_node() has bug in some situations.

The generic cpu_to_node() does not work in arm64, powerpc, riscv
when the CONFIG_NUMA is enabled:

The cpu_to_node() is called before it is initialized.

So all the four places are set with the wrong node id (get by
cpu_to_node()):

a.) early_trace_init() in kernel/trace/trace.c
b.) sched_init() in kernel/sched/core.c
c.) init_sched_fair_class() in kernel/sched/fair.c
d.) workqueue_init_early() in kernel/workqueue.c

Thanks

Huang Shijie

2024-01-28 17:58:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] init: refactor the generic cpu_to_node for NUMA

2024-01-28 19:43:57

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] init: refactor the generic cpu_to_node for NUMA

Hi Huang,

kernel test robot noticed the following build errors:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.8-rc1 next-20240125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Huang-Shijie/init-refactor-the-generic-cpu_to_node-for-NUMA/20240118-111802
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20240118031412.3300-1-shijie%40os.amperecomputing.com
patch subject: [PATCH] init: refactor the generic cpu_to_node for NUMA
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240129/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240129/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

ld: vmlinux.o: in function `amd_pmu_cpu_prepare':
>> arch/x86/events/amd/core.c:549: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `amd_alloc_nb':
arch/x86/events/amd/core.c:507: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `amd_uncore_ctx_init':
>> arch/x86/events/amd/uncore.c:476: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `allocate_shared_regs':
>> arch/x86/events/intel/core.c:4520: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `intel_cpuc_prepare':
arch/x86/events/intel/core.c:4561: undefined reference to `cpu_to_node'
ld: vmlinux.o:arch/x86/events/intel/core.c:4538: more undefined references to `cpu_to_node' follow
ld: vmlinux.o: in function `smp_prepare_cpus_done':
>> init/main.c:1542: undefined reference to `_cpu_to_node'
ld: vmlinux.o: in function `check_timer':
>> arch/x86/kernel/apic/io_apic.c:2169: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `kvm_alloc_cpumask':
>> arch/x86/kernel/kvm.c:687: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `fork_idle':
>> kernel/fork.c:2826: undefined reference to `cpu_to_node'
ld: vmlinux.o: in function `cpus_share_numa':
>> kernel/workqueue.c:6768: undefined reference to `cpu_to_node'
>> ld: kernel/workqueue.c:6768: undefined reference to `cpu_to_node'
ld: vmlinux.o:kernel/workqueue.c:6748: more undefined references to `cpu_to_node' follow

vim +549 arch/x86/events/amd/core.c

21d59e3e2c403c arch/x86/events/amd/core.c Sandipan Das 2022-04-21 544
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 545 static int amd_pmu_cpu_prepare(int cpu)
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 546 {
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 547 struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 548
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 @549 cpuc->lbr_sel = kzalloc_node(sizeof(struct er_account), GFP_KERNEL,
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 550 cpu_to_node(cpu));
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 551 if (!cpuc->lbr_sel)
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 552 return -ENOMEM;
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 553
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 554 WARN_ON_ONCE(cpuc->amd_nb);
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 555
32b62f446827f6 arch/x86/events/amd/core.c Peter Zijlstra 2016-03-25 556 if (!x86_pmu.amd_nb_constraints)
95ca792c7582fd arch/x86/events/amd/core.c Thomas Gleixner 2016-07-13 557 return 0;
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 558
c079c791c5a062 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-11-25 559 cpuc->amd_nb = amd_alloc_nb(cpu);
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 560 if (cpuc->amd_nb)
95ca792c7582fd arch/x86/events/amd/core.c Thomas Gleixner 2016-07-13 561 return 0;
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 562
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 563 kfree(cpuc->lbr_sel);
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 564 cpuc->lbr_sel = NULL;
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 565
f4f925dae7419f arch/x86/events/amd/core.c Sandipan Das 2022-08-11 566 return -ENOMEM;
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 567 }
b38b24ead33417 arch/x86/kernel/cpu/perf_event_amd.c Peter Zijlstra 2010-03-23 568

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki