Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720136 > unrolled thread

Re: WARNING: possible circular locking dependency detected

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: WARNING: possible circular locking dependency detected Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-08-25 16:50 +0200
    Re: WARNING: possible circular locking dependency detected Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-25 18:20 +0200
      Re: WARNING: possible circular locking dependency detected Thomas Gleixner <tglx@linutronix.de> - 2017-08-25 18:30 +0200
    Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-28 09:50 +0200
    Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-28 16:20 +0200
    Re: WARNING: possible circular locking dependency detected Peter Zijlstra <peterz@infradead.org> - 2017-08-29 21:40 +0200

#1720136 — Re: WARNING: possible circular locking dependency detected

On 2017-08-25 12:03:04 [+0200], Borislav Petkov wrote:
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.13.0-rc6+ #1 Not tainted
> ------------------------------------------------------

While looking at this, I stumbled upon another one also enabled by
"completion annotation" in the TIP:

| ======================================================
| WARNING: possible circular locking dependency detected
| 4.13.0-rc6-00758-gd80d4177391f-dirty #112 Not tainted
| ------------------------------------------------------
| cpu-off.sh/426 is trying to acquire lock:
|  ((complete)&st->done){+.+.}, at: [<ffffffff810cb344>] takedown_cpu+0x84/0xf0
|
| but task is already holding lock:
|  (sparse_irq_lock){+.+.}, at: [<ffffffff811220f2>] irq_lock_sparse+0x12/0x20
|
| which lock already depends on the new lock.
|
| the existing dependency chain (in reverse order) is:
|
| -> #1 (sparse_irq_lock){+.+.}:
|        __mutex_lock+0x88/0x9a0
|        mutex_lock_nested+0x16/0x20
|        irq_lock_sparse+0x12/0x20
|        irq_affinity_online_cpu+0x13/0xd0
|        cpuhp_invoke_callback+0x4a/0x130
|
| -> #0 ((complete)&st->done){+.+.}:
|        check_prev_add+0x351/0x700
|        __lock_acquire+0x114a/0x1220
|        lock_acquire+0x47/0x70
|        wait_for_completion+0x5c/0x180
|        takedown_cpu+0x84/0xf0
|        cpuhp_invoke_callback+0x4a/0x130
|        cpuhp_down_callbacks+0x3d/0x80
…
|
| other info that might help us debug this:
|
|  Possible unsafe locking scenario:
|        CPU0                    CPU1
|        ----                    ----
|   lock(sparse_irq_lock);
|                                lock((complete)&st->done);
|                                lock(sparse_irq_lock);
|   lock((complete)&st->done);
|
|  *** DEADLOCK ***

We hold the sparse_irq_lock lock while waiting for the completion in the
CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
while the other CPU is waiting for the completion.
This is not an issue if my interpretation of lockdep here is correct.

How do we annotate this?

Sebastian

[toc] | [next] | [standalone]

#1720240

On Fri, Aug 25, 2017 at 11:47 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2017-08-25 12:03:04 [+0200], Borislav Petkov wrote:
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 4.13.0-rc6+ #1 Not tainted
>> ------------------------------------------------------
>
> While looking at this, I stumbled upon another one also enabled by
> "completion annotation" in the TIP:
>
> | ======================================================
> | WARNING: possible circular locking dependency detected
> | 4.13.0-rc6-00758-gd80d4177391f-dirty #112 Not tainted
> | ------------------------------------------------------
> | cpu-off.sh/426 is trying to acquire lock:
> |  ((complete)&st->done){+.+.}, at: [<ffffffff810cb344>] takedown_cpu+0x84/0xf0
> |
> | but task is already holding lock:
> |  (sparse_irq_lock){+.+.}, at: [<ffffffff811220f2>] irq_lock_sparse+0x12/0x20
> |
> | which lock already depends on the new lock.
> |
> | the existing dependency chain (in reverse order) is:
> |
> | -> #1 (sparse_irq_lock){+.+.}:
> |        __mutex_lock+0x88/0x9a0
> |        mutex_lock_nested+0x16/0x20
> |        irq_lock_sparse+0x12/0x20
> |        irq_affinity_online_cpu+0x13/0xd0
> |        cpuhp_invoke_callback+0x4a/0x130
> |
> | -> #0 ((complete)&st->done){+.+.}:
> |        check_prev_add+0x351/0x700
> |        __lock_acquire+0x114a/0x1220
> |        lock_acquire+0x47/0x70
> |        wait_for_completion+0x5c/0x180
> |        takedown_cpu+0x84/0xf0
> |        cpuhp_invoke_callback+0x4a/0x130
> |        cpuhp_down_callbacks+0x3d/0x80
> …
> |
> | other info that might help us debug this:
> |
> |  Possible unsafe locking scenario:
> |        CPU0                    CPU1
> |        ----                    ----
> |   lock(sparse_irq_lock);
> |                                lock((complete)&st->done);
> |                                lock(sparse_irq_lock);
> |   lock((complete)&st->done);
> |
> |  *** DEADLOCK ***
>
> We hold the sparse_irq_lock lock while waiting for the completion in the
> CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
> while the other CPU is waiting for the completion.
> This is not an issue if my interpretation of lockdep here is correct.

