Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device tree NUMA distance map

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.05.2023 09:32, Henry Wang wrote:
>> -----Original Message-----
>> Subject: Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device
>> tree NUMA distance map
>>
>>>>>> The [2] link you provided discusses NUMA_LOCAL_DISTANCE.
>>>>>
>>>>> I inferred the discussion as "we should try to keep consistent between the
>>>> value
>>>>> used in device tree and ACPI tables". Maybe my inference is wrong.
>>>>>
>>>>>> Looking at
>>>>>> Linux'es Documentation/devicetree/numa.txt, there's no mention of an
>>>>>> upper bound on the distance values. It only says that on the diagonal
>>>>>> entries should be 10 (i.e. matching ACPI, without really saying so).
>>>>>
>>>>> I agree that the NUMA device tree binding is a little bit vague. So I 
>>>>> cannot
>>>>> say the case that you provided is not valid. I would like to ask Arm
>>>> maintainers
>>>>> (putting them into To:) opinion on this as I think I am not the one to
>> decide
>>>> the
>>>>> expected behavior on Arm.
>>>>>
>>>>> Bertrand/Julien/Stefano: Would you please kindly share your opinion on
>>>> which
>>>>> value should be used as the default value of the node distance map? Do
>>>> you
>>>>> think reusing the "unreachable" distance, i.e. 0xFF, as the default node
>>>> distance
>>>>> is acceptable here? Thanks!
>>>>
>>>> My suggestion would be to, rather than rejecting values >= 0xff, to 
>>>> saturate
>>>> at 0xfe, while keeping 0xff for NUMA_NO_DISTANCE (and overall keeping
>>>> things
>>>> consistent with ACPI).
>>>
>>> Since it has been a while and there were no feedback from Arm
>> maintainers,
>>> I would like to follow your suggestions for v5. However while I was writing
>> the
>>> code for the "saturation", i.e., adding below snippet in
>> numa_set_distance():
>>> ```
>>> if ( distance > NUMA_NO_DISTANCE )
>>>         distance = NUMA_MAX_DISTANCE;
>>> ```
>>> I noticed an issue which needs your clarification:
>>> Currently, the default value of the distance map is NUMA_NO_DISTANCE,
>>> which indicates the nodes are not reachable. IMHO, if in device tree, we do
>>> saturations like above for ACPI invalid distances (distances >= 0xff), by
>> saturating
>>> the distance to 0xfe, we are making the unreachable nodes to reachable. I
>> am
>>> not sure if this will lead to problems. Do you have any more thoughts?
>> Thanks!
>>
>> I can only answer this with a question back: How is "unreachable"
>> represented
>> in DT? 
> 
> Exactly, that is also what we I am trying to find but failed. My understanding
> is that, the spec of NUMA is defined in the ACPI, and the DT NUMA binding
> only specifies how users can use DT to represent the same set of ACPI data,
> instead of define another standard.
> 
> By looking into the existing implementation of NUMA for DT,
> in Linux, from drivers/base/arch_numa.c: numa_set_distance(), there is a
> "if ((u8)distance != distance)" then return. So I think at least in Linux the
> distance value is consistent with the ACPI spec. 

Can we simply gain a similar check (suitably commented), eliminating the
need for saturation?

Jan

>> Or is "unreachable" simply expressed by the absence of any data?
> 
> Maybe I am wrong but I don't think so, as in the Linux implementation,
> drivers/of/of_numa.c: of_numa_parse_distance_map_v1(), the for loop
> "for (i = 0; i + 2 < entry_count; i += 3)" basically implies no fields should
> be omitted in the distance map entry.
> 
> Kind regards,
> Henry