| Message ID | 20210811102423.28908-28-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm64 | expand |
Hi Wei, On 11/08/2021 11:24, Wei Chen wrote: > Sometimes, CPU logical ID maybe different with physical CPU ID. > Xen is using CPU logial ID for runtime usage, so we should use > CPU logical ID to create map between NUMA node and CPU. This commit message gives the impression that you are trying to fix a bug. However, what you are explaining is the reason why the code will use the logical ID rather than physical ID. I think the commit message should explain what the patch is doing. You can then add an explanation why you are using the CPU logical ID. Something like "Note we storing the CPU logical ID because...". > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/smpboot.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index aa78958c07..dd5a45bffc 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -121,7 +121,12 @@ static void __init dt_smp_init_cpus(void) > { > [0 ... NR_CPUS - 1] = MPIDR_INVALID > }; > + static nodeid_t node_map[NR_CPUS] __initdata = > + { > + [0 ... NR_CPUS - 1] = NUMA_NO_NODE > + }; > bool bootcpu_valid = false; > + uint32_t nid = 0; > int rc; > > mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK; > @@ -172,6 +177,26 @@ static void __init dt_smp_init_cpus(void) > continue; > } > > +#ifdef CONFIG_DEVICE_TREE_NUMA > + /* > + * When CONFIG_DEVICE_TREE_NUMA is set, try to fetch numa infomation > + * from CPU dts node, otherwise the nid is always 0. > + */ > + if ( !dt_property_read_u32(cpu, "numa-node-id", &nid) ) You can avoid the #ifdef by writing: if ( IS_ENABLED(CONFIG_DEVICE_TREE_NUMA) && ... ) However, I would using CONFIG_NUMA because this code is already DT specific. So we can shorten the name a bit. > + { > + printk(XENLOG_WARNING > + "cpu[%d] dts path: %s: doesn't have numa infomation!\n", s/information/information/ > + cpuidx, dt_node_full_name(cpu)); > + /* > + * The the early stage of NUMA initialization, when Xen found any s/The/During/? > + * CPU dts node doesn't have numa-node-id info, the NUMA will be > + * treated as off, all CPU will be set to a FAKE node 0. So if we > + * get numa-node-id failed here, we should set nid to 0. > + */ > + nid = 0; > + } > +#endif > + > /* > * 8 MSBs must be set to 0 in the DT since the reg property > * defines the MPIDR[23:0] > @@ -231,9 +256,12 @@ static void __init dt_smp_init_cpus(void) > { > printk("cpu%d init failed (hwid %"PRIregister"): %d\n", i, hwid, rc); > tmp_map[i] = MPIDR_INVALID; > + node_map[i] = NUMA_NO_NODE; > } > - else > + else { > tmp_map[i] = hwid; > + node_map[i] = nid; > + } > } > > if ( !bootcpu_valid ) > @@ -249,6 +277,7 @@ static void __init dt_smp_init_cpus(void) > continue; > cpumask_set_cpu(i, &cpu_possible_map); > cpu_logical_map(i) = tmp_map[i]; > + numa_set_node(i, node_map[i]); > } > } > > Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月26日 1:07 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Jan Beulich > <jbeulich@suse.com> > Subject: Re: [XEN RFC PATCH 27/40] xen/arm: build CPU NUMA node map while > creating cpu_logical_map > > Hi Wei, > > On 11/08/2021 11:24, Wei Chen wrote: > > Sometimes, CPU logical ID maybe different with physical CPU ID. > > Xen is using CPU logial ID for runtime usage, so we should use > > CPU logical ID to create map between NUMA node and CPU. > > This commit message gives the impression that you are trying to fix a > bug. However, what you are explaining is the reason why the code will > use the logical ID rather than physical ID. > > I think the commit message should explain what the patch is doing. You > can then add an explanation why you are using the CPU logical ID. > Something like "Note we storing the CPU logical ID because...". > > Ok > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/smpboot.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > > index aa78958c07..dd5a45bffc 100644 > > --- a/xen/arch/arm/smpboot.c > > +++ b/xen/arch/arm/smpboot.c > > @@ -121,7 +121,12 @@ static void __init dt_smp_init_cpus(void) > > { > > [0 ... NR_CPUS - 1] = MPIDR_INVALID > > }; > > + static nodeid_t node_map[NR_CPUS] __initdata = > > + { > > + [0 ... NR_CPUS - 1] = NUMA_NO_NODE > > + }; > > bool bootcpu_valid = false; > > + uint32_t nid = 0; > > int rc; > > > > mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK; > > @@ -172,6 +177,26 @@ static void __init dt_smp_init_cpus(void) > > continue; > > } > > > > +#ifdef CONFIG_DEVICE_TREE_NUMA > > + /* > > + * When CONFIG_DEVICE_TREE_NUMA is set, try to fetch numa > infomation > > + * from CPU dts node, otherwise the nid is always 0. > > + */ > > + if ( !dt_property_read_u32(cpu, "numa-node-id", &nid) ) > > You can avoid the #ifdef by writing: > > if ( IS_ENABLED(CONFIG_DEVICE_TREE_NUMA) && ... ) > > However, I would using CONFIG_NUMA because this code is already DT > specific. So we can shorten the name a bit. > OK > > + { > > + printk(XENLOG_WARNING > > + "cpu[%d] dts path: %s: doesn't have numa infomation!\n", > > s/information/information/ OK > > > + cpuidx, dt_node_full_name(cpu)); > > + /* > > + * The the early stage of NUMA initialization, when Xen > found any > > s/The/During/? Oh, yes, I will fix it. > > > + * CPU dts node doesn't have numa-node-id info, the NUMA > will be > > + * treated as off, all CPU will be set to a FAKE node 0. So > if we > > + * get numa-node-id failed here, we should set nid to 0. > > + */ > > + nid = 0; > > + } > > +#endif > > + > > /* > > * 8 MSBs must be set to 0 in the DT since the reg property > > * defines the MPIDR[23:0] > > @@ -231,9 +256,12 @@ static void __init dt_smp_init_cpus(void) > > { > > printk("cpu%d init failed (hwid %"PRIregister"): %d\n", i, > hwid, rc); > > tmp_map[i] = MPIDR_INVALID; > > + node_map[i] = NUMA_NO_NODE; > > } > > - else > > + else { > > tmp_map[i] = hwid; > > + node_map[i] = nid; > > + } > > } > > > > if ( !bootcpu_valid ) > > @@ -249,6 +277,7 @@ static void __init dt_smp_init_cpus(void) > > continue; > > cpumask_set_cpu(i, &cpu_possible_map); > > cpu_logical_map(i) = tmp_map[i]; > > + numa_set_node(i, node_map[i]); > > } > > } > > > > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index aa78958c07..dd5a45bffc 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -121,7 +121,12 @@ static void __init dt_smp_init_cpus(void) { [0 ... NR_CPUS - 1] = MPIDR_INVALID }; + static nodeid_t node_map[NR_CPUS] __initdata = + { + [0 ... NR_CPUS - 1] = NUMA_NO_NODE + }; bool bootcpu_valid = false; + uint32_t nid = 0; int rc; mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK; @@ -172,6 +177,26 @@ static void __init dt_smp_init_cpus(void) continue; } +#ifdef CONFIG_DEVICE_TREE_NUMA + /* + * When CONFIG_DEVICE_TREE_NUMA is set, try to fetch numa infomation + * from CPU dts node, otherwise the nid is always 0. + */ + if ( !dt_property_read_u32(cpu, "numa-node-id", &nid) ) + { + printk(XENLOG_WARNING + "cpu[%d] dts path: %s: doesn't have numa infomation!\n", + cpuidx, dt_node_full_name(cpu)); + /* + * The the early stage of NUMA initialization, when Xen found any + * CPU dts node doesn't have numa-node-id info, the NUMA will be + * treated as off, all CPU will be set to a FAKE node 0. So if we + * get numa-node-id failed here, we should set nid to 0. + */ + nid = 0; + } +#endif + /* * 8 MSBs must be set to 0 in the DT since the reg property * defines the MPIDR[23:0] @@ -231,9 +256,12 @@ static void __init dt_smp_init_cpus(void) { printk("cpu%d init failed (hwid %"PRIregister"): %d\n", i, hwid, rc); tmp_map[i] = MPIDR_INVALID; + node_map[i] = NUMA_NO_NODE; } - else + else { tmp_map[i] = hwid; + node_map[i] = nid; + } } if ( !bootcpu_valid ) @@ -249,6 +277,7 @@ static void __init dt_smp_init_cpus(void) continue; cpumask_set_cpu(i, &cpu_possible_map); cpu_logical_map(i) = tmp_map[i]; + numa_set_node(i, node_map[i]); } }
Sometimes, CPU logical ID maybe different with physical CPU ID. Xen is using CPU logial ID for runtime usage, so we should use CPU logical ID to create map between NUMA node and CPU. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/arm/smpboot.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)