Hello Sebastian,

I think you parsed the message correctly.

The message is saying that, for example:

context A (maybe being up?)
--
lock(sparse_irq_lock) // wait for sparse_irq_lock in B to be released
complete(st->done) // impossible to hit here

context B (maybe wanting to synchronize with the cpu being up?)
--
lock(sparse_irq_lock) // acquired successfully
wait_for_completion(st->done) // wait for completion of st->done in A
unlock(sparse_irq_lock) // impossible to hit here

I cannot check the kernel code at the moment.. I wonder if this scenario is
impossible. Could you answer it?

-- 
Thanks,
Byungchul

[toc] | [prev] | [next] | [standalone]

#1720249

On Sat, 26 Aug 2017, Byungchul Park wrote:
> On Fri, Aug 25, 2017 at 11:47 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > We hold the sparse_irq_lock lock while waiting for the completion in the
> > CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
> > while the other CPU is waiting for the completion.
> > This is not an issue if my interpretation of lockdep here is correct.
> 
> Hello Sebastian,
> 
> I think you parsed the message correctly.
> 
> The message is saying that, for example:
> 
> context A (maybe being up?)
> --
> lock(sparse_irq_lock) // wait for sparse_irq_lock in B to be released
> complete(st->done) // impossible to hit here
> 
> context B (maybe wanting to synchronize with the cpu being up?)
> --
> lock(sparse_irq_lock) // acquired successfully
> wait_for_completion(st->done) // wait for completion of st->done in A
> unlock(sparse_irq_lock) // impossible to hit here
> 
> I cannot check the kernel code at the moment.. I wonder if this scenario is
> impossible. Could you answer it?

Yes, it's impossible because cpu hotplug is globally serialized. So the cpu
down scenario cannot happen in parallel with the cpu up scenario.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1721127

On Fri, Aug 25, 2017 at 04:47:55PM +0200, Sebastian Andrzej Siewior wrote:
> On 2017-08-25 12:03:04 [+0200], Borislav Petkov wrote:
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 4.13.0-rc6+ #1 Not tainted
> > ------------------------------------------------------
> 
> While looking at this, I stumbled upon another one also enabled by
> "completion annotation" in the TIP:
> 
> | ======================================================
> | WARNING: possible circular locking dependency detected
> | 4.13.0-rc6-00758-gd80d4177391f-dirty #112 Not tainted
> | ------------------------------------------------------
> | cpu-off.sh/426 is trying to acquire lock:
> |  ((complete)&st->done){+.+.}, at: [<ffffffff810cb344>] takedown_cpu+0x84/0xf0
> |
> | but task is already holding lock:
> |  (sparse_irq_lock){+.+.}, at: [<ffffffff811220f2>] irq_lock_sparse+0x12/0x20
> |
> | which lock already depends on the new lock.
> |
> | the existing dependency chain (in reverse order) is:
> |
> | -> #1 (sparse_irq_lock){+.+.}:
> |        __mutex_lock+0x88/0x9a0
> |        mutex_lock_nested+0x16/0x20
> |        irq_lock_sparse+0x12/0x20
> |        irq_affinity_online_cpu+0x13/0xd0
> |        cpuhp_invoke_callback+0x4a/0x130
> |
> | -> #0 ((complete)&st->done){+.+.}:
> |        check_prev_add+0x351/0x700
> |        __lock_acquire+0x114a/0x1220
> |        lock_acquire+0x47/0x70
> |        wait_for_completion+0x5c/0x180
> |        takedown_cpu+0x84/0xf0
> |        cpuhp_invoke_callback+0x4a/0x130
> |        cpuhp_down_callbacks+0x3d/0x80
> …
> |
> | other info that might help us debug this:
> |
> |  Possible unsafe locking scenario:
> |        CPU0                    CPU1
> |        ----                    ----
> |   lock(sparse_irq_lock);
> |                                lock((complete)&st->done);
> |                                lock(sparse_irq_lock);
> |   lock((complete)&st->done);
> |
> |  *** DEADLOCK ***
> 
> We hold the sparse_irq_lock lock while waiting for the completion in the
> CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
> while the other CPU is waiting for the completion.
> This is not an issue if my interpretation of lockdep here is correct.
> 
> How do we annotate this?

