| Message ID | 20220822025810.2240707-2-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree based NUMA support for Arm - Part#2 | expand |
On 22.08.2022 04:58, Wei Chen wrote: > --- a/xen/arch/x86/include/asm/numa.h > +++ b/xen/arch/x86/include/asm/numa.h > @@ -32,8 +32,9 @@ extern void numa_add_cpu(int cpu); > extern void numa_init_array(void); > extern bool numa_off; > > - > -extern int srat_disabled(void); > +extern int arch_numa_setup(const char *opt); > +extern bool arch_numa_disabled(bool init_as_disable); What is the parameter name intended to mean? Since the only caller passes "false", this also isn't really possible to guess from the use(s) in this patch. In any event perhaps best for the parameter to be introduced only once it's actually needed. > --- a/xen/arch/x86/numa.c > +++ b/xen/arch/x86/numa.c > @@ -50,9 +50,31 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; > bool numa_off; > s8 acpi_numa = 0; > > -int srat_disabled(void) > +int __init arch_numa_setup(const char *opt) > { > - return numa_off || acpi_numa < 0; > +#ifdef CONFIG_ACPI_NUMA > + if ( !strncmp(opt, "noacpi", 6) ) > + { > + numa_off = false; > + acpi_numa = -1; > + return 0; With this "return" ... > + } > + else ... this "else" is unnecessary and hence would better be dropped, not the least to ... > +#endif > + return -EINVAL; ... avoid the otherwise ambiguous indentation of this line. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年8月25日 18:18 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v3 1/6] xen/x86: Provide helpers for common code to > access acpi_numa > > On 22.08.2022 04:58, Wei Chen wrote: > > --- a/xen/arch/x86/include/asm/numa.h > > +++ b/xen/arch/x86/include/asm/numa.h > > @@ -32,8 +32,9 @@ extern void numa_add_cpu(int cpu); > > extern void numa_init_array(void); > > extern bool numa_off; > > > > - > > -extern int srat_disabled(void); > > +extern int arch_numa_setup(const char *opt); > > +extern bool arch_numa_disabled(bool init_as_disable); > > What is the parameter name intended to mean? Since the only caller > passes "false", this also isn't really possible to guess from the > use(s) in this patch. In any event perhaps best for the parameter > to be introduced only once it's actually needed. > This parameter will be used in patch#5 and set to true, I will introduce this parameter in that patch. > > --- a/xen/arch/x86/numa.c > > +++ b/xen/arch/x86/numa.c > > @@ -50,9 +50,31 @@ nodemask_t __read_mostly node_online_map = { { [0] = > 1UL } }; > > bool numa_off; > > s8 acpi_numa = 0; > > > > -int srat_disabled(void) > > +int __init arch_numa_setup(const char *opt) > > { > > - return numa_off || acpi_numa < 0; > > +#ifdef CONFIG_ACPI_NUMA > > + if ( !strncmp(opt, "noacpi", 6) ) > > + { > > + numa_off = false; > > + acpi_numa = -1; > > + return 0; > > With this "return" ... > > > + } > > + else > > ... this "else" is unnecessary and hence would better be dropped, > not the least to ... > > > +#endif > > + return -EINVAL; > > ... avoid the otherwise ambiguous indentation of this line. > This is a good suggestion, current indentation looks weird, I will fix above 3 in next version. Thanks, Wei Che > Jan
diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h index c32ccffde3..f41fe20f9b 100644 --- a/xen/arch/x86/include/asm/numa.h +++ b/xen/arch/x86/include/asm/numa.h @@ -32,8 +32,9 @@ extern void numa_add_cpu(int cpu); extern void numa_init_array(void); extern bool numa_off; - -extern int srat_disabled(void); +extern int arch_numa_setup(const char *opt); +extern bool arch_numa_disabled(bool init_as_disable); +extern bool 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/arch/x86/numa.c b/xen/arch/x86/numa.c index 627ae8aa95..3d3081c967 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c @@ -50,9 +50,31 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; bool numa_off; s8 acpi_numa = 0; -int srat_disabled(void) +int __init arch_numa_setup(const char *opt) { - return numa_off || acpi_numa < 0; +#ifdef CONFIG_ACPI_NUMA + if ( !strncmp(opt, "noacpi", 6) ) + { + numa_off = false; + acpi_numa = -1; + return 0; + } + else +#endif + return -EINVAL; +} + +bool arch_numa_disabled(bool init_as_disable) +{ + if ( !init_as_disable ) + return acpi_numa < 0; + + return acpi_numa <= 0; +} + +bool srat_disabled(void) +{ + return numa_off || arch_numa_disabled(false); } /* @@ -291,28 +313,21 @@ void numa_set_node(int cpu, nodeid_t node) /* [numa=off] */ static int __init cf_check numa_setup(const char *opt) { - if ( !strncmp(opt,"off",3) ) + if ( !strncmp(opt, "off", 3) ) numa_off = true; - else if ( !strncmp(opt,"on",2) ) + else if ( !strncmp(opt, "on", 2) ) numa_off = false; #ifdef CONFIG_NUMA_EMU else if ( !strncmp(opt, "fake=", 5) ) { numa_off = false; - numa_fake = simple_strtoul(opt+5,NULL,0); + numa_fake = simple_strtoul(opt + 5, NULL, 0); if ( numa_fake >= MAX_NUMNODES ) numa_fake = MAX_NUMNODES; } -#endif -#ifdef CONFIG_ACPI_NUMA - else if ( !strncmp(opt,"noacpi",6) ) - { - numa_off = false; - acpi_numa = -1; - } #endif else - return -EINVAL; + return arch_numa_setup(opt); return 0; }
acpi_numa is a specific NUMA switch for ACPI NUMA implementation. Other NUMA implementation may not need this switch. But this switch is not only used by ACPI code, it is also used directly in some general NUMA logic code. So far this hasn't caused any problem because Xen only has x86 implementing ACPI NUMA, but now Arm is implementing device tree based NUMA. Accesssing acpi_numa directly in some functions will be a block of reusing NUMA common code. It is also difficult for us to replace it with a new generic switch, because it is hard to prove that the new switch states can guarantee the original code will work correctly. So in this patch, we provide two helpers for common code to update and get states of acpi_numa. And other new NUMA implementations just need to provide the same helpers for common code. In this case, the generic NUMA logic code can be reused by all NUMA implementations. Signed-off-by: Wei Chen <wei.chen@arm.com> --- v2 -> v3: 1. Drop enumeration of numa status. 2. Use helpers to get/update acpi_numa. 3. Insert spaces among parameters of strncmp in numa_setup. v1 -> v2: 1. Remove fw_numa. 2. Use enumeration to replace numa_off and acpi_numa. 3. Correct return value of srat_disabled. 4. Introduce numa_enabled_with_firmware. --- xen/arch/x86/include/asm/numa.h | 5 ++-- xen/arch/x86/numa.c | 41 ++++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 15 deletions(-)