Re: [PATCH v2] of, numa: Validate some distance map rules

From: John Garry
Date: Thu Nov 08 2018 - 10:01:43 EST

On 08/11/2018 13:41, Anshuman Khandual wrote:
On 11/08/2018 03:47 PM, John Garry wrote:
Currently the NUMA distance map parsing does not validate the distance
table for the distance-matrix rules 1-2 in [1].

Right.


However the arch NUMA code may enforce some of these rules, but not all.

Either it does enforce all these rules again or it does not. What is the
purpose it serves when enforcing parts of it ! Rather it should focus on
looking for boundary conditions (e.g maximum number of nodes supported
on a platform against whats present on the table) which are important
from kernel point of view.

Such is the case for the arm64 port, which does not enforce the rule that
the distance between separates nodes cannot equal LOCAL_DISTANCE.

The patch adds the following rules validation:
- distance of node to self equals LOCAL_DISTANCE
- distance of separate nodes > LOCAL_DISTANCE

Which exactly corresponds to first two rules as mentioned in the DT doc
(Documents/devicetree/bindings/numa.txt)

1. Each entry represents distance from first node to second node.
The distances are equal in either direction.
2. The distance from a node to self (local distance) is represented
with value 10 and all internode distance should be represented with
a value greater than 10.



Not exactly. As you know, it does not add validation of distance equality in either direction (for values present in the table).

This change avoids a yet-unresolved crash reported in [2].

A note on dealing with symmetrical distances between nodes:

Validating symmetrical distances between nodes is difficult. If it were
mandated in the bindings that every distance must be recorded in the
table, then it would be easy. However, it isn't.

But if [a, b] and [b, a] both are present, they must be equal.


In addition to this, it is also possible to record [b, a] distance only
(and not [a, b]). So, when processing the table for [b, a], we cannot
assert that current distance of [a, b] != [b, a] as invalid, as [a, b]
distance may not be present in the table and current distance would be
default at REMOTE_DISTANCE.

Falling back to REMOTE_DISTANCE is right in case one of the symmetrical
distances is not present. But it must abort if they are present and not
identical. That rule should be enforced here while processing DT.

Do you really think it's worth the effort?

For this, we would need something considerably more complicated than what we have today. Maybe similar to the arm64 NUMA code does, in allocating a memblock and then doing some 2-phase table parsing. But then we're just replicated this same functionality.

On this point, I will also note that there is another problem with the DT NUMA bindings (first problem being that both distance entries are not required to be present in the table).

That is, it's inconsistency with ACPI SLIT. ACPI SLIT allows asymmetrical distance. So that's why I'm not too hung up on this issue of validating equality of distances.

This patch does not change the current kernel policy: if the table has entries with unequal distances, then it's not rejected nor fixed up.



As such, we maintain the policy that we overwrite distance [a, b] = [b, a]
for b > a. This policy is different to kernel ACPI SLIT validation, which
allows non-symmetrical distances (ACPI spec SLIT rules allow it). However,
the distance debug message is dropped as it may be misleading (for a distance
which is later overwritten).

We could override but if the DT passed table is inconsistent then it must
abort as non-symmetrical distance is not supported. Hence overriding the
distance should happen only in case the inverse set is absent (probably
outside the first loop).


Some final notes on semantics:

- It is implied that it is the responsibility of the arch NUMA code to
reset the NUMA distance map for an error in distance map parsing.

Right.


- It is the responsibility of the FW NUMA topology parsing (whether OF or
ACPI) to enforce NUMA distance rules, and not arch NUMA code.

This is where we still have some inconsistencies. Arch NUMA code on arm64
should either enforce rule 1 and 2 as stated above or should not at all.

It was implied that any validation can be later dropped from the arch NUMA code.

As
discussed previously numa_set_distance() enforces first part of rule 2
(self distance should be 10) but not the second part of rule 2 (internode
distance > 10). So either the following condition should be dropped from

(from == to && distance != LOCAL_DISTANCE)

or the following condition should be added to

(from != to && distance <= LOCAL_DISTANCE)

numa_set_distance() in order to conform to the semantics described above.
But thats for a separate discussion how much and what should be enforced
in any given DT compatible arch NUMA node. May not be part of this patch.


[1] Documents/devicetree/bindings/numa.txt
[2] https://www.spinics.net/lists/arm-kernel/msg683304.html

Cc: stable@xxxxxxxxxxxxxxx # 4.7
Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
Acked-by: Will Deacon <will.deacon@xxxxxxx>
---
Changes from v1:
- Add note on masking crash reported
- Add Will's tag
- Add cc stable

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 35c64a4295e0..fe6b13608e51 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c
@@ -104,9 +104,14 @@ static int __init of_numa_parse_distance_map_v1(struct device_node *map)
distance = of_read_number(matrix, 1);
matrix++;

+ if ((nodea == nodeb && distance != LOCAL_DISTANCE) ||
+ (nodea != nodeb && distance <= LOCAL_DISTANCE)) {
+ pr_err("Invalid distance[node%d -> node%d] = %d\n",
+ nodea, nodeb, distance);
+ return -EINVAL;
+ }
+
numa_set_distance(nodea, nodeb, distance);
- pr_debug("distance[node%d -> node%d] = %d\n",
- nodea, nodeb, distance);

It makes sense to go through the table and print the finalized values
(being passed to arch NUMA) out side of this loop after any identical
replacement is done.

/* Set default distance of node B->A same as A->B */
if (nodeb > nodea)


As mentioned before it should invalidate the table [a, b] != [b, a] in
case they are present and not equal.


I think that most of these comments are related to earlier points.

.


Thanks,
John