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