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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> Subject: Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device
> tree NUMA distance map
> 
> >>>>> +        /* The default value in node_distance_map is
> >> NUMA_NO_DISTANCE
> >>>> */
> >>>>> +        if ( opposite == NUMA_NO_DISTANCE )
> >>>>
> >>>> And the matrix you're reading from can't hold NUMA_NO_DISTANCE
> >> entries?
> >>>> I ask because you don't check this above; you only check against
> >>>> NUMA_LOCAL_DISTANCE.
> >>>
> >>> My understanding for the purpose of this part of code is to check if the
> >> opposite
> >>> way distance has already been set, so we need to compare the opposite
> >> way
> >>> distance with the default value NUMA_NO_DISTANCE here.
> >>>
> >>> Back to your question, I can see your point of the question. However I
> don't
> >> think
> >>> NUMA_NO_DISTANCE is a valid value to describe the node distance in the
> >> device
> >>> tree. This is because I hunted down the previous discussions and found
> [2]
> >> about
> >>> we should try to keep consistent between the value used in device tree
> and
> >> ACPI
> >>> tables. From the ACPI spec, 0xFF, i.e. NUMA_NO_DISTANCE means
> >> unreachable.
> >>> I think this is also the reason why NUMA_NO_DISTANCE can be used as
> the
> >> default
> >>> value of the distance map, otherwise we won't have any value to use.
> >>
> >> 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!

Kind regards,
Henry

> 
> Jan