Skip to content

Commit

Permalink
power, sched: stop updating inside arch_update_cpu_topology() when no…
Browse files Browse the repository at this point in the history
…thing to be update

Since v1:
	Edited the comment according to Srivatsa's suggestion.

During the testing, we encounter below WARN followed by Oops:

	WARNING: at kernel/sched/core.c:6218
	...
	NIP [c000000000101660] .build_sched_domains+0x11d0/0x1200
	LR [c000000000101358] .build_sched_domains+0xec8/0x1200
	PACATMSCRATCH [800000000000f032]
	Call Trace:
	[c00000001b103850] [c000000000101358] .build_sched_domains+0xec8/0x1200
	[c00000001b1039a0] [c00000000010aad4] .partition_sched_domains+0x484/0x510
	[c00000001b103aa0] [c00000000016d0a8] .rebuild_sched_domains+0x68/0xa0
	[c00000001b103b30] [c00000000005cbf0] .topology_work_fn+0x10/0x30
	...
	Oops: Kernel access of bad area, sig: 11 [alexander-zimmermann#1]
	...
	NIP [c00000000045c000] .__bitmap_weight+0x60/0xf0
	LR [c00000000010132c] .build_sched_domains+0xe9c/0x1200
	PACATMSCRATCH [8000000000029032]
	Call Trace:
	[c00000001b1037a0] [c000000000288ff4] .kmem_cache_alloc_node_trace+0x184/0x3a0
	[c00000001b103850] [c00000000010132c] .build_sched_domains+0xe9c/0x1200
	[c00000001b1039a0] [c00000000010aad4] .partition_sched_domains+0x484/0x510
	[c00000001b103aa0] [c00000000016d0a8] .rebuild_sched_domains+0x68/0xa0
	[c00000001b103b30] [c00000000005cbf0] .topology_work_fn+0x10/0x30
	...

This was caused by that 'sd->groups == NULL' after building groups, which
was caused by the empty 'sd->span'.

The cpu's domain contained nothing because the cpu was assigned to a wrong
node, due to the following unfortunate sequence of events:

1. The hypervisor sent a topology update to the guest OS, to notify changes
   to the cpu-node mapping. However, the update was actually redundant - i.e.,
   the "new" mapping was exactly the same as the old one.

2. Due to this, the 'updated_cpus' mask turned out to be empty after exiting
   the 'for-loop' in arch_update_cpu_topology().

3. So we ended up calling stop-machine() with an empty cpumask list, which made
   stop-machine internally elect cpumask_first(cpu_online_mask), i.e., CPU0 as
   the cpu to run the payload (the update_cpu_topology() function).

4. This causes update_cpu_topology() to be run by CPU0. And since 'updates'
   is kzalloc()'ed inside arch_update_cpu_topology(), update_cpu_topology()
   finds update->cpu as well as update->new_nid to be 0. In other words, we
   end up assigning CPU0 (and eventually its siblings) to node 0, incorrectly.

Along with the following wrong updating, it causes the sched-domain rebuild
code to break and crash the system.

Fix this by skipping the topology update in cases where we find that
the topology has not actually changed in reality (ie., spurious updates).

CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: Nathan Fontenot <[email protected]>
CC: Stephen Rothwell <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Robert Jennings <[email protected]>
CC: Jesse Larrew <[email protected]>
CC: "Srivatsa S. Bhat" <[email protected]>
CC: Alistair Popple <[email protected]>
Suggested-by: "Srivatsa S. Bhat" <[email protected]>
Signed-off-by: Michael Wang <[email protected]>
Reviewed-by: Srivatsa S. Bhat <[email protected]>
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
  • Loading branch information
Michael Wang authored and ozbenh committed Apr 9, 2014
1 parent d3d35d9 commit 9a01336
Showing 1 changed file with 15 additions and 0 deletions.
15 changes: 15 additions & 0 deletions arch/powerpc/mm/numa.c
Original file line number Diff line number Diff line change
Expand Up @@ -1591,6 +1591,20 @@ int arch_update_cpu_topology(void)
cpu = cpu_last_thread_sibling(cpu);
}

/*
* In cases where we have nothing to update (because the updates list
* is too short or because the new topology is same as the old one),
* skip invoking update_cpu_topology() via stop-machine(). This is
* necessary (and not just a fast-path optimization) since stop-machine
* can end up electing a random CPU to run update_cpu_topology(), and
* thus trick us into setting up incorrect cpu-node mappings (since
* 'updates' is kzalloc()'ed).
*
* And for the similar reason, we will skip all the following updating.
*/
if (!cpumask_weight(&updated_cpus))
goto out;

stop_machine(update_cpu_topology, &updates[0], &updated_cpus);

/*
Expand All @@ -1612,6 +1626,7 @@ int arch_update_cpu_topology(void)
changed = 1;
}

out:
kfree(updates);
return changed;
}
Expand Down

0 comments on commit 9a01336

Please sign in to comment.