Re: hotplug lockdep splat (tip)

[Peter Zijlstra]

On Mon, Sep 04, 2017 at 03:27:07PM +0200, Mike Galbraith wrote:
> Well, flavor of gripe changed.

> 
> [  164.114290] ======================================================
> [  164.115146] WARNING: possible circular locking dependency detected
> [  164.115751] 4.13.0.g90abd70-tip-lockdep #4 Tainted: G            E  
> [  164.116348] ------------------------------------------------------
> [  164.116919] cpuhp/0/12 is trying to acquire lock:
> [  164.117381]  (cpuhp_state){+.+.}, at: [<ffffffff8108e4aa>] cpuhp_thread_fun+0x2a/0x160
> [  164.118097] 
>                but now in release context of a crosslock acquired at the following:
> [  164.118845]  ((complete)&per_cpu_ptr(&cpuhp_state, i)->done#2){+.+.}, at: [<ffffffff8108e71f>] cpuhp_issue_call+0x13f/0x170
> [  164.119789] 
>                which lock already depends on the new lock.
> 
> [  164.120483] 
>                the existing dependency chain (in reverse order) is:
> [  164.121121] 
>                -> #2 ((complete)&per_cpu_ptr(&cpuhp_state, i)->done#2){+.+.}:

> [  164.122741]        wait_for_completion+0x53/0x1a0
> [  164.123181]        takedown_cpu+0x8a/0xf0

So this is:

	takedown_cpu()
	  irq_lock_sparse
	  wait_for_completion(st->done);


>                -> #1 (sparse_irq_lock){+.+.}:

> [  164.131664]        irq_lock_sparse+0x17/0x20
> [  164.132039]        irq_affinity_online_cpu+0x18/0xd0
> [  164.132463]        cpuhp_invoke_callback+0x1f6/0x830
> [  164.132928]        cpuhp_up_callbacks+0x37/0xb0
> [  164.133487]        cpuhp_thread_fun+0x14f/0x160

This is:

		cpuhp_state
		  irq_lock_sparse


> [  164.148780]  complete+0x29/0x60
> [  164.149064]  cpuhp_thread_fun+0xa1/0x160

And this is I think:

  cpuhp_thread_fun()
    cpuhq_state
    complete(st->done)


Which then spells deadlock like:

  CPU0			CPU1			CPU2

  cpuhp_state
			irq_lock_sparse()
						cpuhp_state
			wait_for_completion()
    irq_lock_sparse()
						complete()

or something, because CPU0 needs to wait for CPU1's irq_lock_sparse
which will wait for CPU2's completion, which in turn waits for CPU0's
cpuhp_state.


Now, this again mixes up and down chains, but now on cpuhp_state.


But I cannot reproduce this.. I've let:

  while :; do
    echo 0 > /sys/devices/system/cpu/cpu1/online ;
    echo 1 > /sys/devices/system/cpu/cpu1/online ;
  done

run for a while, but nothing... :/

Doth teh beloweth make nice?


---
 kernel/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index acf5308fad51..2ab324d7ff7b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -67,11 +67,15 @@ struct cpuhp_cpu_state {
 static DEFINE_PER_CPU(struct cpuhp_cpu_state, cpuhp_state);
 
 #if defined(CONFIG_LOCKDEP) && defined(CONFIG_SMP)
-static struct lock_class_key cpuhp_state_key;
+static struct lock_class_key cpuhp_state_up_key;
+#ifdef CONFIG_HOTPLUG_CPU
+static struct lock_class_key cpuhp_state_down_key;
+#endif
 static struct lockdep_map cpuhp_state_lock_map =
-	STATIC_LOCKDEP_MAP_INIT("cpuhp_state", &cpuhp_state_key);
+	STATIC_LOCKDEP_MAP_INIT("cpuhp_state-up", &cpuhp_state_up_key);
 #endif
 
+
 /**
  * cpuhp_step - Hotplug state machine step
  * @name:	Name of the step
@@ -533,6 +537,28 @@ void __init cpuhp_threads_init(void)
 	kthread_unpark(this_cpu_read(cpuhp_state.thread));
 }
 
+/*
+ * _cpu_down() and _cpu_up() have different lock ordering wrt st->done, but
+ * because these two functions are globally serialized and st->done is private
+ * to them, we can simply re-init st->done for each of them to separate the
+ * lock chains.
+ *
+ * Must be macro to ensure we have two different call sites.
+ */
+#ifdef CONFIG_LOCKDEP
+#define lockdep_reinit_st_done()				\
+do {								\
+	int __cpu;						\
+	for_each_possible_cpu(__cpu) {				\
+		struct cpuhp_cpu_state *st =			\
+			per_cpu_ptr(&cpuhp_state, __cpu);	\
+		init_completion(&st->done);			\
+	}							\
+} while(0)
+#else
+#define lockdep_reinit_st_done()
+#endif
+
 #ifdef CONFIG_HOTPLUG_CPU
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
@@ -676,12 +702,6 @@ void cpuhp_report_idle_dead(void)
 				 cpuhp_complete_idle_dead, st, 0);
 }
 
-#else
-#define takedown_cpu		NULL
-#endif
-
-#ifdef CONFIG_HOTPLUG_CPU
-
 /* Requires cpu_add_remove_lock to be held */
 static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 			   enum cpuhp_state target)
@@ -697,6 +717,10 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 
 	cpus_write_lock();
 
+	lockdep_reinit_st_done();
+	lockdep_init_map(&cpuhp_state_lock_map, "cpuhp_state-down",
+			 &cpuhp_state_down_key, 0);
+
 	cpuhp_tasks_frozen = tasks_frozen;
 
 	prev_state = st->state;
@@ -759,6 +783,9 @@ int cpu_down(unsigned int cpu)
 	return do_cpu_down(cpu, CPUHP_OFFLINE);
 }
 EXPORT_SYMBOL(cpu_down);
+
+#else
+#define takedown_cpu		NULL
 #endif /*CONFIG_HOTPLUG_CPU*/
 
 /**
@@ -806,6 +833,10 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
 
 	cpus_write_lock();
 
+	lockdep_reinit_st_done();
+	lockdep_init_map(&cpuhp_state_lock_map, "cpuhp_state-up",
+			 &cpuhp_state_up_key, 0);
+
 	if (!cpu_present(cpu)) {
 		ret = -EINVAL;
 		goto out;