| Message ID | 20210811102423.28908-14-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:23, Wei Chen wrote: > This API is used to set one CPU to a NUMA node. If the system > configure NUMA off or system initialize NUMA failed, the > online NUMA node would set to only node#0. This will be done > in following patches. When NUMA turn off or init failed, > node_online_map will be cleared and set node#0 online. So we > use node_online_map to prevent to set a CPU to an offline node. IHMO numa_set_node() should behave exactly the same way on x86 and Arm because this is going to be used by the common code. From the commit message, I don't quite understand why the check is necessary on Arm but not on x86. Can you clarify it? > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/numa.c | 31 +++++++++++++++++++++++++++++++ > xen/include/asm-arm/numa.h | 2 ++ > xen/include/asm-x86/numa.h | 1 - > xen/include/xen/numa.h | 1 + > 5 files changed, 35 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/arm/numa.c > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 3d3b97b5b4..6e3fb8033e 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -35,6 +35,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o > obj-y += mem_access.o > obj-y += mm.o > obj-y += monitor.o > +obj-$(CONFIG_NUMA) += numa.o > obj-y += p2m.o > obj-y += percpu.o > obj-y += platform.o > diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c > new file mode 100644 > index 0000000000..1e30c5bb13 > --- /dev/null > +++ b/xen/arch/arm/numa.c > @@ -0,0 +1,31 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Arm Architecture support layer for NUMA. > + * > + * Copyright (C) 2021 Arm Ltd > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > + */ > +#include <xen/init.h> > +#include <xen/nodemask.h> > +#include <xen/numa.h> > + > +void numa_set_node(int cpu, nodeid_t nid) > +{ > + if ( nid >= MAX_NUMNODES || > + !nodemask_test(nid, &node_online_map) ) > + nid = 0; > + > + cpu_to_node[cpu] = nid; > +} I think numa_set_node() will want to be implemented in common code. > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h > index ab9c4a2448..1162c702df 100644 > --- a/xen/include/asm-arm/numa.h > +++ b/xen/include/asm-arm/numa.h > @@ -27,6 +27,8 @@ extern mfn_t first_valid_mfn; > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > #define __node_distance(a, b) (20) > > +#define numa_set_node(x, y) do { } while (0) I would define it in xen/numa.h so other arch can take advantage ot it. Also, please use a static inline helper so the arguments are evaluated. > + > #endif > > #endif /* __ARCH_ARM_NUMA_H */ > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h > index f8e4e15586..69859b0a57 100644 > --- a/xen/include/asm-x86/numa.h > +++ b/xen/include/asm-x86/numa.h > @@ -22,7 +22,6 @@ extern nodeid_t pxm_to_node(unsigned int pxm); > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > > extern int srat_disabled(void); > -extern void numa_set_node(int cpu, nodeid_t node); > extern nodeid_t setup_node(unsigned int pxm); > extern void srat_detect_node(int cpu); > > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h > index 258a5cb3db..3972aa6b93 100644 > --- a/xen/include/xen/numa.h > +++ b/xen/include/xen/numa.h > @@ -70,6 +70,7 @@ extern int valid_numa_range(u64 start, u64 end, nodeid_t node); > > extern void numa_init_array(void); > extern void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn); > +extern void numa_set_node(int cpu, nodeid_t node); > extern bool numa_off; > extern int numa_fake; > > Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月25日 18:37 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for > Arm > > Hi Wei, > > On 11/08/2021 11:23, Wei Chen wrote: > > This API is used to set one CPU to a NUMA node. If the system > > configure NUMA off or system initialize NUMA failed, the > > online NUMA node would set to only node#0. This will be done > > in following patches. When NUMA turn off or init failed, > > node_online_map will be cleared and set node#0 online. So we > > use node_online_map to prevent to set a CPU to an offline node. > > IHMO numa_set_node() should behave exactly the same way on x86 and Arm > because this is going to be used by the common code. > > From the commit message, I don't quite understand why the check is > necessary on Arm but not on x86. Can you clarify it? > Yes, in patch#27, in smpboot.c, dt_smp_init_cpus function. We will parse CPU numa-node-id from dtb CPU node. If we get a valid node ID for one CPU, we will invoke numa_set_node to create CPU-NODE map. But in our testing, we found when NUMA init failed, numa_set_node still can set CPU to a offline or invalid NODE. So we're using node_online_map to prevent this behavior. Otherwise we have to check node_online_map everywhere before we call numa_set_node. x86 actually is doing the same way, but it handles node_online_map check out of numa_set_node: 57 void __init init_cpu_to_node(void) 58 { 59 unsigned int i; 60 nodeid_t node; 61 62 for ( i = 0; i < nr_cpu_ids; i++ ) 63 { 64 u32 apicid = x86_cpu_to_apicid[i]; 65 if ( apicid == BAD_APICID ) 66 continue; 67 node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; 68 if ( node == NUMA_NO_NODE || !node_online(node) ) 69 node = 0; 70 numa_set_node(i, node); 71 } 72 } > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/Makefile | 1 + > > xen/arch/arm/numa.c | 31 +++++++++++++++++++++++++++++++ > > xen/include/asm-arm/numa.h | 2 ++ > > xen/include/asm-x86/numa.h | 1 - > > xen/include/xen/numa.h | 1 + > > 5 files changed, 35 insertions(+), 1 deletion(-) > > create mode 100644 xen/arch/arm/numa.c > > > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > > index 3d3b97b5b4..6e3fb8033e 100644 > > --- a/xen/arch/arm/Makefile > > +++ b/xen/arch/arm/Makefile > > @@ -35,6 +35,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o > > obj-y += mem_access.o > > obj-y += mm.o > > obj-y += monitor.o > > +obj-$(CONFIG_NUMA) += numa.o > > obj-y += p2m.o > > obj-y += percpu.o > > obj-y += platform.o > > diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c > > new file mode 100644 > > index 0000000000..1e30c5bb13 > > --- /dev/null > > +++ b/xen/arch/arm/numa.c > > @@ -0,0 +1,31 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Arm Architecture support layer for NUMA. > > + * > > + * Copyright (C) 2021 Arm Ltd > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > > + * > > + */ > > +#include <xen/init.h> > > +#include <xen/nodemask.h> > > +#include <xen/numa.h> > > + > > +void numa_set_node(int cpu, nodeid_t nid) > > +{ > > + if ( nid >= MAX_NUMNODES || > > + !nodemask_test(nid, &node_online_map) ) > > + nid = 0; > > + > > + cpu_to_node[cpu] = nid; > > +} > I think numa_set_node() will want to be implemented in common code. > See my above comment. If x86 is ok, I think yes, we can do it in common code. > > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h > > index ab9c4a2448..1162c702df 100644 > > --- a/xen/include/asm-arm/numa.h > > +++ b/xen/include/asm-arm/numa.h > > @@ -27,6 +27,8 @@ extern mfn_t first_valid_mfn; > > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > > #define __node_distance(a, b) (20) > > > > +#define numa_set_node(x, y) do { } while (0) > > I would define it in xen/numa.h so other arch can take advantage ot it. > Also, please use a static inline helper so the arguments are evaluated. > Ok > > + > > #endif > > > > #endif /* __ARCH_ARM_NUMA_H */ > > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h > > index f8e4e15586..69859b0a57 100644 > > --- a/xen/include/asm-x86/numa.h > > +++ b/xen/include/asm-x86/numa.h > > @@ -22,7 +22,6 @@ extern nodeid_t pxm_to_node(unsigned int pxm); > > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > > > > extern int srat_disabled(void); > > -extern void numa_set_node(int cpu, nodeid_t node); > > extern nodeid_t setup_node(unsigned int pxm); > > extern void srat_detect_node(int cpu); > > > > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h > > index 258a5cb3db..3972aa6b93 100644 > > --- a/xen/include/xen/numa.h > > +++ b/xen/include/xen/numa.h > > @@ -70,6 +70,7 @@ extern int valid_numa_range(u64 start, u64 end, > nodeid_t node); > > > > extern void numa_init_array(void); > > extern void numa_initmem_init(unsigned long start_pfn, unsigned long > end_pfn); > > +extern void numa_set_node(int cpu, nodeid_t node); > > extern bool numa_off; > > extern int numa_fake; > > > > > > Cheers, > > -- > Julien Grall
On 25/08/2021 13:07, Wei Chen wrote: > Hi Julien, Hi Wei, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 2021年8月25日 18:37 >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; jbeulich@suse.com >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> >> Subject: Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for >> Arm >> >> Hi Wei, >> >> On 11/08/2021 11:23, Wei Chen wrote: >>> This API is used to set one CPU to a NUMA node. If the system >>> configure NUMA off or system initialize NUMA failed, the >>> online NUMA node would set to only node#0. This will be done >>> in following patches. When NUMA turn off or init failed, >>> node_online_map will be cleared and set node#0 online. So we >>> use node_online_map to prevent to set a CPU to an offline node. >> >> IHMO numa_set_node() should behave exactly the same way on x86 and Arm >> because this is going to be used by the common code. >> >> From the commit message, I don't quite understand why the check is >> necessary on Arm but not on x86. Can you clarify it? >> > > Yes, in patch#27, in smpboot.c, dt_smp_init_cpus function. > We will parse CPU numa-node-id from dtb CPU node. If we get > a valid node ID for one CPU, we will invoke numa_set_node to > create CPU-NODE map. But in our testing, we found when NUMA > init failed, numa_set_node still can set CPU to a offline > or invalid NODE. So we're using node_online_map to prevent > this behavior. Otherwise we have to check node_online_map > everywhere before we call numa_set_node. What do you mean by invalid NODE? Is it 0xFF (NUMA_NO_NODE)? > > x86 actually is doing the same way, but it handles node_online_map > check out of numa_set_node: Right... >> I think numa_set_node() will want to be implemented in common code. >> > > See my above comment. If x86 is ok, I think yes, we can do it > in common code. ... on x86, this check is performed outside of numa_set_node() for one caller whereas on Arm you are adding it in numa_set_node(). For example, numa_set_node() can be called with NUMA_NO_NODE. On x86, we would set cpu_to_node[] to that value. However, if I am not mistaken, on Arm we would set the value to 0. This will change the behavior of users to cpu_to_node() later on (such as XEN_SYSCTL_cputopoinfo). NUMA is not something architecture specific, so I dont't think the implementation should differ here. In this case, I think numa_set_node() shouldn't check if the node is valid. Instead, the caller should take care of it if it is important. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月25日 21:24 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for > Arm > > > > On 25/08/2021 13:07, Wei Chen wrote: > > Hi Julien, > > Hi Wei, > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: 2021年8月25日 18:37 > >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > >> sstabellini@kernel.org; jbeulich@suse.com > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > >> Subject: Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for > >> Arm > >> > >> Hi Wei, > >> > >> On 11/08/2021 11:23, Wei Chen wrote: > >>> This API is used to set one CPU to a NUMA node. If the system > >>> configure NUMA off or system initialize NUMA failed, the > >>> online NUMA node would set to only node#0. This will be done > >>> in following patches. When NUMA turn off or init failed, > >>> node_online_map will be cleared and set node#0 online. So we > >>> use node_online_map to prevent to set a CPU to an offline node. > >> > >> IHMO numa_set_node() should behave exactly the same way on x86 and Arm > >> because this is going to be used by the common code. > >> > >> From the commit message, I don't quite understand why the check is > >> necessary on Arm but not on x86. Can you clarify it? > >> > > > > Yes, in patch#27, in smpboot.c, dt_smp_init_cpus function. > > We will parse CPU numa-node-id from dtb CPU node. If we get > > a valid node ID for one CPU, we will invoke numa_set_node to > > create CPU-NODE map. But in our testing, we found when NUMA > > init failed, numa_set_node still can set CPU to a offline > > or invalid NODE. So we're using node_online_map to prevent > > this behavior. Otherwise we have to check node_online_map > > everywhere before we call numa_set_node. > > What do you mean by invalid NODE? Is it 0xFF (NUMA_NO_NODE)? No, I mean some wrong content in device tree. For example, if the dtb set a wrong numa-node-id in CPU dt-node. > > > > > x86 actually is doing the same way, but it handles node_online_map > > check out of numa_set_node: > > Right... > > >> I think numa_set_node() will want to be implemented in common code. > >> > > > > See my above comment. If x86 is ok, I think yes, we can do it > > in common code. > > ... on x86, this check is performed outside of numa_set_node() for one > caller whereas on Arm you are adding it in numa_set_node(). > > For example, numa_set_node() can be called with NUMA_NO_NODE. On x86, we > would set cpu_to_node[] to that value. However, if I am not mistaken, on > Arm we would set the value to 0. > > This will change the behavior of users to cpu_to_node() later on (such > as XEN_SYSCTL_cputopoinfo). > > NUMA is not something architecture specific, so I dont't think the > implementation should differ here. > > In this case, I think numa_set_node() shouldn't check if the node is > valid. Instead, the caller should take care of it if it is important. > Yes, I agree. I will change it in next version. > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 3d3b97b5b4..6e3fb8033e 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -35,6 +35,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o obj-y += mem_access.o obj-y += mm.o obj-y += monitor.o +obj-$(CONFIG_NUMA) += numa.o obj-y += p2m.o obj-y += percpu.o obj-y += platform.o diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c new file mode 100644 index 0000000000..1e30c5bb13 --- /dev/null +++ b/xen/arch/arm/numa.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Arm Architecture support layer for NUMA. + * + * Copyright (C) 2021 Arm Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + */ +#include <xen/init.h> +#include <xen/nodemask.h> +#include <xen/numa.h> + +void numa_set_node(int cpu, nodeid_t nid) +{ + if ( nid >= MAX_NUMNODES || + !nodemask_test(nid, &node_online_map) ) + nid = 0; + + cpu_to_node[cpu] = nid; +} diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h index ab9c4a2448..1162c702df 100644 --- a/xen/include/asm-arm/numa.h +++ b/xen/include/asm-arm/numa.h @@ -27,6 +27,8 @@ extern mfn_t first_valid_mfn; #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) #define __node_distance(a, b) (20) +#define numa_set_node(x, y) do { } while (0) + #endif #endif /* __ARCH_ARM_NUMA_H */ diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h index f8e4e15586..69859b0a57 100644 --- a/xen/include/asm-x86/numa.h +++ b/xen/include/asm-x86/numa.h @@ -22,7 +22,6 @@ extern nodeid_t pxm_to_node(unsigned int pxm); #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) extern int srat_disabled(void); -extern void numa_set_node(int cpu, nodeid_t node); extern nodeid_t setup_node(unsigned int pxm); extern void srat_detect_node(int cpu); diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h index 258a5cb3db..3972aa6b93 100644 --- a/xen/include/xen/numa.h +++ b/xen/include/xen/numa.h @@ -70,6 +70,7 @@ extern int valid_numa_range(u64 start, u64 end, nodeid_t node); extern void numa_init_array(void); extern void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn); +extern void numa_set_node(int cpu, nodeid_t node); extern bool numa_off; extern int numa_fake;
This API is used to set one CPU to a NUMA node. If the system configure NUMA off or system initialize NUMA failed, the online NUMA node would set to only node#0. This will be done in following patches. When NUMA turn off or init failed, node_online_map will be cleared and set node#0 online. So we use node_online_map to prevent to set a CPU to an offline node. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/arm/Makefile | 1 + xen/arch/arm/numa.c | 31 +++++++++++++++++++++++++++++++ xen/include/asm-arm/numa.h | 2 ++ xen/include/asm-x86/numa.h | 1 - xen/include/xen/numa.h | 1 + 5 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 xen/arch/arm/numa.c