RE: [PATCH v3 03/17] xen/arm: implement node distance helpers for Arm

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [PATCH v3 03/17] xen/arm: implement node distance helpers for
> Arm
> > As both x86 and Arm have implemented __node_distance, so we move
> > its definition from asm/numa.h to xen/numa.h.
> 
> Nit: You mean "declaration", not "definition".

Correct, I overlooked this miswording in commit message while going
through your comments in v2. will correct in v4.

> 
> > At same time, the
> > outdated u8 return value of x86 has been changed to unsigned char.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> 
> non-Arm parts; to more it's not applicable anyway:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

I will add # non-Arm parts in the end of the tag and take the tag.

> 
> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -28,6 +28,11 @@ enum dt_numa_status {
> >
> >  static enum dt_numa_status __ro_after_init device_tree_numa =
> DT_NUMA_DEFAULT;
> >
> > +static unsigned char __ro_after_init
> > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > +    { 0 }
> > +};
> 
> Nit: There's no (incomplete or complete) initializer needed here, if
> all you're after is having all slots set to zero.

Yeah, I agree with you, so I will drop the initializer in v4, however...

> 
> However, looking at the code below, don't you mean to have the array
> pre-set to all NUMA_NO_DISTANCE?

...I am a bit puzzled about why pre-setting the array to all
NUMA_NO_DISTANCE matters here, as I think the node distance map will
be populated when parsing the device tree anyway no matter what their
initial values.

> 
> > +    /* NUMA defines 0xff as an unreachable node and 0-9 are undefined */
> > +    if ( distance >= NUMA_NO_DISTANCE ||
> > +         (distance >= NUMA_DISTANCE_UDF_MIN &&
> 
> Compilers may warn about comparison of "unsigned int" to be >= 0. I'm
> not sure about the usefulness of the NUMA_DISTANCE_UDF_MIN define in
> the first place, so maybe best drop it and its use here?

Yeah, will do that in v4.

> 
> > +unsigned char __node_distance(nodeid_t from, nodeid_t to)
> > +{
> > +    /* When NUMA is off, any distance will be treated as remote. */
> > +    if ( numa_disabled() )
> > +        return NUMA_REMOTE_DISTANCE;
> 
> Wouldn't it make sense to have the "from == to" special case ahead of
> this (rather than further down), thus yielding a sensible result for
> from == to == 0? And else return NUMA_NO_DISTANCE, thus having a
> sensible result also for any from/to != 0?

Could you please elaborate a bit more about why 0 matters here? As from
my understanding,
(1) If from == to, then we set the distance to NUMA_LOCAL_DISTANCE
which represents the diagonal of the matrix.
(2) If from and to is in the matrix range, then we return
node_distance_map[from][to].
(3) Other cases we return NUMA_NO_DISTANCE.

So this diff is enough:
diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
@@ -98,6 +98,9 @@ void numa_detect_cpu_node(unsigned int cpu)

 unsigned char __node_distance(nodeid_t from, nodeid_t to)
 {
+    if ( from == to )
+        return NUMA_LOCAL_DISTANCE;
+
     /* When NUMA is off, any distance will be treated as remote. */
     if ( numa_disabled() )
         return NUMA_REMOTE_DISTANCE;
@@ -109,7 +112,7 @@ unsigned char __node_distance(nodeid_t from, nodeid_t to)
      */
     if ( from >= ARRAY_SIZE(node_distance_map) ||
          to >= ARRAY_SIZE(node_distance_map[0]) )
-        return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE;
+        return NUMA_NO_DISTANCE;

     return node_distance_map[from][to];
 }

Would you mind pointing out why my understanding is wrong? Thanks!

> 
> > +    /*
> > +     * Check whether the nodes are in the matrix range.
> > +     * When any node is out of range, except from and to nodes are the
> > +     * same, we treat them as unreachable (return 0xFF)
> > +     */
> > +    if ( from >= ARRAY_SIZE(node_distance_map) ||
> > +         to >= ARRAY_SIZE(node_distance_map[0]) )
> > +        return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE;
> > +
> > +    return node_distance_map[from][to];
> > +}
> > +
> > +EXPORT_SYMBOL(__node_distance);
> 
> What is this needed for?

Will drop it.

Kind regards,
Henry

> 
> Jan