Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> 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
>
