Re: Crash report: Broken NUMA distance map causes crash on arm64 system

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 10:01:05AM +0000, John Garry wrote:
> On 31/10/2018 20:46, Peter Zijlstra wrote:

> > > I also note that if I apply the patch, below, to reject the invalid NUMA
> > > distance, we're still getting a warning/error:
> > > 
> > > [    7.144407] CPU: All CPU(s) started at EL2
> > > [    7.148678] alternatives: patching kernel code
> > > [    7.153557] ERROR: Node-0 not representative
> > > [    7.153557]
> > > [    7.159365]   10 15 20 25
> > > [    7.162097]   15 10 25 30
> > > [    7.164832]   20 25 10 15
> > > [    7.167562]   25 30 15 10
> > 
> > Yeah, that's an 'obviously' broken topology too.
> > 
> 
> AFAICT, this conforms to ACPI spec SLIT rules, and the kernel SLIT
> validation allows this also. So maybe we should shout louder here or even
> mark the SLIT as invalid if totally broken.

Right. Part of the problem is that I only have a few necessary
conditions (symmetry, trace-identity, node0-complete-distance) for the
distance table, but together they are not sufficient to determine if a
topology is 'good'.

(also, strictly speaking, non-symmetric topologies are possible -- think
uni-directional mesh links -- we've just not ever seen and validated
those)

That said; I can certainly make this code give up and warn harder on
those conditions.

How is something like the below?

---

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 9d74371e4aad..41e703dd875f 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1207,6 +1218,11 @@ sd_init(struct sched_domain_topology_level *tl,
 	return sd;
 }
 
+static const struct cpumask *cpu_system_mask(int cpu)
+{
+	return cpu_online_mask;
+}
+
 /*
  * Topology list, bottom-up.
  */
@@ -1337,7 +1353,7 @@ void sched_init_numa(void)
 	int level = 0;
 	int i, j, k;
 
-	sched_domains_numa_distance = kzalloc(sizeof(int) * nr_node_ids, GFP_KERNEL);
+	sched_domains_numa_distance = kzalloc(sizeof(int) * (nr_node_ids + 1), GFP_KERNEL);
 	if (!sched_domains_numa_distance)
 		return;
 
@@ -1353,39 +1369,22 @@ void sched_init_numa(void)
 	 * node_distance(i,j) in order to avoid cubic time.
 	 */
 	next_distance = curr_distance;
+
 	for (i = 0; i < nr_node_ids; i++) {
 		for (j = 0; j < nr_node_ids; j++) {
-			for (k = 0; k < nr_node_ids; k++) {
-				int distance = node_distance(i, k);
-
-				if (distance > curr_distance &&
-				    (distance < next_distance ||
-				     next_distance == curr_distance))
-					next_distance = distance;
-
-				/*
-				 * While not a strong assumption it would be nice to know
-				 * about cases where if node A is connected to B, B is not
-				 * equally connected to A.
-				 */
-				if (sched_debug() && node_distance(k, i) != distance)
-					sched_numa_warn("Node-distance not symmetric");
-
-				if (sched_debug() && i && !find_numa_distance(distance))
-					sched_numa_warn("Node-0 not representative");
-			}
-			if (next_distance != curr_distance) {
-				sched_domains_numa_distance[level++] = next_distance;
-				sched_domains_numa_levels = level;
-				curr_distance = next_distance;
-			} else break;
-		}
+			int distance = node_distance(0, j);
 
-		/*
-		 * In case of sched_debug() we verify the above assumption.
-		 */
-		if (!sched_debug())
-			break;
+			if (distance > curr_distance &&
+			    (distance < next_distance ||
+			     next_distance == curr_distance))
+				next_distance = distance;
+
+		}
+		if (next_distance != curr_distance) {
+			sched_domains_numa_distance[level++] = next_distance;
+			sched_domains_numa_levels = level;
+			curr_distance = next_distance;
+		} else break;
 	}
 
 	/*
@@ -1428,7 +1427,24 @@ void sched_init_numa(void)
 			sched_domains_numa_masks[i][j] = mask;
 
 			for_each_node(k) {
-				if (node_distance(j, k) > sched_domains_numa_distance[i])
+				int distance = node_distance(j, k);
+
+				if (!i && j != k) {
+					sched_numa_warn("Non-trace locality\n");
+					goto fail;
+				}
+
+				if (distance > curr_distance) {
+					sched_numa_warn("Node-0 not complete\n");
+					goto fail;
+				}
+
+				if (distance != node_distance(k, j)) {
+					sched_numa_warn("Non symmetric\n");
+					goto fail;
+				}
+
+				if (distance > sched_domains_numa_distance[i])
 					continue;
 
 				cpumask_or(mask, mask, cpumask_of_node(k));
@@ -1437,9 +1453,10 @@ void sched_init_numa(void)
 	}
 
 	/* Compute default topology size */
-	for (i = 0; sched_domain_topology[i].mask; i++);
+	for (k = 0; sched_domain_topology[k].mask; k++)
+		;
 
-	tl = kzalloc((i + level + 1) *
+	tl = kzalloc((k + level + 1) *
 			sizeof(struct sched_domain_topology_level), GFP_KERNEL);
 	if (!tl)
 		return;
@@ -1447,7 +1464,7 @@ void sched_init_numa(void)
 	/*
 	 * Copy the default topology bits..
 	 */
-	for (i = 0; sched_domain_topology[i].mask; i++)
+	for (i = 0; i < k; i++)
 		tl[i] = sched_domain_topology[i];
 
 	/*
@@ -1478,6 +1495,31 @@ void sched_init_numa(void)
 	sched_max_numa_distance = sched_domains_numa_distance[level - 1];
 
 	init_numa_topology_type();
+
+	return;
+
+fail:
+	/* Compute default topology size */
+	for (k = 0; sched_domain_topology[k].mask; k++)
+		;
+
+	tl = kzalloc((k + 2) *
+			sizeof(struct sched_domain_topology_level), GFP_KERNEL);
+	if (!tl)
+		return;
+
+	/*
+	 * Copy the default topology bits..
+	 */
+	for (i = 0; i < k; i++)
+		tl[i] = sched_domain_topology[i];
+
+	tl[i] = (struct sched_domain_topology_level){
+		.mask = cpu_system_mask,
+		SD_INIT_NAME(SYSTEM)
+	};
+
+	sched_domain_topology = tl;
 }
 
 void sched_domains_numa_masks_set(unsigned int cpu)