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
> >> 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.
> 
> From this patch alone it doesn't become clear whether indeed all array
> slots (and not just ones for valid nodes) would be populated. I think
> the code in the patch here would better not make itself dependent on
> behavior of code added subsequently (which may change; recall that a
> series may be committed in pieces).

Correct, I agree. I added a numa_init_distance() function (in patch #12) to
set all values to NUMA_NO_DISTANCE. The numa_init_distance() will be
called in the beginning of numa_init().

> 
> >>> +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?
> 
> When NUMA is off, there's only one node - node 0. Hence 0 has special
> meaning in that case.
> 
> > 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].
> 
> Provided that's set correctly. IOW this interacts with the other comment
> (which really I made only after the one here, just that that's of course
> not visible from the reply that I sent).
> 
> > (3) Other cases we return NUMA_NO_DISTANCE.
> 
> And when NUMA is off, it should be NUMA_NO_DISTANCE in _all_ other
> cases,
> i.e. ...
> 
> >      /* When NUMA is off, any distance will be treated as remote. */
> >      if ( numa_disabled() )
> >          return NUMA_REMOTE_DISTANCE;
> 
> ... this return is wrong in that case (even if in reality this likely
> wouldn't matter much).

Thanks for the explanation! I think I now understand :) Would this diff below
look good to you then? Appreciate your patience.

unsigned char __node_distance(nodeid_t from, nodeid_t to)
 {
-    /* When NUMA is off, any distance will be treated as remote. */
+    if ( from == to )
+        return NUMA_LOCAL_DISTANCE;
+
+    /* When NUMA is off, any distance will be treated as unreachable (0xFF). */
     if ( numa_disabled() )
-        return NUMA_REMOTE_DISTANCE;
+        return NUMA_NO_DISTANCE;

     /*
      * 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)
+     * same, we treat them as unreachable (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 NUMA_NO_DISTANCE;

     return node_distance_map[from][to];
 }

Kind regards,
Henry

> 
> Jan
>