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

Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information



On Mon, Feb 20, 2017 at 11:35 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>
>>
>> Parse memory node and fetch numa-node-id information.
>> For each memory range, store in node_memblk_range[]
>> along with node id.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/bootfdt.c        |  4 +--
>>  xen/arch/arm/dt_numa.c        | 84
>> ++++++++++++++++++++++++++++++++++++++++++-
>>  xen/common/numa.c             |  8 +++++
>>  xen/include/xen/device_tree.h |  3 ++
>>  xen/include/xen/numa.h        |  1 +
>>  5 files changed, 97 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index d1122d8..5e2df92 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -56,8 +56,8 @@ static bool_t __init device_tree_node_compatible(const
>> void *fdt, int node,
>>      return 0;
>>  }
>>
>> -static void __init device_tree_get_reg(const __be32 **cell, u32
>> address_cells,
>> -                                       u32 size_cells, u64 *start, u64
>> *size)
>> +void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>> +                                u32 size_cells, u64 *start, u64 *size)
>>  {
>>      *start = dt_next_cell(address_cells, cell);
>>      *size = dt_next_cell(size_cells, cell);
>> diff --git a/xen/arch/arm/dt_numa.c b/xen/arch/arm/dt_numa.c
>> index 4b94c36..fce9e67 100644
>> --- a/xen/arch/arm/dt_numa.c
>> +++ b/xen/arch/arm/dt_numa.c
>> @@ -27,6 +27,7 @@
>>  #include <xen/numa.h>
>>
>>  nodemask_t numa_nodes_parsed;
>> +extern struct node node_memblk_range[NR_NODE_MEMBLKS];
>
>
> This should have been declared in an header (likely in patch #3).
>
>>
>>  /*
>>   * Even though we connect cpus to numa domains later in SMP
>> @@ -48,11 +49,73 @@ static int __init dt_numa_process_cpu_node(const void
>> *fdt, int node,
>>      return 0;
>>  }
>>
>> +static int __init dt_numa_process_memory_node(const void *fdt, int node,
>> +                                              const char *name,
>> +                                              u32 address_cells,
>> +                                              u32 size_cells)
>
>
> Rather than reimplementing the wheel, it might be better to hook the parsing
> in bootfdt.c. This would avoid an extra parsing of the device-tree,
> duplicate the code and expose primitive for early DT parsing.

The process_memory_node() is called only if EFI_BOOT is not enabled. So
hooking might not work.

>
> If parsing NUMA information can be done after the DT has been unflattened,
> this will be even better.
>
>> +{
>> +    const struct fdt_property *prop;
>> +    int i, ret, banks;
>
>
> Both banks and i should be unsigned.
>
>> +    const __be32 *cell;
>> +    paddr_t start, size;
>> +    u32 reg_cells = address_cells + size_cells;
>> +    u32 nid;
>> +
>> +    if ( address_cells < 1 || size_cells < 1 )
>> +    {
>> +        printk(XENLOG_WARNING
>> +               "fdt: node `%s': invalid #address-cells or #size-cells",
>> name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
>> +    if ( nid >= MAX_NUMNODES) {
>
>
> Coding style
>
> if ( ... )
> {
>
>> +        /*
>> +         * No node id found. Skip this memory node.
>> +         */
>
>
> This could be a single line:
>
> /* ..... */
>
> So no warning if it is impossible to get the numa-node-id? Also, I don't
> think this is right to boot using NUMA on platform having a buggy DT. So we
> should probably return an error and disable NUMA.

OK.
>
>> +        return 0;
>> +    }
>> +
>> +    prop = fdt_get_property(fdt, node, "reg", NULL);
>> +    if ( !prop )
>> +    {
>> +        printk(XENLOG_WARNING "fdt: node `%s': missing `reg' property\n",
>> +               name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    cell = (const __be32 *)prop->data;
>> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>> +
>> +    for ( i = 0; i < banks; i++ )
>> +    {
>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start,
>> &size);
>> +        if ( !size )
>> +            continue;
>> +
>> +        /* It is fine to add this area to the nodes data it will be used
>> later*/
>> +        ret = conflicting_memblks(start, start + size);
>> +        if (ret < 0)
>> +             numa_add_memblk(nid, start, size);
>
>
> numa_add_memblk rely on the caller to check whether the array is not full. I
> think we should move the check in numa_add_memblk and return an error in
> this case.

OK
>
>> +        else
>> +        {
>> +             printk(XENLOG_ERR
>> +                    "NUMA DT: node %u (%"PRIx64"-%"PRIx64") overlaps with
>> ret %d (%"PRIx64"-%"PRIx64")\n",
>> +                    nid, start, start + size, ret,
>> +                    node_memblk_range[i].start,
>> node_memblk_range[i].end);
>
>
> i != ret. Please use the correct variable accordingly. However, I don't
> think the overlap range really matters here.

OK
>
>> +             return -EINVAL;
>> +        }
>> +    }
>> +
>> +    node_set(nid, numa_nodes_parsed);
>> +
>> +    return 0;
>> +}
>> +
>>  static int __init dt_numa_scan_cpu_node(const void *fdt, int node,
>>                                          const char *name, int depth,
>>                                          u32 address_cells, u32
>> size_cells,
>>                                          void *data)
>> -
>
>
> Spurious change. Please don't add the blank line at the first place (patch
> #6).
>
>>  {
>>      if ( device_tree_node_matches(fdt, node, "cpu") )
>>          return dt_numa_process_cpu_node(fdt, node, name, address_cells,
>> @@ -61,6 +124,18 @@ static int __init dt_numa_scan_cpu_node(const void
>> *fdt, int node,
>>      return 0;
>>  }
>>
>> +static int __init dt_numa_scan_memory_node(const void *fdt, int node,
>> +                                           const char *name, int depth,
>> +                                           u32 address_cells, u32
>> size_cells,
>> +                                           void *data)
>> +{
>> +    if ( device_tree_node_matches(fdt, node, "memory") )
>> +        return dt_numa_process_memory_node(fdt, node, name,
>> address_cells,
>> +                                           size_cells);
>
>
> Similarly to the CPUs, this code is wrong. You should check the type =
> "memory".

 if (!dt_node_type(node, "memory") ) should be fine?

>
>
>> +
>> +    return 0;
>> +}
>> +
>>  int __init dt_numa_init(void)
>>  {
>>      int ret;
>> @@ -68,5 +143,12 @@ int __init dt_numa_init(void)
>>      nodes_clear(numa_nodes_parsed);
>>      ret = device_tree_for_each_node((void *)device_tree_flattened,
>>                                      dt_numa_scan_cpu_node, NULL);
>> +
>> +    if ( ret )
>> +        return ret;
>> +
>> +    ret = device_tree_for_each_node((void *)device_tree_flattened,
>> +                                    dt_numa_scan_memory_node, NULL);
>> +
>>      return ret;
>>  }
>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>> index 9b9cf9c..62c76af 100644
>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -55,6 +55,14 @@ struct node node_memblk_range[NR_NODE_MEMBLKS];
>>  nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>>  struct node nodes[MAX_NUMNODES] __initdata;
>>
>> +void __init numa_add_memblk(nodeid_t nodeid, u64 start, u64 size)
>
>
> Please replace u64 by paddr_t.
>
>> +{
>> +    node_memblk_range[num_node_memblks].start = start;
>> +    node_memblk_range[num_node_memblks].end = start + size;
>> +    memblk_nodeid[num_node_memblks] = nodeid;
>> +    num_node_memblks++;
>> +}
>
>
> You probably want to factor the code in acpi_numa_memory_affinity_init to
> create this function.
>
> Also, you don't check if the array is full.

I think x86 can use this. I will make it part of initial code clean up.
>
>> +
>>  int valid_numa_range(u64 start, u64 end, nodeid_t node)
>>  {
>>  #ifdef CONFIG_NUMA
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index de6b351..d92e47e 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -192,6 +192,9 @@ bool_t device_tree_node_matches(const void *fdt, int
>> node,
>>                                  const char *match);
>>  u32 device_tree_get_u32(const void *fdt, int node,
>>                          const char *prop_name, u32 dflt);
>> +void device_tree_get_reg(const __be32 **cell, u32 address_cells,
>> +                         u32 size_cells, u64 *start, u64 *size);
>> +
>
>
> Same remark as on patch #6 for the position of the declaration.
>
>>  /**
>>   * dt_unflatten_host_device_tree - Unflatten the host device tree
>>   *
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 77c5cfd..9392a89 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -67,6 +67,7 @@ static inline __attribute__((pure)) nodeid_t
>> phys_to_nid(paddr_t addr)
>>  #define clear_node_cpumask(cpu) do {} while (0)
>>  #endif /* CONFIG_NUMA */
>>
>> +extern void numa_add_memblk(nodeid_t nodeid, u64 start, u64 size);
>>  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®.