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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Feb 22, 2017 at 5:14 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
>
> On 22/02/17 11:38, Vijay Kilari wrote:
>>
>> 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
>
>
> 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 <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.
>
>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel