|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v1 08/21] ARM: NUMA: Parse NUMA distance information
On Mon, Feb 20, 2017 at 11:58 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>
>>
>> Parse distance-matrix and fetch node distance information.
>> Store distance information in node_distance[].
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>> xen/arch/arm/dt_numa.c | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/numa.c | 19 +++++++++-
>> xen/include/asm-arm/numa.h | 1 +
>> 3 files changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/dt_numa.c b/xen/arch/arm/dt_numa.c
>> index fce9e67..8979612 100644
>> --- a/xen/arch/arm/dt_numa.c
>> +++ b/xen/arch/arm/dt_numa.c
>> @@ -28,6 +28,19 @@
>>
>> nodemask_t numa_nodes_parsed;
>> extern struct node node_memblk_range[NR_NODE_MEMBLKS];
>> +extern int _node_distance[MAX_NUMNODES * 2];
>> +extern int *node_distance;
>
>
> I don't like this idea of having _node_distance and node_distance. Looking
> at the code, I see little point to do that. You could just initialize
> node_distance with the correct value.
>
> Also the node distance can fit in u8, so you can save memory by using u8.
u8 might restrict the distance value
>
> Lastly, I am not sure why you pre-allocate the memory. The distance table.
> could be quite big.
ok. will do malloc
>
>> +
>> +static int numa_set_distance(u32 nodea, u32 nodeb, u32 distance)
>
>
> Please avoid the use of u32 in favor of uint32_t.
>
> Also, this function does not look very DT specific.
>
>> +{
>> + if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES )
>> + return -EINVAL;
>> +
>
>
> I would have expected some sanity check here.
>
>
>> + _node_distance[(nodea * MAX_NUMNODES) + nodeb] = distance;
>> + node_distance = &_node_distance[0];
>> +
>> + return 0;
>> +}
>>
>> /*
>> * Even though we connect cpus to numa domains later in SMP
>> @@ -112,6 +125,66 @@ static int __init dt_numa_process_memory_node(const
>> void *fdt, int node,
>> return 0;
>> }
>>
>> +static int __init dt_numa_parse_distance_map(const void *fdt, int node,
>> + const char *name,
>> + u32 address_cells,
>> + u32 size_cells)
>> +{
>> + const struct fdt_property *prop;
>> + const __be32 *matrix;
>> + int entry_count, len, i;
>> +
>> + printk(XENLOG_INFO "NUMA: parsing numa-distance-map\n");
>> +
>> + prop = fdt_get_property(fdt, node, "distance-matrix", &len);
>> + if ( !prop )
>> + {
>> + printk(XENLOG_WARNING
>> + "NUMA: No distance-matrix property in distance-map\n");
>> +
>> + return -EINVAL;
>> + }
>> +
>> + if ( len % sizeof(u32) != 0 )
>> + {
>> + printk(XENLOG_WARNING
>> + "distance-matrix in node is not a multiple of u32\n");
>> +
>> + return -EINVAL;
>> + }
>> +
>> + entry_count = len / sizeof(u32);
>> + if ( entry_count <= 0 )
>> + {
>> + printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
>> +
>> + return -EINVAL;
>> + }
>> +
>> + matrix = (const __be32 *)prop->data;
>> + for ( i = 0; i + 2 < entry_count; i += 3 )
>> + {
>> + u32 nodea, nodeb, distance;
>> +
>> + nodea = dt_read_number(matrix, 1);
>> + matrix++;
>> + nodeb = dt_read_number(matrix, 1);
>> + matrix++;
>> + distance = dt_read_number(matrix, 1);
>> + matrix++;
>> +
>> + numa_set_distance(nodea, nodeb, distance);
>
>
> What if numa_set_distance failed?
IMO, no need to fail numa. Should be set to default values for node_distance[].
>
>> + printk(XENLOG_INFO "NUMA: distance[node%d -> node%d] = %d\n",
>> + nodea, nodeb, distance);
>> +
>> + /* Set default distance of node B->A same as A->B */
>> + if ( nodeb > nodea )
>> + numa_set_distance(nodeb, nodea, distance);
>> + }
>> +
>> + 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,
>> @@ -136,6 +209,18 @@ static int __init dt_numa_scan_memory_node(const void
>> *fdt, int node,
>> return 0;
>> }
>>
>> +static int __init dt_numa_scan_distance_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, "distance-map") )
>
>
> Similar to memory and cpu, the name is not fixed. What you want to look for
> is the compatible numa-distance-map-v1.
ok
>
>
>> + return dt_numa_parse_distance_map(fdt, node, name, address_cells,
>> + size_cells);
>> +
>> + return 0;
>> +}
>> +
>> int __init dt_numa_init(void)
>> {
>> int ret;
>> @@ -149,6 +234,11 @@ int __init dt_numa_init(void)
>>
>> ret = device_tree_for_each_node((void *)device_tree_flattened,
>> dt_numa_scan_memory_node, NULL);
>> + if ( ret )
>> + return ret;
>> +
>> + ret = device_tree_for_each_node((void *)device_tree_flattened,
>> + dt_numa_scan_distance_node, NULL);
>>
>> return ret;
>> }
>> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
>> index 9a7f0bb..11d100b 100644
>> --- a/xen/arch/arm/numa.c
>> +++ b/xen/arch/arm/numa.c
>> @@ -22,14 +22,31 @@
>> #include <asm/mm.h>
>> #include <xen/numa.h>
>> #include <asm/acpi.h>
>> +#include <xen/errno.h>
>
>
> Why did you add this include. I don't see any errno here.
>
>> +
>> +int _node_distance[MAX_NUMNODES * 2];
>> +int *node_distance;
>> +
>> +u8 __node_distance(nodeid_t a, nodeid_t b)
>> +{
>> + if ( !node_distance )
>> + return a == b ? 10 : 20;
>
>
> Why does the 10/20 comes from?
That is default distance value.
>
>> +
>> + return _node_distance[a * MAX_NUMNODES + b];
>> +}
>> +
>> +EXPORT_SYMBOL(__node_distance);
>>
>> int __init numa_init(void)
>> {
>> - int ret = 0;
>> + int i, ret = 0;
>>
>> if ( numa_off )
>> goto no_numa;
>>
>> + for ( i = 0; i < MAX_NUMNODES * 2; i++ )
>> + _node_distance[i] = 0;
>> +
>
>
> Hmmmm... _node_distance will be zeroed by the compiler. So why that?
>
> If you want to initialize correctly then use 10/20.
>
>> ret = dt_numa_init();
>>
>> no_numa:
>> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
>> index cdfeecd..b8857e2 100644
>> --- a/xen/include/asm-arm/numa.h
>> +++ b/xen/include/asm-arm/numa.h
>> @@ -11,6 +11,7 @@ typedef u8 nodeid_t;
>> int arch_numa_setup(char *opt);
>> int __init numa_init(void);
>> int __init dt_numa_init(void);
>> +u8 __node_distance(nodeid_t a, nodeid_t b);
>
>
> This should be defined in common/numa.h as it is used by common code.
>
>> #else
>> static inline int arch_numa_setup(char *opt)
>> {
>>
>
> Regards,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |