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

2017-03-02 Thread Julien Grall 

Hello Vijay,

On 02/03/17 12:10, Vijay Kilari wrote:

On Wed, Feb 22, 2017 at 5:14 PM, Julien Grall  wrote:

On 22/02/17 11:38, Vijay Kilari wrote:


[...]


That is default distance value.



From where? Please give a link to the doc.


10/20 is used by x86 implementation as well.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/topology.h?id=refs/tags/v4.10#n47



Then please introduce LOCAL_DISTANCE and REMOTE_DISTANCE to avoid 
hardcoded value.




Also the default matrix is shown below
Documentation/devicetree/bindings/numa.txt


Looking at the binding, the show an example not the default value.

Regards,

--
Julien Grall

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

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

2017-03-02 Thread Vijay Kilari 
On Wed, Feb 22, 2017 at 5:14 PM, Julien Grall  wrote:
> Hello Vijay,
>
>
> On 22/02/17 11:38, Vijay Kilari wrote:
>>
>> On Mon, Feb 20, 2017 at 11:58 PM, Julien Grall 
>> wrote:
>>>
>>> Hello Vijay,
>>>
>>> On 09/02/17 15:57, vijay.kil...@gmail.com wrote:


 From: Vijaya Kumar K 

 Parse distance-matrix and fetch node distance information.
 Store distance information in node_distance[].

 Signed-off-by: Vijaya Kumar K 
 ---
  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
>
>
> The numa distance function returns an u8 and the common code rely on u8. So
> IHMO it is fine to restrict to u8.
>
> If you want to keep u8 then please fix the rest of the code.
>
> [...]
>
 +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[].
>
>
> If you look at your implementation of numa_set_distance it returns an error
> if the nodea, nodeb are too big. So you should really check the
> return an report an error. Because the DT is buggy.
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 
  #include 
  #include 
 +#include 
>>>
>>>
>>>
>>> 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.
>
>
> From where? Please give a link to the doc.

10/20 is used by x86 implementation as well.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/topology.h?id=refs/tags/v4.10#n47

Also the default matrix is shown below
Documentation/devicetree/bindings/numa.txt

>
> Regards,
>
> --
> Julien Grall

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

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

2017-02-22 Thread Julien Grall 

Hello Vijay,

On 22/02/17 11:38, Vijay Kilari wrote:

On Mon, Feb 20, 2017 at 11:58 PM, Julien Grall  wrote:

Hello Vijay,

On 09/02/17 15:57, vijay.kil...@gmail.com wrote:


From: Vijaya Kumar K 

Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].

Signed-off-by: Vijaya Kumar K 
---
 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


The numa distance function returns an u8 and the common code rely on u8. 
So IHMO it is fine to restrict to u8.


If you want to keep u8 then please fix the rest of the code.

[...]


+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[].


If you look at your implementation of numa_set_distance it returns an 
error if the nodea, nodeb are too big. So you should really check the

return an report an error. Because the DT is buggy.

[...]





+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 
 #include 
 #include 
+#include 



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.


From where? Please give a link to the doc.

Regards,

--
Julien Grall

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

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

2017-02-22 Thread Vijay Kilari 
On Mon, Feb 20, 2017 at 11:58 PM, Julien Grall  wrote:
> Hello Vijay,
>
> On 09/02/17 15:57, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K 
>>
>> Parse distance-matrix and fetch node distance information.
>> Store distance information in node_distance[].
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  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", );
>> +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,

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

2017-02-20 Thread Julien Grall 

Hello Vijay,

On 09/02/17 15:57, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].

Signed-off-by: Vijaya Kumar K 
---
 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.

Lastly, I am not sure why you pre-allocate the memory. The distance 
table could be quite big.



+
+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", );
+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?


+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.



+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

[Xen-devel] [RFC PATCH v1 08/21] ARM: NUMA: Parse NUMA distance information

2017-02-09 Thread vijay . kilari 
From: Vijaya Kumar K 

Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].

Signed-off-by: Vijaya Kumar K 
---
 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;
+
+static int numa_set_distance(u32 nodea, u32 nodeb, u32 distance)
+{
+   if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES )
+   return -EINVAL;
+
+   _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", );
+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);
+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") )
+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 
 #include 
 #include 
+#include 
+
+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;
+
+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;
+
 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
---