Does something like so work?

---
 include/linux/completion.h | 15 ++++++++++++---
 kernel/kthread.c           | 14 +++++++++++++-
 kernel/sched/completion.c  | 18 +++++++++++++-----
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 791f053f28b7..0eccd2d44c85 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -34,9 +34,9 @@ struct completion {
 };
 
 #ifdef CONFIG_LOCKDEP_COMPLETIONS
-static inline void complete_acquire(struct completion *x)
+static inline void complete_acquire(struct completion *x, int subclass)
 {
-	lock_acquire_exclusive((struct lockdep_map *)&x->map, 0, 0, NULL, _RET_IP_);
+	lock_acquire_exclusive((struct lockdep_map *)&x->map, subclass, 0, NULL, _RET_IP_);
 }
 
 static inline void complete_release(struct completion *x)
@@ -59,7 +59,7 @@ do {									\
 } while (0)
 #else
 #define init_completion(x) __init_completion(x)
-static inline void complete_acquire(struct completion *x) {}
+static inline void complete_acquire(struct completion *x, int subclass) {}
 static inline void complete_release(struct completion *x) {}
 static inline void complete_release_commit(struct completion *x) {}
 #endif
@@ -132,6 +132,15 @@ static inline void reinit_completion(struct completion *x)
 }
 
 extern void wait_for_completion(struct completion *);
+
+#ifndef CONFIG_LOCKDEP
+static inline void
+wait_for_completion_nested(struct completion *x, int subclass)
+{
+	wait_for_completion(x);
+}
+#endif
+
 extern void wait_for_completion_io(struct completion *);
 extern int wait_for_completion_interruptible(struct completion *x);
 extern int wait_for_completion_killable(struct completion *x);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 26db528c1d88..6092702dd908 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -485,7 +485,19 @@ int kthread_park(struct task_struct *k)
 		set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
 		if (k != current) {
 			wake_up_process(k);
-			wait_for_completion(&kthread->parked);
+			/*
+			 * CPU-UP		CPU-DOWN
+			 *
+			 * cpu_hotplug_lock
+			 * wait_for_completion()
+			 * 			cpu_hotplug_lock
+			 * 			complete()
+			 *
+			 * Which normally spells deadlock, except of course
+			 * that up and down are globally serialized so the
+			 * above cannot in fact happen concurrently.
+			 */
+			wait_for_completion_nested(&kthread->parked, 1);
 		}
 	}
 
diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index cc873075c3bd..18ca9b7ef677 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -101,11 +101,11 @@ do_wait_for_common(struct completion *x,
 
 static inline long __sched
 __wait_for_common(struct completion *x,
-		  long (*action)(long), long timeout, int state)
+		  long (*action)(long), long timeout, int state, int subclass)
 {
 	might_sleep();
 
-	complete_acquire(x);
+	complete_acquire(x, subclass);
 
 	spin_lock_irq(&x->wait.lock);
 	timeout = do_wait_for_common(x, action, timeout, state);
@@ -117,9 +117,9 @@ __wait_for_common(struct completion *x,
 }
 
 static long __sched
-wait_for_common(struct completion *x, long timeout, int state)
+wait_for_common(struct completion *x, long timeout, int state, int subclass)
 {
-	return __wait_for_common(x, schedule_timeout, timeout, state);
+	return __wait_for_common(x, schedule_timeout, timeout, state, subclass);
 }
 
 static long __sched
@@ -140,10 +140,18 @@ wait_for_common_io(struct completion *x, long timeout, int state)
  */
 void __sched wait_for_completion(struct completion *x)
 {
-	wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE);
+	wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE, 0);
 }
 EXPORT_SYMBOL(wait_for_completion);
 
+#ifdef CONFIG_LOCKDEP
+void __sched wait_for_completion_nested(struct completion *x, int subclass)
+{
+	wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE, subclass);
+}
+EXPORT_SYMBOL(wait_for_completion);
+#endif
+
 /**
  * wait_for_completion_timeout: - waits for completion of a task (w/timeout)
  * @x:  holds the state of this particular completion

[toc] | [prev] | [next] | [standalone]

#1721651

On Fri, Aug 25, 2017 at 04:47:55PM +0200, Sebastian Andrzej Siewior wrote:
> On 2017-08-25 12:03:04 [+0200], Borislav Petkov wrote:
> | ======================================================
> | WARNING: possible circular locking dependency detected
> | 4.13.0-rc6-00758-gd80d4177391f-dirty #112 Not tainted
> | ------------------------------------------------------
> | cpu-off.sh/426 is trying to acquire lock:
> |  ((complete)&st->done){+.+.}, at: [<ffffffff810cb344>] takedown_cpu+0x84/0xf0
> |
> | but task is already holding lock:
> |  (sparse_irq_lock){+.+.}, at: [<ffffffff811220f2>] irq_lock_sparse+0x12/0x20
> |
> | which lock already depends on the new lock.
> |
> | the existing dependency chain (in reverse order) is:
> |
> | -> #1 (sparse_irq_lock){+.+.}:
> |        __mutex_lock+0x88/0x9a0
> |        mutex_lock_nested+0x16/0x20
> |        irq_lock_sparse+0x12/0x20
> |        irq_affinity_online_cpu+0x13/0xd0
> |        cpuhp_invoke_callback+0x4a/0x130
> |
> | -> #0 ((complete)&st->done){+.+.}:
> |        check_prev_add+0x351/0x700
> |        __lock_acquire+0x114a/0x1220
> |        lock_acquire+0x47/0x70
> |        wait_for_completion+0x5c/0x180
> |        takedown_cpu+0x84/0xf0
> |        cpuhp_invoke_callback+0x4a/0x130
> |        cpuhp_down_callbacks+0x3d/0x80
> …
> |
> | other info that might help us debug this:
> |
> |  Possible unsafe locking scenario:
> |        CPU0                    CPU1
> |        ----                    ----
> |   lock(sparse_irq_lock);
> |                                lock((complete)&st->done);
> |                                lock(sparse_irq_lock);
> |   lock((complete)&st->done);
> |
> |  *** DEADLOCK ***
> 
> We hold the sparse_irq_lock lock while waiting for the completion in the
> CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
> while the other CPU is waiting for the completion.
> This is not an issue if my interpretation of lockdep here is correct.
> 
> How do we annotate this?

Right, so I'm also seeing this one on offline. Its not making sense to
me. wait_for_completion() cannot be #0, we don't hold any other locks
(except the scheduler locks) while we have the complete_acquire() thing.

Now, I have enough hackery here to make it go away, but I'm not in fact
sure I understand things yet :-(

[toc] | [prev] | [next] | [standalone]

#1722758

On Fri, Aug 25, 2017 at 04:47:55PM +0200, Sebastian Andrzej Siewior wrote:
> On 2017-08-25 12:03:04 [+0200], Borislav Petkov wrote:
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 4.13.0-rc6+ #1 Not tainted
> > ------------------------------------------------------
> 
> While looking at this, I stumbled upon another one also enabled by
> "completion annotation" in the TIP:
> 
> | ======================================================
> | WARNING: possible circular locking dependency detected
> | 4.13.0-rc6-00758-gd80d4177391f-dirty #112 Not tainted
> | ------------------------------------------------------
> | cpu-off.sh/426 is trying to acquire lock:
> |  ((complete)&st->done){+.+.}, at: [<ffffffff810cb344>] takedown_cpu+0x84/0xf0
> |
> | but task is already holding lock:
> |  (sparse_irq_lock){+.+.}, at: [<ffffffff811220f2>] irq_lock_sparse+0x12/0x20
> |
> | which lock already depends on the new lock.
> |
> | the existing dependency chain (in reverse order) is:
> |
> | -> #1 (sparse_irq_lock){+.+.}:
> |        __mutex_lock+0x88/0x9a0
> |        mutex_lock_nested+0x16/0x20
> |        irq_lock_sparse+0x12/0x20
> |        irq_affinity_online_cpu+0x13/0xd0
> |        cpuhp_invoke_callback+0x4a/0x130
> |
> | -> #0 ((complete)&st->done){+.+.}:
> |        check_prev_add+0x351/0x700
> |        __lock_acquire+0x114a/0x1220
> |        lock_acquire+0x47/0x70
> |        wait_for_completion+0x5c/0x180
> |        takedown_cpu+0x84/0xf0
> |        cpuhp_invoke_callback+0x4a/0x130
> |        cpuhp_down_callbacks+0x3d/0x80
> …
> |
> | other info that might help us debug this:
> |
> |  Possible unsafe locking scenario:
> |        CPU0                    CPU1
> |        ----                    ----
> |   lock(sparse_irq_lock);
> |                                lock((complete)&st->done);
> |                                lock(sparse_irq_lock);
> |   lock((complete)&st->done);
> |
> |  *** DEADLOCK ***
> 
> We hold the sparse_irq_lock lock while waiting for the completion in the
> CPU-down case and in the CPU-up case we acquire the sparse_irq_lock lock
> while the other CPU is waiting for the completion.
> This is not an issue if my interpretation of lockdep here is correct.
> 
> How do we annotate this?

The below is the nicest thing I could come up with. This results in
_cpu_down and _cpu_up having a different class for st->done.

Its a bit weird, and would probably need a wee comment to explain
things, but it boots and avoids the splat on hotplug.

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

diff --git a/kernel/cpu.c b/kernel/cpu.c
index acf5308fad51..d93df21c5cfb 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -375,13 +375,6 @@ static int cpuhp_up_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
 /*
  * The cpu hotplug threads manage the bringup and teardown of the cpus
  */
