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

[Julien Grall <julien.grall@xxxxxxx>]

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.

[...]

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

Regards,

--
Julien Grall

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