Message ID | 20230110084930.1095203-3-wei.chen@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Device tree based NUMA support for Arm - Part#3 | expand |
On 10.01.2023 09:49, Wei Chen wrote: > --- a/xen/arch/arm/include/asm/numa.h > +++ b/xen/arch/arm/include/asm/numa.h > @@ -22,6 +22,12 @@ typedef u8 nodeid_t; > */ > #define NR_NODE_MEMBLKS NR_MEM_BANKS > > +enum dt_numa_status { > + DT_NUMA_INIT, I don't see any use of this. I also think the name isn't good, as INIT can be taken for "initializer" as well as "initialized". Suggesting an alternative would require knowing what the future plans with this are; right now ... > + DT_NUMA_ON, > + DT_NUMA_OFF, > +}; ... the other two are also used only in a single file, at which point their placing in a header is also questionable. > --- /dev/null > +++ b/xen/arch/arm/numa.c > @@ -0,0 +1,44 @@ > +/* 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/numa.h> > + > +static enum dt_numa_status __read_mostly device_tree_numa; __ro_after_init? > --- a/xen/arch/x86/include/asm/numa.h > +++ b/xen/arch/x86/include/asm/numa.h > @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n); > > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > > -extern bool numa_disabled(void); > extern nodeid_t setup_node(unsigned int pxm); > extern void srat_detect_node(int cpu); > > --- a/xen/include/xen/numa.h > +++ b/xen/include/xen/numa.h > @@ -55,6 +55,7 @@ extern void numa_init_array(void); > extern void numa_set_node(unsigned int cpu, nodeid_t node); > extern void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn); > extern void numa_fw_bad(void); > +extern bool numa_disabled(void); > > extern int arch_numa_setup(const char *opt); > extern bool arch_numa_unavailable(void); How is this movement of a declaration related to the subject of the patch? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2023年1月11日 0:38 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien > Grall <julien@xen.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei > Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 02/17] xen/arm: implement helpers to get and update > NUMA status > > On 10.01.2023 09:49, Wei Chen wrote: > > --- a/xen/arch/arm/include/asm/numa.h > > +++ b/xen/arch/arm/include/asm/numa.h > > @@ -22,6 +22,12 @@ typedef u8 nodeid_t; > > */ > > #define NR_NODE_MEMBLKS NR_MEM_BANKS > > > > +enum dt_numa_status { > > + DT_NUMA_INIT, > > I don't see any use of this. I also think the name isn't good, as INIT > can be taken for "initializer" as well as "initialized". Suggesting an > alternative would require knowing what the future plans with this are; > right now ... > static enum dt_numa_status __read_mostly device_tree_numa; We use DT_NUMA_INIT to indicate device_tree_numa is in a default value (System's initial value, hasn't done initialization). Maybe rename it To DT_NUMA_UNINIT be better? > > + DT_NUMA_ON, > > + DT_NUMA_OFF, > > +}; > > ... the other two are also used only in a single file, at which point > their placing in a header is also questionable. > This is a good point, I will move them to that file. > > --- /dev/null > > +++ b/xen/arch/arm/numa.c > > @@ -0,0 +1,44 @@ > > +/* 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/numa.h> > > + > > +static enum dt_numa_status __read_mostly device_tree_numa; > > __ro_after_init? > OK. > > --- a/xen/arch/x86/include/asm/numa.h > > +++ b/xen/arch/x86/include/asm/numa.h > > @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n); > > > > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > > > > -extern bool numa_disabled(void); > > extern nodeid_t setup_node(unsigned int pxm); > > extern void srat_detect_node(int cpu); > > > > --- a/xen/include/xen/numa.h > > +++ b/xen/include/xen/numa.h > > @@ -55,6 +55,7 @@ extern void numa_init_array(void); > > extern void numa_set_node(unsigned int cpu, nodeid_t node); > > extern void numa_initmem_init(unsigned long start_pfn, unsigned long > end_pfn); > > extern void numa_fw_bad(void); > > +extern bool numa_disabled(void); > > > > extern int arch_numa_setup(const char *opt); > > extern bool arch_numa_unavailable(void); > > How is this movement of a declaration related to the subject of the patch? > Can I add some messages in commit log to say something like "As we have Implemented numa_disabled for Arm, so we move numa_disabled to common header"? Cheers, Wei Chen > Jan
On 12.01.2023 07:22, Wei Chen wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2023年1月11日 0:38 >> >> On 10.01.2023 09:49, Wei Chen wrote: >>> --- a/xen/arch/arm/include/asm/numa.h >>> +++ b/xen/arch/arm/include/asm/numa.h >>> @@ -22,6 +22,12 @@ typedef u8 nodeid_t; >>> */ >>> #define NR_NODE_MEMBLKS NR_MEM_BANKS >>> >>> +enum dt_numa_status { >>> + DT_NUMA_INIT, >> >> I don't see any use of this. I also think the name isn't good, as INIT >> can be taken for "initializer" as well as "initialized". Suggesting an >> alternative would require knowing what the future plans with this are; >> right now ... >> > > static enum dt_numa_status __read_mostly device_tree_numa; There's no DT_NUMA_INIT here. You _imply_ it having a value of zero. > We use DT_NUMA_INIT to indicate device_tree_numa is in a default value > (System's initial value, hasn't done initialization). Maybe rename it > To DT_NUMA_UNINIT be better? Perhaps, yes. >>> --- a/xen/arch/x86/include/asm/numa.h >>> +++ b/xen/arch/x86/include/asm/numa.h >>> @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n); >>> >>> #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) >>> >>> -extern bool numa_disabled(void); >>> extern nodeid_t setup_node(unsigned int pxm); >>> extern void srat_detect_node(int cpu); >>> >>> --- a/xen/include/xen/numa.h >>> +++ b/xen/include/xen/numa.h >>> @@ -55,6 +55,7 @@ extern void numa_init_array(void); >>> extern void numa_set_node(unsigned int cpu, nodeid_t node); >>> extern void numa_initmem_init(unsigned long start_pfn, unsigned long >> end_pfn); >>> extern void numa_fw_bad(void); >>> +extern bool numa_disabled(void); >>> >>> extern int arch_numa_setup(const char *opt); >>> extern bool arch_numa_unavailable(void); >> >> How is this movement of a declaration related to the subject of the patch? >> > > Can I add some messages in commit log to say something like "As we have > Implemented numa_disabled for Arm, so we move numa_disabled to common header"? See your own patch 3, where you have a similar statement (albeit you mean "declaration" there, not "definition"). However, right now numa_disabled() is a #define on Arm, so the declaration becoming common isn't really warranted. In fact it'll get in the way of converting function-like macros to inline functions for Misra. Jan
Hi Jan, On 2023/1/12 16:08, Jan Beulich wrote: > On 12.01.2023 07:22, Wei Chen wrote: >>> -----Original Message----- >>> From: Jan Beulich <jbeulich@suse.com> >>> Sent: 2023年1月11日 0:38 >>> >>> On 10.01.2023 09:49, Wei Chen wrote: >>>> --- a/xen/arch/arm/include/asm/numa.h >>>> +++ b/xen/arch/arm/include/asm/numa.h >>>> @@ -22,6 +22,12 @@ typedef u8 nodeid_t; >>>> */ >>>> #define NR_NODE_MEMBLKS NR_MEM_BANKS >>>> >>>> +enum dt_numa_status { >>>> + DT_NUMA_INIT, >>> >>> I don't see any use of this. I also think the name isn't good, as INIT >>> can be taken for "initializer" as well as "initialized". Suggesting an >>> alternative would require knowing what the future plans with this are; >>> right now ... >>> >> >> static enum dt_numa_status __read_mostly device_tree_numa; > > There's no DT_NUMA_INIT here. You _imply_ it having a value of zero. > How about I assign device_tree_numa explicitly like: ... __read_mostly device_tree_numa = DT_NUMA_UNINIT; >> We use DT_NUMA_INIT to indicate device_tree_numa is in a default value >> (System's initial value, hasn't done initialization). Maybe rename it >> To DT_NUMA_UNINIT be better? > > Perhaps, yes. > >>>> --- a/xen/arch/x86/include/asm/numa.h >>>> +++ b/xen/arch/x86/include/asm/numa.h >>>> @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n); >>>> >>>> #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) >>>> >>>> -extern bool numa_disabled(void); >>>> extern nodeid_t setup_node(unsigned int pxm); >>>> extern void srat_detect_node(int cpu); >>>> >>>> --- a/xen/include/xen/numa.h >>>> +++ b/xen/include/xen/numa.h >>>> @@ -55,6 +55,7 @@ extern void numa_init_array(void); >>>> extern void numa_set_node(unsigned int cpu, nodeid_t node); >>>> extern void numa_initmem_init(unsigned long start_pfn, unsigned long >>> end_pfn); >>>> extern void numa_fw_bad(void); >>>> +extern bool numa_disabled(void); >>>> >>>> extern int arch_numa_setup(const char *opt); >>>> extern bool arch_numa_unavailable(void); >>> >>> How is this movement of a declaration related to the subject of the patch? >>> >> >> Can I add some messages in commit log to say something like "As we have >> Implemented numa_disabled for Arm, so we move numa_disabled to common header"? > > See your own patch 3, where you have a similar statement (albeit you mean > "declaration" there, not "definition"). However, right now numa_disabled() > is a #define on Arm, so the declaration becoming common isn't really > warranted. In fact it'll get in the way of converting function-like macros > to inline functions for Misra. > Yes, I think you're right. This may also seem a little strange,when we disable NUMA, there are two headers have numa_disabled statement. I will revert this change. And I also will covert the macro to a static inline function. Cheers, Wei Chen > Jan
On 16.01.2023 10:20, Wei Chen wrote: > Hi Jan, > > On 2023/1/12 16:08, Jan Beulich wrote: >> On 12.01.2023 07:22, Wei Chen wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: 2023年1月11日 0:38 >>>> >>>> On 10.01.2023 09:49, Wei Chen wrote: >>>>> --- a/xen/arch/arm/include/asm/numa.h >>>>> +++ b/xen/arch/arm/include/asm/numa.h >>>>> @@ -22,6 +22,12 @@ typedef u8 nodeid_t; >>>>> */ >>>>> #define NR_NODE_MEMBLKS NR_MEM_BANKS >>>>> >>>>> +enum dt_numa_status { >>>>> + DT_NUMA_INIT, >>>> >>>> I don't see any use of this. I also think the name isn't good, as INIT >>>> can be taken for "initializer" as well as "initialized". Suggesting an >>>> alternative would require knowing what the future plans with this are; >>>> right now ... >>>> >>> >>> static enum dt_numa_status __read_mostly device_tree_numa; >> >> There's no DT_NUMA_INIT here. You _imply_ it having a value of zero. >> > > How about I assign device_tree_numa explicitly like: > ... __read_mostly device_tree_numa = DT_NUMA_UNINIT; Well, yes, this is what I was asking for when mentioning the lack of use of the enumerator. Irrespective of that I remain unhappy with the name, though. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2023年1月16日 19:15 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien > Grall <julien@xen.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei > Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 02/17] xen/arm: implement helpers to get and update > NUMA status > > On 16.01.2023 10:20, Wei Chen wrote: > > Hi Jan, > > > > On 2023/1/12 16:08, Jan Beulich wrote: > >> On 12.01.2023 07:22, Wei Chen wrote: > >>>> -----Original Message----- > >>>> From: Jan Beulich <jbeulich@suse.com> > >>>> Sent: 2023年1月11日 0:38 > >>>> > >>>> On 10.01.2023 09:49, Wei Chen wrote: > >>>>> --- a/xen/arch/arm/include/asm/numa.h > >>>>> +++ b/xen/arch/arm/include/asm/numa.h > >>>>> @@ -22,6 +22,12 @@ typedef u8 nodeid_t; > >>>>> */ > >>>>> #define NR_NODE_MEMBLKS NR_MEM_BANKS > >>>>> > >>>>> +enum dt_numa_status { > >>>>> + DT_NUMA_INIT, > >>>> > >>>> I don't see any use of this. I also think the name isn't good, as > INIT > >>>> can be taken for "initializer" as well as "initialized". Suggesting > an > >>>> alternative would require knowing what the future plans with this are; > >>>> right now ... > >>>> > >>> > >>> static enum dt_numa_status __read_mostly device_tree_numa; > >> > >> There's no DT_NUMA_INIT here. You _imply_ it having a value of zero. > >> > > > > How about I assign device_tree_numa explicitly like: > > ... __read_mostly device_tree_numa = DT_NUMA_UNINIT; > > Well, yes, this is what I was asking for when mentioning the lack of use > of the enumerator. Irrespective of that I remain unhappy with the name, > though. > How about DT_NUMA_DEF or do you have some suggestions for the name? Cheers, Wei Chen > Jan
On 16.01.2023 13:01, Wei Chen wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2023年1月16日 19:15 >> >> On 16.01.2023 10:20, Wei Chen wrote: >>> On 2023/1/12 16:08, Jan Beulich wrote: >>>> On 12.01.2023 07:22, Wei Chen wrote: >>>>>> -----Original Message----- >>>>>> From: Jan Beulich <jbeulich@suse.com> >>>>>> Sent: 2023年1月11日 0:38 >>>>>> >>>>>> On 10.01.2023 09:49, Wei Chen wrote: >>>>>>> --- a/xen/arch/arm/include/asm/numa.h >>>>>>> +++ b/xen/arch/arm/include/asm/numa.h >>>>>>> @@ -22,6 +22,12 @@ typedef u8 nodeid_t; >>>>>>> */ >>>>>>> #define NR_NODE_MEMBLKS NR_MEM_BANKS >>>>>>> >>>>>>> +enum dt_numa_status { >>>>>>> + DT_NUMA_INIT, >>>>>> >>>>>> I don't see any use of this. I also think the name isn't good, as >> INIT >>>>>> can be taken for "initializer" as well as "initialized". Suggesting >> an >>>>>> alternative would require knowing what the future plans with this are; >>>>>> right now ... >>>>>> >>>>> >>>>> static enum dt_numa_status __read_mostly device_tree_numa; >>>> >>>> There's no DT_NUMA_INIT here. You _imply_ it having a value of zero. >>>> >>> >>> How about I assign device_tree_numa explicitly like: >>> ... __read_mostly device_tree_numa = DT_NUMA_UNINIT; >> >> Well, yes, this is what I was asking for when mentioning the lack of use >> of the enumerator. Irrespective of that I remain unhappy with the name, >> though. >> > > How about DT_NUMA_DEF or do you have some suggestions for the name? Yeah, "DEFAULT" is probably the least bad one. Jan
diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h index 7d6ae36a19..52ca414e47 100644 --- a/xen/arch/arm/include/asm/numa.h +++ b/xen/arch/arm/include/asm/numa.h @@ -22,6 +22,12 @@ typedef u8 nodeid_t; */ #define NR_NODE_MEMBLKS NR_MEM_BANKS +enum dt_numa_status { + DT_NUMA_INIT, + DT_NUMA_ON, + DT_NUMA_OFF, +}; + #else /* Fake one node for now. See also node_online_map. */ @@ -39,6 +45,17 @@ extern mfn_t first_valid_mfn; #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) #define __node_distance(a, b) (20) +#define numa_disabled() (true) +static inline bool arch_numa_unavailable(void) +{ + return true; +} + +static inline bool arch_numa_broken(void) +{ + return true; +} + #endif #define arch_want_default_dmazone() (false) diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c new file mode 100644 index 0000000000..1c02b6a25d --- /dev/null +++ b/xen/arch/arm/numa.c @@ -0,0 +1,44 @@ +/* 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/numa.h> + +static enum dt_numa_status __read_mostly device_tree_numa; + +void __init numa_fw_bad(void) +{ + printk(KERN_ERR "NUMA: device tree numa info table not used.\n"); + device_tree_numa = DT_NUMA_OFF; +} + +bool __init arch_numa_unavailable(void) +{ + return device_tree_numa != DT_NUMA_ON; +} + +bool arch_numa_disabled(void) +{ + return device_tree_numa == DT_NUMA_OFF; +} + +int __init arch_numa_setup(const char *opt) +{ + return -EINVAL; +} diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h index 7866afa408..61efe60a95 100644 --- a/xen/arch/x86/include/asm/numa.h +++ b/xen/arch/x86/include/asm/numa.h @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n); #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) -extern bool numa_disabled(void); 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 b86d0851fc..7d7aeb3a3c 100644 --- a/xen/include/xen/numa.h +++ b/xen/include/xen/numa.h @@ -55,6 +55,7 @@ extern void numa_init_array(void); extern void numa_set_node(unsigned int cpu, nodeid_t node); extern void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn); extern void numa_fw_bad(void); +extern bool numa_disabled(void); extern int arch_numa_setup(const char *opt); extern bool arch_numa_unavailable(void);
NUMA has one global and one implementation specific switches. For ACPI NUMA implementation, Xen has acpi_numa, so we introduce device_tree_numa for device tree NUMA implementation. And use enumerations to indicate init, off and on status. arch_numa_disabled will get device_tree_numa status, but for arch_numa_setup we have not provided boot arguments to setup device_tree_numa. So we just return -EINVAL in this patch. Signed-off-by: Wei Chen <wei.chen@arm.com> --- v1 -> v2: 1. Use arch_numa_disabled to replace numa_enable_with_firmware. 2. Introduce enumerations for device tree numa status. 3. Use common numa_disabled, drop Arm version numa_disabled. 4. Introduce arch_numa_setup for Arm. 5. Rename bad_srat to numa_bad. 6. Add numa_enable_with_firmware helper. 7. Add numa_disabled helper. 8. Refine commit message. --- xen/arch/arm/include/asm/numa.h | 17 +++++++++++++ xen/arch/arm/numa.c | 44 +++++++++++++++++++++++++++++++++ xen/arch/x86/include/asm/numa.h | 1 - xen/include/xen/numa.h | 1 + 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 xen/arch/arm/numa.c