-static void cpuhp_create(unsigned int cpu)
-{
-	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
-
-	init_completion(&st->done);
-}
-
 static int cpuhp_should_run(unsigned int cpu)
 {
 	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
@@ -520,7 +513,6 @@ static int cpuhp_kick_ap_work(unsigned int cpu)
 
 static struct smp_hotplug_thread cpuhp_threads = {
 	.store			= &cpuhp_state.thread,
-	.create			= &cpuhp_create,
 	.thread_should_run	= cpuhp_should_run,
 	.thread_fn		= cpuhp_thread_fun,
 	.thread_comm		= "cpuhp/%u",
@@ -687,7 +679,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 			   enum cpuhp_state target)
 {
 	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
-	int prev_state, ret = 0;
+	int i, prev_state, ret = 0;
 
 	if (num_online_cpus() == 1)
 		return -EBUSY;
@@ -697,6 +689,9 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 
 	cpus_write_lock();
 
+	for_each_possible_cpu(i)
+		init_completion(&per_cpu_ptr(&cpuhp_state, i)->done);
+
 	cpuhp_tasks_frozen = tasks_frozen;
 
 	prev_state = st->state;
@@ -802,10 +797,13 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
 {
 	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
 	struct task_struct *idle;
-	int ret = 0;
+	int i, ret = 0;
 
 	cpus_write_lock();
 
+	for_each_possible_cpu(i)
+		init_completion(&per_cpu_ptr(&cpuhp_state, i)->done);
+
 	if (!cpu_present(cpu)) {
 		ret = -EINVAL;
 		goto out;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web