Re: [RFC PATCH v3 16/24] ARM: NUMA: Add memory NUMA support

[Julien Grall <julien.grall@arm.com>]

Hi Vijay,

On 18/07/17 12:41, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Implement arch_sanitize_nodes_memory() which looks at all banks
> in bootinfo.mem, update nodes[] with corresponding nodeid.
> Call numa_scan_nodes() generic function with ram start and
> end address, which takes care of further computing memnodeshift
> and populating memnodemap[] using generic implementation.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> v3: - Dropped common code from asm-arm/numa.h
>     - Re-used numa_initmem_init() from common code.
> ---
>  xen/arch/arm/numa/numa.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++-
>  xen/common/numa.c        | 14 +++++++++
>  xen/include/xen/numa.h   |  1 +
>  3 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/numa/numa.c b/xen/arch/arm/numa/numa.c
> index dc80aa5..85352dc 100644
> --- a/xen/arch/arm/numa/numa.c
> +++ b/xen/arch/arm/numa/numa.c
> @@ -18,6 +18,7 @@
>  #include <xen/ctype.h>
>  #include <xen/nodemask.h>
>  #include <xen/numa.h>
> +#include <xen/pfn.h>
>  #include <asm/acpi.h>
>
>  static uint8_t (*node_distance_fn)(nodeid_t a, nodeid_t b);
> @@ -64,9 +65,66 @@ void register_node_distance(uint8_t (fn)(nodeid_t a, nodeid_t b))
>      node_distance_fn = fn;
>  }
>
> +bool __init arch_sanitize_nodes_memory(void)

Likely when I say a function is confusing on a previous version, it 
means you have to add more comments in the function...

> +{
> +    nodemask_t mem_nodes_parsed;
> +    int bank, nodeid;
> +    struct node *nd;
> +    paddr_t start, size, end;
> +
> +    nodes_clear(mem_nodes_parsed);
> +    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
> +    {
> +        start = bootinfo.mem.bank[bank].start;
> +        size = bootinfo.mem.bank[bank].size;
> +        end = start + size;
> +
> +        nodeid = get_mem_nodeid(start, end);
> +        if ( nodeid >= NUMA_NO_NODE )

This check is very confusing.

> +        {
> +            printk(XENLOG_WARNING
> +                   "NUMA: node for mem bank start 0x%lx - 0x%lx not found\n",

start and end are paddr_t should it should be PRI_paddr.

> +                   start, end);
> +
> +            return false;
> +        }
> +
> +        nd = get_numa_node(nodeid);
> +        if ( !node_test_and_set(nodeid, mem_nodes_parsed) )
> +        {
> +            nd->start = start;
> +            nd->end = end;
> +        }
> +        else
> +        {
> +            if ( start < nd->start )
> +                nd->start = start;
> +            if ( nd->end < end )
> +                nd->end = end;

This function is called "sanitize nodes", but here you also update 
start/end. Mind explaining why you need this on ARM when it is not 
necessary on x86?

> +        }
> +    }
> +
> +    return true;
> +}
> +
> +static void __init numa_reset_numa_nodes(void)
> +{
> +    int i;
> +    struct node *nd;
> +
> +    for ( i = 0; i < MAX_NUMNODES; i++ )
> +    {
> +        nd = get_numa_node(i);
> +        nd->start = 0;
> +        nd->end = 0;
> +    }
> +}
> +
>  void __init numa_init(void)
>  {
> -    int ret = 0;
> +    int ret = 0, bank;
> +    paddr_t ram_start = ~0;
> +    paddr_t ram_end = 0;
>
>      nodes_clear(processor_nodes_parsed);
>      init_cpu_to_node();
> @@ -83,6 +141,23 @@ void __init numa_init(void)
>      }
>
>  no_numa:
> +    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
> +    {
> +        paddr_t bank_start = bootinfo.mem.bank[bank].start;
> +        paddr_t bank_end = bank_start + bootinfo.mem.bank[bank].size;
> +
> +        ram_start = min(ram_start, bank_start);
> +        ram_end = max(ram_end, bank_end);
> +    }
> +
> +    /*
> +     * In arch_sanitize_nodes_memory() we update nodes[] properly.
> +     * Hence we reset the nodes[] before calling numa_scan_nodes().
> +     */
> +    numa_reset_numa_nodes();
> +
> +    numa_initmem_init(PFN_UP(ram_start), PFN_DOWN(ram_end));

I might miss something. numa_initmem_init is here to scan the NUMA nodes 
and set them up. So why are you calling it here?

> +
>      return;
>  }
>
> diff --git a/xen/common/numa.c b/xen/common/numa.c
> index 5e985d2..0f79a07 100644
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -76,6 +76,20 @@ nodeid_t get_memblk_nodeid(unsigned int id)
>      return memblk_nodeid[id];
>  }
>
> +int __init get_mem_nodeid(paddr_t start, paddr_t end)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < get_num_node_memblks(); i++ )
> +    {
> +        if ( start >= node_memblk_range[i].start &&
> +             end <= node_memblk_range[i].end )
> +            return memblk_nodeid[i];
> +    }
> +
> +    return -EINVAL;
> +}
> +
>  static nodeid_t *get_memblk_nodeid_map(void)
>  {
>      return &memblk_nodeid[0];
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index 8a306e7..a541eb7 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -70,6 +70,7 @@ struct node *get_numa_node(unsigned int id);
>  nodeid_t get_memblk_nodeid(unsigned int memblk);
>  struct node *get_node_memblk_range(unsigned int memblk);
>  int numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t size);
> +int get_mem_nodeid(paddr_t start, paddr_t end);
>  int get_num_node_memblks(void);
>  bool arch_sanitize_nodes_memory(void);
>  void numa_failed(void);
>

Cheers,

-- 
Julien Grall

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