Re: [Xen-devel] [RFC PATCH v1 04/21] NUMA: Refactor generic and arch specific code of numa_setup

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Feb 20, 2017 at 7:09 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
>
> On 09/02/17 15:56, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> numa_setup() contains generic and arch specific code.
>> Split numa_setup() and move architecture specific code
>> under arch_numa_setup().
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/Makefile      |  1 +
>>  xen/arch/arm/numa.c        | 28 ++++++++++++++++++++++++++++
>>  xen/arch/x86/numa.c        | 11 +----------
>>  xen/common/numa.c          | 14 ++++++++++++++
>>  xen/include/asm-arm/numa.h |  9 ++++++++-
>>  xen/include/asm-x86/numa.h |  2 +-
>>  xen/include/xen/numa.h     |  1 +
>>  7 files changed, 54 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7afb8a3..b5d7a19 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -49,6 +49,7 @@ obj-y += vm_event.o
>>  obj-y += vtimer.o
>>  obj-y += vpsci.o
>>  obj-y += vuart.o
>> +obj-$(CONFIG_NUMA) += numa.o
>>
>>  #obj-bin-y += ....o
>>
>> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
>> new file mode 100644
>> index 0000000..59d09c7
>> --- /dev/null
>> +++ b/xen/arch/arm/numa.c
>> @@ -0,0 +1,28 @@
>> +/*
>> + * ARM NUMA Implementation
>> + *
>> + * Copyright (C) 2016 - Cavium Inc.
>> + * Vijaya Kumar K <vijaya.kumar@xxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
>
> The copyright is wrong.
>
>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/ctype.h>
>> +#include <xen/mm.h>
>> +#include <xen/nodemask.h>
>> +#include <asm/mm.h>
>> +#include <xen/numa.h>
>> +
>> +int __init arch_numa_setup(char *opt)
>> +{
>> +    return 1;
>> +}
>> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
>> index bc787e0..28d1891 100644
>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -18,9 +18,6 @@
>>  #include <xen/sched.h>
>>  #include <xen/softirq.h>
>>
>> -static int numa_setup(char *s);
>> -custom_param("numa", numa_setup);
>> -
>>  #ifndef Dprintk
>>  #define Dprintk(x...)
>>  #endif
>> @@ -34,7 +31,6 @@ nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
>>
>>  nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>>
>> -bool_t numa_off = 0;
>>  s8 acpi_numa = 0;
>>
>>  int srat_disabled(void)
>> @@ -145,13 +141,8 @@ void __init numa_initmem_init(unsigned long
>> start_pfn, unsigned long end_pfn)
>>                      (u64)end_pfn << PAGE_SHIFT);
>>  }
>>
>> -/* [numa=off] */
>> -static __init int numa_setup(char *opt)
>> +int __init arch_numa_setup(char *opt)
>
>
> I don't understand why you split numa_setup. All the options look valid for
> ARM.

OK.  This is all valid for arm, provided CONFIG_NUMA_EMU is implemented.
Can be moved to generic and for now we can keep CONFIG_NUMA_EMU
disabled for arm.

>
>>  {
>> -    if ( !strncmp(opt,"off",3) )
>> -        numa_off = 1;
>> -    if ( !strncmp(opt,"on",2) )
>> -        numa_off = 0;
>>  #ifdef CONFIG_NUMA_EMU
>>      if ( !strncmp(opt, "fake=", 5) )
>>      {
>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>> index 13f147c..9b9cf9c 100644
>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -32,6 +32,10 @@
>>  #include <xen/softirq.h>
>>  #include <asm/setup.h>
>>
>> +static int numa_setup(char *s);
>
>
> Forward declaration can be avoided in most of the cases. So please add at
> the right place from the beginning.
>
>
>> +custom_param("numa", numa_setup);
>> +
>> +bool_t numa_off = 0;
>>  struct node_data node_data[MAX_NUMNODES];
>>
>>  /* Mapping from pdx to node id */
>> @@ -250,6 +254,16 @@ EXPORT_SYMBOL(memnode_shift);
>>  EXPORT_SYMBOL(memnodemap);
>>  EXPORT_SYMBOL(node_data);
>>
>> +static __init int numa_setup(char *opt)
>> +{
>> +    if ( !strncmp(opt,"off",3) )
>> +        numa_off = 1;
>> +    if ( !strncmp(opt,"on",2) )
>> +        numa_off = 0;
>> +
>> +    return arch_numa_setup(opt);
>> +}
>> +
>>  static void dump_numa(unsigned char key)
>>  {
>>      s_time_t now = NOW();
>> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
>> index a60c7eb..c1e8a7d 100644
>> --- a/xen/include/asm-arm/numa.h
>> +++ b/xen/include/asm-arm/numa.h
>> @@ -5,7 +5,14 @@ typedef u8 nodeid_t;
>>
>>  #define NODES_SHIFT 2
>>
>> -#ifndef CONFIG_NUMA
>> +#ifdef CONFIG_NUMA
>> +int arch_numa_setup(char *opt);
>> +#else
>> +static inline int arch_numa_setup(char *opt)
>> +{
>> +    return 1;
>> +}
>
>
> What is the point of this?
>
>
>> +
>>  /* Fake one node for now. See also node_online_map. */
>>  #define cpu_to_node(cpu) 0
>>  #define node_to_cpumask(node)   (cpu_online_map)
>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>> index df1f7d5..659ff6a 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 void numa_init_array(void);
>> -extern bool_t numa_off;
>>
>>  extern int srat_disabled(void);
>>  extern void srat_detect_node(int cpu);
>> @@ -32,5 +31,6 @@ extern nodeid_t apicid_to_node[];
>>  void srat_parse_regions(u64 addr);
>>  extern u8 __node_distance(nodeid_t a, nodeid_t b);
>>  unsigned int arch_get_dma_bitsize(void);
>> +int arch_numa_setup(char *opt);
>>
>>  #endif
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 810f742..77c5cfd 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -18,6 +18,7 @@
>>    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
>>     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>>
>> +extern bool_t numa_off;
>
>
> The place you added looks a bit random. And would be surprised to see a need
> when NUMA is not enabled.

Ok. I will check.

>
>>  struct node {
>>         u64 start,end;
>>  };
>>
>
> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel