Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> 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.c...@arm.com>
> > Signed-off-by: Henry Wang <henry.w...@arm.com>
>
> non-Arm parts; to more it's not applicable anyway:
> Acked-by: Jan Beulich <jbeul...@suse.com>
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
