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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2023 11:23, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>
>>> --- 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.

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

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

> 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;

... this return is wrong in that case (even if in reality this likely
wouldn't matter much).

Jan

> @@ -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!
> 
> Kind regards,
> Henry