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 <shijie@os.amperecomputing.com>
---
 drivers/base/arch_numa.c | 11 +++++++++++
 include/linux/topology.h |  6 ++----
 init/main.c              | 29 +++++++++++++++++++++++++++--
 3 files changed, 40 insertions(+), 6 deletions(-)
  

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
  

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
  

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-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240129/202401290116.GpUOCzGd-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240129/202401290116.GpUOCzGd-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401290116.GpUOCzGd-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: cpu_to_node
   >>> referenced by main.c:880 (init/main.c:880)
   >>>               init/main.o:(start_kernel) in archive vmlinux.a
   >>> referenced by main.c:1542 (init/main.c:1542)
   >>>               init/main.o:(kernel_init_freeable) in archive vmlinux.a
   >>> referenced by core.c:550 (arch/x86/events/amd/core.c:550)
   >>>               arch/x86/events/amd/core.o:(amd_pmu_cpu_prepare) in archive vmlinux.a
   >>> referenced 179 more times
--
>> ld.lld: error: undefined symbol: _cpu_to_node
   >>> referenced by main.c:1542 (init/main.c:1542)
   >>>               init/main.o:(kernel_init_freeable) in archive vmlinux.a
  

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/202401290316.0eu1Mue2-lkp@intel.com/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/202401290316.0eu1Mue2-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401290316.0eu1Mue2-lkp@intel.com/

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