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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2023 07:36, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> 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().

Why do you need a function for this? As said, this array can be pre-set at
compile time (unless I'm overlooking something).

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

Looks largely okay, but possibly one part can now be omitted (see below).

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

Please avoid mentioning the actual value of 0xFF: This serves no real
purpose (afaict) and is liable to go stale at some point.

>      if ( numa_disabled() )
> -        return NUMA_REMOTE_DISTANCE;
> +        return NUMA_NO_DISTANCE;

With the code below this is now only an optimization. Might be worth
saying so in the comment (assuming having this optimization is deemed
worth it).

Jan

>      /*
>       * 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
>>