[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v1 10/21] ARM: NUMA: Add memory NUMA support



On Thu, Mar 2, 2017 at 9:35 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
>
> On 09/02/17 15:57, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> For all banks in bootinfo.mem, update nodes[] with
>> corresponding nodeid and register these nodes by
>> calling setup_node_bootmem().
>> compute memnode_shift and initialize memnodemap[] to fetch
>> nodeid for a given physical address.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/numa.c    | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/common/numa.c      | 14 ++++++++
>>  xen/include/xen/numa.h |  1 +
>>  3 files changed, 105 insertions(+)
>>
>> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
>> index d4dbad4..aa34c82 100644
>> --- a/xen/arch/arm/numa.c
>> +++ b/xen/arch/arm/numa.c
>> @@ -24,10 +24,15 @@
>>  #include <asm/acpi.h>
>>  #include <xen/errno.h>
>>  #include <xen/cpumask.h>
>> +#include <asm/setup.h>
>>
>>  int _node_distance[MAX_NUMNODES * 2];
>>  int *node_distance;
>>  extern nodemask_t numa_nodes_parsed;
>> +extern struct node nodes[MAX_NUMNODES] __initdata;
>> +extern int num_node_memblks;
>> +extern struct node node_memblk_range[NR_NODE_MEMBLKS];
>> +extern nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>>
>>  void __init numa_set_cpu_node(int cpu, unsigned long hwid)
>>  {
>> @@ -51,6 +56,88 @@ u8 __node_distance(nodeid_t a, nodeid_t b)
>>
>>  EXPORT_SYMBOL(__node_distance);
>>
>> +static int __init numa_mem_init(void)
>
>
> The function return only 2 different value either 0 or -EINVAL. It should be
> better to use a boolean.
>
>> +{
>> +    nodemask_t memory_nodes_parsed;
>> +    int bank, nodeid;
>> +    struct node *nd;
>> +    paddr_t start, size, end;
>> +
>> +    nodes_clear(memory_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_numa_node(start, end);
>> +        if ( nodeid == -EINVAL || nodeid > MAX_NUMNODES )
>
>
> If found, how the nodeid could be invalid? IHMO, this is a parsing bug.
>
>> +        {
>> +            printk(XENLOG_WARNING
>> +                   "NUMA: node for mem bank start 0x%lx - 0x%lx not
>> found\n",
>> +                   start, end);
>> +
>> +            return -EINVAL;
>> +        }
>> +
>> +        nd = &nodes[nodeid];
>> +        if ( !node_test_and_set(nodeid, memory_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 quite confusing. What is the purpose? Why do you go through
> bootinfo.mem and not node_memblk_range?

node_memblk_range[] contains memory bank to node info read from DT memory node.
We go through bootinfo.mem and populate nodes[] with actual memory
range indexed by
nodeid.

I think this is similar to nodes_cover_memory in arch/x86/srat.c

>
>> +
>> +    return 0;
>> +}
>> +
>> +/* Use the information discovered above to actually set up the nodes. */
>> +static int __init numa_scan_mem_nodes(void)
>
>
> This function looks very similar to acpi_scan_nodes on x86. This is call to
> move more code in common.
>
Yes, I have already fixed it in next revision.

>
>> +{
>> +    int i;
>> +
>> +    memnode_shift = compute_hash_shift(node_memblk_range,
>> num_node_memblks,
>> +                                       memblk_nodeid);
>> +    if ( memnode_shift < 0 )
>> +    {
>> +        printk(XENLOG_WARNING "No NUMA hash found.\n");
>> +        memnode_shift = 0;
>> +    }
>> +
>> +    for_each_node_mask(i, numa_nodes_parsed)
>> +    {
>> +        u64 size = node_memblk_range[i].end - node_memblk_range[i].start;
>> +
>> +        if ( size == 0 )
>> +            printk(XENLOG_WARNING "NUMA: Node %u has no memory. \n", i);
>> +
>> +        printk(XENLOG_INFO
>> +               "NUMA: NODE[%d]: Start 0x%lx End 0x%lx\n",
>> +               i, nodes[i].start, nodes[i].end);
>> +        setup_node_bootmem(i, nodes[i].start, nodes[i].end);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init numa_initmem_init(void)
>> +{
>> +    if ( !numa_mem_init() )
>> +    {
>> +        if ( !numa_scan_mem_nodes() )
>> +            return 0;
>> +    }
>
>
> This could be simplified with
>
> if ( !numa_mem_init() && !num_scan_mem_nodes )
>    return 0;
>
> However looking at the usage, this function seems rather pointless.

OK. I have changed this in next revision.

>
>> +
>> +    return -EINVAL;
>> +}
>> +
>>  /*
>>   * Setup early cpu_to_node.
>>   */
>> @@ -74,6 +161,9 @@ int __init numa_init(void)
>>
>>      ret = dt_numa_init();
>>
>> +    if ( !ret )
>> +        ret = numa_initmem_init();
>> +
>>  no_numa:
>>      return ret;
>>  }
>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>> index 62c76af..2f5266a 100644
>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -63,6 +63,20 @@ void __init numa_add_memblk(nodeid_t nodeid, u64 start,
>> u64 size)
>>      num_node_memblks++;
>>  }
>>
>> +int __init get_numa_node(u64 start, u64 end)
>
>
> I think this function will be confusing for the caller. Why using
> num_node_memblks and not the node itself?

OK. Wll check
>
>> +{
>> +    int i;
>
>
> Please unsigned int
>
>> +
>> +    for ( i = 0; i < num_node_memblks; i++ )
>> +    {
>> +        if ( start >= node_memblk_range[i].start &&
>> +             end <= node_memblk_range[i].end )
>> +            return memblk_nodeid[i];
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>>  int valid_numa_range(u64 start, u64 end, nodeid_t node)
>>  {
>>  #ifdef CONFIG_NUMA
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 9392a89..4f04ab4 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -68,6 +68,7 @@ static inline __attribute__((pure)) nodeid_t
>> phys_to_nid(paddr_t addr)
>>  #endif /* CONFIG_NUMA */
>>
>>  extern void numa_add_memblk(nodeid_t nodeid, u64 start, u64 size);
>> +extern int get_numa_node(u64 start, u64 end);
>>  extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
>>  extern int conflicting_memblks(u64 start, u64 end);
>>  extern void cutoff_node(int i, u64 start, u64 end);
>>
>
> Regards,
>
> --
> Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.