workqueue, pci: INFO: possible recursive locking detected

Hi,

I have been seeing this warning every time during boot. I haven't spent time digging through it though... Please let me know if any machine-specific info is needed.

Regards,
Srivatsa S. Bhat

----------------------------------------------------

=============================================
[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

May be due to missing lock nesting notation

3 locks held by kworker/0:1/142:

 #0:  (events){.+.+.+}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610
 #1:  ((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610
 #2:  (&__lockdep_no_validate__){......}, at: [<ffffffff8139a3ba>] device_attach+0x2a/0xc0

stack backtrace:
CPU: 0 PID: 142 Comm: kworker/0:1 Not tainted 3.11.0-rc1-lockdep-fix-a #6
Hardware name: IBM  -[8737R2A]-/00Y2738, BIOS -[B2E120RUS-1.20]- 11/30/2012
Workqueue: events work_for_cpu_fn
 ffff881036fecd88 ffff881036fef678 ffffffff8161a919 0000000000000003
 ffff881036fec400 ffff881036fef6a8 ffffffff810c2234 ffff881036fec400
 ffff881036fecd88 ffff881036fec400 0000000000000000 ffff881036fef708
Call Trace:
 [<ffffffff8161a919>] dump_stack+0x59/0x80
 [<ffffffff810c2234>] print_deadlock_bug+0xf4/0x100
 [<ffffffff810c3d14>] validate_chain+0x504/0x750
 [<ffffffff810c426d>] __lock_acquire+0x30d/0x580
 [<ffffffff810c4577>] lock_acquire+0x97/0x170
 [<ffffffff81077100>] ? start_flush_work+0x220/0x220
 [<ffffffff81077148>] flush_work+0x48/0xb0
 [<ffffffff81077100>] ? start_flush_work+0x220/0x220
 [<ffffffff810c2c10>] ? mark_held_locks+0x80/0x130
 [<ffffffff81074cfb>] ? queue_work_on+0x4b/0xa0
 [<ffffffff810c2f85>] ? trace_hardirqs_on_caller+0x105/0x1d0
 [<ffffffff810c305d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81077320>] work_on_cpu+0x80/0x90
 [<ffffffff8106f950>] ? wqattrs_hash+0x190/0x190
 [<ffffffff812d37b0>] ? pci_pm_prepare+0x60/0x60
 [<ffffffff812a0059>] ? cpumask_next_and+0x29/0x50
 [<ffffffff812d38da>] __pci_device_probe+0x9a/0xe0
 [<ffffffff81620850>] ? _raw_spin_unlock_irq+0x30/0x50
 [<ffffffff812d4be2>] ? pci_dev_get+0x22/0x30
 [<ffffffff812d4c2a>] pci_device_probe+0x3a/0x60
 [<ffffffff81620850>] ? _raw_spin_unlock_irq+0x30/0x50
 [<ffffffff8139a4bc>] really_probe+0x6c/0x320
 [<ffffffff8139a7b7>] driver_probe_device+0x47/0xa0
 [<ffffffff8139a8c0>] ? __driver_attach+0xb0/0xb0
 [<ffffffff8139a913>] __device_attach+0x53/0x60
 [<ffffffff81398404>] bus_for_each_drv+0x74/0xa0
 [<ffffffff8139a430>] device_attach+0xa0/0xc0
 [<ffffffff812cb2d9>] pci_bus_add_device+0x39/0x60
 [<ffffffff812eec21>] virtfn_add+0x251/0x3e0
 [<ffffffff810c305d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff812ef29f>] sriov_enable+0x22f/0x3d0
 [<ffffffff812ef48d>] pci_enable_sriov+0x4d/0x60
 [<ffffffffa0143045>] be_vf_setup+0x175/0x410 [be2net]
 [<ffffffffa01493ca>] be_setup+0x37a/0x4b0 [be2net]
 [<ffffffffa0149ac0>] be_probe+0x5c0/0x820 [be2net]
 [<ffffffff812d37fe>] local_pci_probe+0x4e/0x90
 [<ffffffff8106f968>] work_for_cpu_fn+0x18/0x30
 [<ffffffff81075e4a>] process_one_work+0x1da/0x610
 [<ffffffff81075dd9>] ? process_one_work+0x169/0x610
 [<ffffffff8107650c>] worker_thread+0x28c/0x3a0
 [<ffffffff81076280>] ? process_one_work+0x610/0x610
 [<ffffffff8107da3e>] kthread+0xee/0x100
 [<ffffffff8107d950>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff8162a71c>] ret_from_fork+0x7c/0xb0
 [<ffffffff8107d950>] ? __init_kthread_worker+0x70/0x70

Quoted 22 lines · Srivatsa S. Bhat

Hi,

I have been seeing this warning every time during boot. I haven't spent time digging through it though... Please let me know if any machine-specific info is needed.

Regards,
Srivatsa S. Bhat

----------------------------------------------------

=============================================
[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

Quoted 7 lines

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

This is false negative,

the two "wfc"s are different, they are
both on stack. flush_work() can't be deadlock in such case:

void foo(void *)

{
	...
	if (xxx)
		work_on_cpu(..., foo, ...);
	...
}

bar()

{
	work_on_cpu(..., foo, ...);
}

The complaint is caused by "work_on_cpu() uses a static lock_class_key". we should fix work_on_cpu().
(but the caller should also be careful, the foo()/local_pci_probe() is re-entering)

But I can't find an elegant fix.

long work_on_cpu(int cpu, long (*fn)(void *), void *arg)

{
	struct work_for_cpu wfc = { .fn = fn, .arg = arg };

(quoted diff) +5 -0

+#ifdef CONFIG_LOCKDEP
+	static struct lock_class_key __key;
+	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lockdep_init_map(&wfc.work.lockdep_map, &wfc.work, &__key, 0);
+#else

	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
	schedule_work_on(cpu, &wfc.work);
	flush_work(&wfc.work);
	return wfc.ret;
}

Quoted 23 lines · Lai Jiangshan

Quoted 22 lines · Srivatsa S. Bhat

Hi,

I have been seeing this warning every time during boot. I haven't spent time digging through it though... Please let me know if any machine-specific info is needed.

Regards,
Srivatsa S. Bhat

----------------------------------------------------

=============================================
[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

Quoted 9 lines

Quoted 7 lines

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

Hi Lai,

Thanks for taking a look into this!

Quoted 2 lines

This is false negative,

I believe you meant false-positive...

Quoted 31 lines

the two "wfc"s are different, they are
both on stack. flush_work() can't be deadlock in such case:

void foo(void *)

{
	...
	if (xxx)
		work_on_cpu(..., foo, ...);
	...
}

bar()

{
	work_on_cpu(..., foo, ...);
}

The complaint is caused by "work_on_cpu() uses a static lock_class_key". we should fix work_on_cpu().
(but the caller should also be careful, the foo()/local_pci_probe() is re-entering)

But I can't find an elegant fix.

long work_on_cpu(int cpu, long (*fn)(void *), void *arg)

{
	struct work_for_cpu wfc = { .fn = fn, .arg = arg };

kernel/workqueue.c +5 -0

+#ifdef CONFIG_LOCKDEP
+	static struct lock_class_key __key;
+	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lockdep_init_map(&wfc.work.lockdep_map, &wfc.work, &__key, 0);
+#else

Quoted 7 lines

	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
	schedule_work_on(cpu, &wfc.work);
	flush_work(&wfc.work);
	return wfc.ret;
}

Unfortunately that didn't seem to fix it.. I applied the patch shown below, and I got the same old warning.

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644

kernel/workqueue.c +4 -0

+	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lockdep_init_map(&wfc.work.lockdep_map, "&wfc.work", &__key, 0);
+#else
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
 	schedule_work_on(cpu, &wfc.work);
 	flush_work(&wfc.work);
 	return wfc.ret;

Warning:
--------

wmi: Mapper loaded
be2net 0000:11:00.0: irq 102 for MSI/MSI-X
be2net 0000:11:00.0: enabled 1 MSI-x vector(s)
be2net 0000:11:00.0: created 0 RSS queue(s) and 1 default RX queue be2net 0000:11:00.0: created 1 TX queue(s)
pci 0000:11:04.0: [19a2:0710] type 00 class 0x020000

=============================================
[ INFO: possible recursive locking detected ] 3.11.0-rc1-wq-fix #10 Not tainted
---------------------------------------------
kworker/0:1/126 is trying to acquire lock:
(&wfc.work){+.+.+.}, at: [<ffffffff810770f0>] flush_work+0x0/0xb0

but task is already holding lock:
(&wfc.work){+.+.+.}, at: [<ffffffff81075dc9>] process_one_work+0x169/0x610

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock(&wfc.work);
lock(&wfc.work);

*** DEADLOCK ***

May be due to missing lock nesting notation

3 locks held by kworker/0:1/126:

 #0:  (events){.+.+.+}, at: [<ffffffff81075dc9>] process_one_work+0x169/0x610
 #1:  (&wfc.work){+.+.+.}, at: [<ffffffff81075dc9>] process_one_work+0x169/0x610
 #2:  (&__lockdep_no_validate__){......}, at: [<ffffffff81398ada>] device_attach+0x2a/0xc0

stack backtrace:
CPU: 0 PID: 126 Comm: kworker/0:1 Not tainted 3.11.0-rc1-wq-fix #10
Hardware name: IBM  -[8737R2A]-/00Y2738, BIOS -[B2E120RUS-1.20]- 11/30/2012
Workqueue: events work_for_cpu_fn
 ffff881036887408 ffff881036889668 ffffffff81619059 0000000000000003
 ffff881036886a80 ffff881036889698 ffffffff810c1624 ffff881036886a80
 ffff881036887408 ffff881036886a80 0000000000000000 ffff8810368896f8
Call Trace:
 [<ffffffff81619059>] dump_stack+0x59/0x80
 [<ffffffff810c1624>] print_deadlock_bug+0xf4/0x100
 [<ffffffff810c3104>] validate_chain+0x504/0x750
 [<ffffffff810c365d>] __lock_acquire+0x30d/0x580
 [<ffffffff810c3967>] lock_acquire+0x97/0x170
 [<ffffffff810770f0>] ? start_flush_work+0x220/0x220
 [<ffffffff81077138>] flush_work+0x48/0xb0
 [<ffffffff810770f0>] ? start_flush_work+0x220/0x220
 [<ffffffff810c2000>] ? mark_held_locks+0x80/0x130
 [<ffffffff81074ceb>] ? queue_work_on+0x4b/0xa0
 [<ffffffff810c2375>] ? trace_hardirqs_on_caller+0x105/0x1d0
 [<ffffffff810c244d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81077334>] work_on_cpu+0xa4/0xc0
 [<ffffffff8106f940>] ? wqattrs_hash+0x190/0x190
 [<ffffffff812d1ed0>] ? pci_pm_prepare+0x60/0x60
 [<ffffffff812d1ffa>] __pci_device_probe+0x9a/0xe0
 [<ffffffff8161ef90>] ? _raw_spin_unlock_irq+0x30/0x50
 [<ffffffff812d3302>] ? pci_dev_get+0x22/0x30
 [<ffffffff812d334a>] pci_device_probe+0x3a/0x60
 [<ffffffff8161ef90>] ? _raw_spin_unlock_irq+0x30/0x50
 [<ffffffff81398bdc>] really_probe+0x6c/0x320
 [<ffffffff81398ed7>] driver_probe_device+0x47/0xa0
 [<ffffffff81398fe0>] ? __driver_attach+0xb0/0xb0
 [<ffffffff81399033>] __device_attach+0x53/0x60
 [<ffffffff81396b24>] bus_for_each_drv+0x74/0xa0
 [<ffffffff81398b50>] device_attach+0xa0/0xc0
 [<ffffffff812c99f9>] pci_bus_add_device+0x39/0x60
 [<ffffffff812ed341>] virtfn_add+0x251/0x3e0
 [<ffffffff810c244d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff812ed9bf>] sriov_enable+0x22f/0x3d0
 [<ffffffff812edbad>] pci_enable_sriov+0x4d/0x60
 [<ffffffffa0127045>] be_vf_setup+0x175/0x410 [be2net]
 [<ffffffffa012d3ca>] be_setup+0x37a/0x4b0 [be2net]
 [<ffffffffa012dac0>] be_probe+0x5c0/0x820 [be2net]
 [<ffffffff812d1f1e>] local_pci_probe+0x4e/0x90
 [<ffffffff8106f958>] work_for_cpu_fn+0x18/0x30
 [<ffffffff81075e3a>] process_one_work+0x1da/0x610
 [<ffffffff81075dc9>] ? process_one_work+0x169/0x610
 [<ffffffff810764fc>] worker_thread+0x28c/0x3a0
 [<ffffffff81076270>] ? process_one_work+0x610/0x610
 [<ffffffff8107da5e>] kthread+0xee/0x100
 [<ffffffff8107d970>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81628e5c>] ret_from_fork+0x7c/0xb0
 [<ffffffff8107d970>] ? __init_kthread_worker+0x70/0x70

be2net 0000:11:04.0: enabling device (0040 -> 0042) be2net 0000:11:04.0: Could not use PCIe error reporting be2net 0000:11:04.0: VF is not privileged to issue opcode 89-1 be2net 0000:11:04.0: VF is not privileged to issue opcode 125-1

Regards,
Srivatsa S. Bhat

Quoted 15 lines · Srivatsa S. Bhat

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4758

Quoted 2 lines

kernel/workqueue.c +2 -0

+#ifdef CONFIG_LOCKDEP
+	static struct lock_class_key __key;

Sorry, this "static" should be removed.

Quoted 16 lines · Lai Jiangshan

Quoted 15 lines · Srivatsa S. Bhat

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

Quoted 5 lines

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4758

Quoted 2 lines

kernel/workqueue.c +2 -0

+#ifdef CONFIG_LOCKDEP
+	static struct lock_class_key __key;

Sorry, this "static" should be removed.

That didn't help either :-( Because it makes lockdep unhappy, since the key isn't persistent.

This is the patch I used:

---

kernel/workqueue.c +6 -0

index f02c4a4..7967e3b 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
+#ifdef CONFIG_LOCKDEP
+	struct lock_class_key __key;
+	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lockdep_init_map(&wfc.work.lockdep_map, "&wfc.work", &__key, 0);
+#else
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
 	schedule_work_on(cpu, &wfc.work);
 	flush_work(&wfc.work);
 	return wfc.ret;

And here are the new warnings:

Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252) io scheduler noop registered
io scheduler deadline registered
io scheduler cfq registered (default)

BUG: key ffff881039557b98 not in .data!
------------[ cut here ]------------
WARNING: CPU: 8 PID: 1 at kernel/lockdep.c:2987 lockdep_init_map+0x168/0x170()

From 0096b9dac2282ec03d59a3f665b92977381a18ad Mon Sep 17 00:00:00 2001

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

Quoted 53 lines · Srivatsa S. Bhat

Quoted 16 lines · Lai Jiangshan

Quoted 15 lines · Srivatsa S. Bhat

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

Quoted 5 lines

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4758

Quoted 2 lines

kernel/workqueue.c +2 -0

+#ifdef CONFIG_LOCKDEP
+	static struct lock_class_key __key;

Sorry, this "static" should be removed.

That didn't help either :-( Because it makes lockdep unhappy, since the key isn't persistent.

This is the patch I used:

---

kernel/workqueue.c +6 -0

index f02c4a4..7967e3b 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
+#ifdef CONFIG_LOCKDEP
+	struct lock_class_key __key;
+	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lockdep_init_map(&wfc.work.lockdep_map, "&wfc.work", &__key, 0);
+#else
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
 	schedule_work_on(cpu, &wfc.work);
 	flush_work(&wfc.work);
 	return wfc.ret;

And here are the new warnings:

Quoted 7 lines

Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252) io scheduler noop registered
io scheduler deadline registered
io scheduler cfq registered (default)

BUG: key ffff881039557b98 not in .data!
------------[ cut here ]------------
WARNING: CPU: 8 PID: 1 at kernel/lockdep.c:2987 lockdep_init_map+0x168/0x170()

Sorry again.

From 0096b9dac2282ec03d59a3f665b92977381a18ad Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Mon, 22 Jul 2013 19:08:51 +0800
Subject: [PATCH] [PATCH] workqueue: allow the function of work_on_cpu() can
call work_on_cpu()

If the @fn call work_on_cpu() again, the lockdep will complain:

Quoted 10 lines

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

Quoted 8 lines

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

It is false-positive lockdep report. In this sutiation, the two "wfc"s of the two work_on_cpu() are different, they are both on stack. flush_work() can't be deadlock.

To fix this, we need to avoid the lockdep checking in this case, But we don't want to change the flush_work(), so we use completion instead of flush_work() in the work_on_cpu().

Reported-by Srivatsa S. Bhat <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

---

 kernel/workqueue.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

kernel/workqueue.c +4 -1

index f02c4a4..b021a45 100644
@@ -4731,6 +4731,7 @@ struct work_for_cpu {
 	long (*fn)(void *);
 	void *arg;
 	long ret;
+	struct completion done;
 };
 
 static void work_for_cpu_fn(struct work_struct *work)

@@ -4738,6 +4739,7 @@ static void work_for_cpu_fn(struct work_struct *work)
 	struct work_for_cpu *wfc = container_of(work, struct work_for_cpu, work);
 
 	wfc->ret = wfc->fn(wfc->arg);
+	complete(&wfc->done);
 }
 
 /**

@@ -4755,8 +4757,9 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	init_completion(&wfc.done);
 	schedule_work_on(cpu, &wfc.work);
-	flush_work(&wfc.work);
+	wait_for_completion(&wfc.done);
 	return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);

kernel/workqueue.c +2 -0

index f02c4a4..b021a45 100644
@@ -4731,6 +4731,7 @@ struct work_for_cpu {
 	long (*fn)(void *);
 	void *arg;
 	long ret;
+	struct completion done;
 };
 
 static void work_for_cpu_fn(struct work_struct *work)

@@ -4738,6 +4739,7 @@ static void work_for_cpu_fn(struct work_struct *work)
 	struct work_for_cpu *wfc = container_of(work, struct work_for_cpu, work);
 
 	wfc->ret = wfc->fn(wfc->arg);
+	complete(&wfc->done);
 }
 
 /**

Quoted 95 lines · Lai Jiangshan

Quoted 53 lines · Srivatsa S. Bhat

Quoted 16 lines · Lai Jiangshan

Quoted 15 lines · Srivatsa S. Bhat

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
   struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

Quoted 5 lines

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4758

Quoted 2 lines

kernel/workqueue.c +2 -0

+#ifdef CONFIG_LOCKDEP
+  static struct lock_class_key __key;

Sorry, this "static" should be removed.

That didn't help either :-( Because it makes lockdep unhappy, since the key isn't persistent.

This is the patch I used:

---

kernel/workqueue.c +6 -0

index f02c4a4..7967e3b 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
     struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
+#ifdef CONFIG_LOCKDEP
+    struct lock_class_key __key;
+    INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+    lockdep_init_map(&wfc.work.lockdep_map, "&wfc.work", &__key, 0);
+#else
     INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
     schedule_work_on(cpu, &wfc.work);
     flush_work(&wfc.work);
     return wfc.ret;

And here are the new warnings:

Quoted 7 lines

Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252) io scheduler noop registered
io scheduler deadline registered
io scheduler cfq registered (default)

BUG: key ffff881039557b98 not in .data!
------------[ cut here ]------------
WARNING: CPU: 8 PID: 1 at kernel/lockdep.c:2987 lockdep_init_map+0x168/0x170()

Sorry again.

From 0096b9dac2282ec03d59a3f665b92977381a18ad Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Mon, 22 Jul 2013 19:08:51 +0800
Subject: [PATCH] [PATCH] workqueue: allow the function of work_on_cpu() can
call work_on_cpu()

If the @fn call work_on_cpu() again, the lockdep will complain:

Quoted 10 lines

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

Quoted 8 lines

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

It is false-positive lockdep report. In this sutiation, the two "wfc"s of the two work_on_cpu() are different, they are both on stack. flush_work() can't be deadlock.

Quoted 7 lines

To fix this, we need to avoid the lockdep checking in this case, But we don't want to change the flush_work(), so we use completion instead of flush_work() in the work_on_cpu().

Reported-by Srivatsa S. Bhat <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

---

That worked, thanks a lot!

Tested-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Quoted 26 lines

 kernel/workqueue.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

kernel/workqueue.c +2 -0

index f02c4a4..b021a45 100644
@@ -4731,6 +4731,7 @@ struct work_for_cpu {
      long (*fn)(void *);
      void *arg;
      long ret;
+     struct completion done;
 };
 
 static void work_for_cpu_fn(struct work_struct *work)

@@ -4738,6 +4739,7 @@ static void work_for_cpu_fn(struct work_struct *work)
      struct work_for_cpu *wfc = container_of(work, struct work_for_cpu, work);
 
      wfc->ret = wfc->fn(wfc->arg);
+     complete(&wfc->done);
 }
 
 /**

@@ -4755,8 +4757,9 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
      struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4765

Quoted 9 lines

kernel/workqueue.c +2 -1

      INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+     init_completion(&wfc.done);
      schedule_work_on(cpu, &wfc.work);
-     flush_work(&wfc.work);
+     wait_for_completion(&wfc.done);
      return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);

[+cc Alex, Yinghai, linux-pci]

Isn't this for the same issue Alex and others have been working on?

It doesn't feel like we have consensus on how this should be fixed. You're proposing a change to work_on_cpu(), Alex proposed a change to pci_call_probe() [1], Yinghai proposed some changes to the PCI core SR-IOV code and several drivers [2].

[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]

[+cc Alex, Yinghai, linux-pci]

Quoted 131 lines · Srivatsa S. Bhat

Quoted 90 lines · Lai Jiangshan

Quoted 50 lines · Srivatsa S. Bhat

Quoted 15 lines · Lai Jiangshan

Quoted 14 lines · Srivatsa S. Bhat

---

 kernel/workqueue.c |    6 ++++++
 1 file changed, 6 insertions(+)

kernel/workqueue.c +0 -0

index f02c4a4..07d9a67 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
   struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

Quoted 4 lines

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4758

Quoted 2 lines

kernel/workqueue.c +2 -0

+#ifdef CONFIG_LOCKDEP
+  static struct lock_class_key __key;

Sorry, this "static" should be removed.

That didn't help either :-( Because it makes lockdep unhappy, since the key isn't persistent.

This is the patch I used:

---

kernel/workqueue.c +6 -0

index f02c4a4..7967e3b 100644
@@ -4754,7 +4754,13 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
     struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
+#ifdef CONFIG_LOCKDEP
+    struct lock_class_key __key;
+    INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+    lockdep_init_map(&wfc.work.lockdep_map, "&wfc.work", &__key, 0);
+#else
     INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+#endif
     schedule_work_on(cpu, &wfc.work);
     flush_work(&wfc.work);
     return wfc.ret;

And here are the new warnings:

Quoted 7 lines

Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252) io scheduler noop registered
io scheduler deadline registered
io scheduler cfq registered (default)

BUG: key ffff881039557b98 not in .data!
------------[ cut here ]------------
WARNING: CPU: 8 PID: 1 at kernel/lockdep.c:2987 lockdep_init_map+0x168/0x170()

Sorry again.

From 0096b9dac2282ec03d59a3f665b92977381a18ad Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Mon, 22 Jul 2013 19:08:51 +0800
Subject: [PATCH] [PATCH] workqueue: allow the function of work_on_cpu() can
call work_on_cpu()

If the @fn call work_on_cpu() again, the lockdep will complain:

Quoted 10 lines

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

Quoted 8 lines

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

It is false-positive lockdep report. In this sutiation, the two "wfc"s of the two work_on_cpu() are different, they are both on stack. flush_work() can't be deadlock.

Quoted 7 lines

To fix this, we need to avoid the lockdep checking in this case, But we don't want to change the flush_work(), so we use completion instead of flush_work() in the work_on_cpu().

Reported-by Srivatsa S. Bhat <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

---

That worked, thanks a lot!

Tested-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Quoted 26 lines

 kernel/workqueue.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

kernel/workqueue.c +2 -0

index f02c4a4..b021a45 100644
@@ -4731,6 +4731,7 @@ struct work_for_cpu {
      long (*fn)(void *);
      void *arg;
      long ret;
+     struct completion done;
 };
 
 static void work_for_cpu_fn(struct work_struct *work)

@@ -4738,6 +4739,7 @@ static void work_for_cpu_fn(struct work_struct *work)
      struct work_for_cpu *wfc = container_of(work, struct work_for_cpu, work);
 
      wfc->ret = wfc->fn(wfc->arg);
+     complete(&wfc->done);
 }
 
 /**

@@ -4755,8 +4757,9 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
      struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

Quoted 9 lines

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4765

Quoted 9 lines

kernel/workqueue.c +2 -1

      INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+     init_completion(&wfc.done);
      schedule_work_on(cpu, &wfc.work);
-     flush_work(&wfc.work);
+     wait_for_completion(&wfc.done);
      return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);

Isn't this for the same issue Alex and others have been working on?

It doesn't feel like we have consensus on how this should be fixed. You're proposing a change to work_on_cpu(), Alex proposed a change to pci_call_probe() [1], Yinghai proposed some changes to the PCI core SR-IOV code and several drivers [2].

[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]

Quoted 23 lines · Lai Jiangshan

kernel/workqueue.c +2 -0

index f02c4a4..b021a45 100644
@@ -4731,6 +4731,7 @@ struct work_for_cpu {
 	long (*fn)(void *);
 	void *arg;
 	long ret;
+	struct completion done;
 };
 
 static void work_for_cpu_fn(struct work_struct *work)

@@ -4738,6 +4739,7 @@ static void work_for_cpu_fn(struct work_struct *work)
 	struct work_for_cpu *wfc = container_of(work, struct work_for_cpu, work);
 
 	wfc->ret = wfc->fn(wfc->arg);
+	complete(&wfc->done);
 }
 
 /**

@@ -4755,8 +4757,9 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 

kernel/workqueue.c long work_on_cpu(int cpu, long (*fn)(void *), void *arg) · line 4762

Quoted 5 lines

kernel/workqueue.c +2 -1

 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	init_completion(&wfc.done);
 	schedule_work_on(cpu, &wfc.work);
-	flush_work(&wfc.work);
+	wait_for_completion(&wfc.done);

Hmmm... it's kinda nasty. Given how infrequently work_on_cpu() users nest, I think it'd be cleaner to have work_on_cpu_nested() which takes @subclass. It requires extra work on the caller's part but I think that actually is useful as nested work_on_cpu()s are pretty weird things.

The problem is that the userS may not know their work_on_cpu() nested, especially when work_on_cpu()s are on different subsystems and the call depth is deep enough but the nested work_on_cpu() depends on some conditions.

I prefer to change the user instead of introducing work_on_cpu_nested(), and I accept to change the user only instead of change work_on_cpu() since there is only one nested-calls case found.

But I'm thinking, since nested work_on_cpu() don't have any problem, Why workqueue.c don't offer a more friendly API/behavior?

Hey, Lai.

Quoted 3 lines · Lai Jiangshan

The problem is that the userS may not know their work_on_cpu() nested, especially when work_on_cpu()s are on different subsystems and the call depth is deep enough but the nested work_on_cpu() depends on some conditions.

Yeah, that's a possibility. Not sure how much it'd actually matter tho given that this is the only instance we have and we've had the lockdep annotation for years.

Quoted 6 lines

I prefer to change the user instead of introducing work_on_cpu_nested(), and I accept to change the user only instead of change work_on_cpu() since there is only one nested-calls case found.

But I'm thinking, since nested work_on_cpu() don't have any problem, Why workqueue.c don't offer a more friendly API/behavior?

If we wanna solve it from workqueue side, let's please do it by introduing an internal flush_work() variant which skips the lockdep annotation. I'd really like to avoid using completion here. It's nasty as it depends solely on the fact that completion doesn't have lockdep annotation yet. Let's do it explicitly.

Thanks.

From 269bf1a2f47f04e0daf429c2cdf4052b4e8fb309 Mon Sep 17 00:00:00 2001

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

From c2fda509667b0fda4372a237f5a59ea4570b1627 Mon Sep 17 00:00:00 2001

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

Applied to wq/for-3.11-fixes with comment and subject tweaks.

Thanks!

---------- 8< ------------

From c2fda509667b0fda4372a237f5a59ea4570b1627 Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Wed, 24 Jul 2013 18:31:42 +0800

If the @fn call work_on_cpu() again, the lockdep will complain:

Quoted 10 lines

[ INFO: possible recursive locking detected ] 3.11.0-rc1-lockdep-fix-a #6 Not tainted
---------------------------------------------
kworker/0:1/142 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81077100>] flush_work+0x0/0xb0

but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81075dd9>] process_one_work+0x169/0x610

other info that might help us debug this:

Quoted 8 lines

 Possible unsafe locking scenario:

       CPU0
       ----

lock((&wfc.work));
lock((&wfc.work));

*** DEADLOCK ***

It is false-positive lockdep report. In this sutiation, the two "wfc"s of the two work_on_cpu() are different, they are both on stack. flush_work() can't be deadlock.

To fix this, we need to avoid the lockdep checking in this case, thus we instroduce a internal __flush_work() which skip the lockdep.

tj: Minor comment adjustment.

Signed-off-by Lai Jiangshan <[email protected]>

Reported-by "Srivatsa S. Bhat" <[email protected]>

Reported-by Alexander Duyck <[email protected]>

Signed-off-by Tejun Heo <[email protected]>

---

 kernel/workqueue.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

kernel/workqueue.c +22 -10

index f02c4a4..55f5f0a 100644
@@ -2817,6 +2817,19 @@ already_gone:
 	return false;
 }
 
+static bool __flush_work(struct work_struct *work)
+{
+	struct wq_barrier barr;
+
+	if (start_flush_work(work, &barr)) {
+		wait_for_completion(&barr.done);
+		destroy_work_on_stack(&barr.work);
+		return true;
+	} else {
+		return false;
+	}
+}
+
 /**
  * flush_work - wait for a work to finish executing the last queueing instance
  * @work: the work to flush

@@ -2830,18 +2843,10 @@ already_gone:
  */
 bool flush_work(struct work_struct *work)
 {
-	struct wq_barrier barr;
-
 	lock_map_acquire(&work->lockdep_map);
 	lock_map_release(&work->lockdep_map);
 
-	if (start_flush_work(work, &barr)) {
-		wait_for_completion(&barr.done);
-		destroy_work_on_stack(&barr.work);
-		return true;
-	} else {
-		return false;
-	}
+	return __flush_work(work);
 }
 EXPORT_SYMBOL_GPL(flush_work);
 

@@ -4756,7 +4761,14 @@ long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
 	schedule_work_on(cpu, &wfc.work);
-	flush_work(&wfc.work);
+
+	/*
+	 * The work item is on-stack and can't lead to deadlock through
+	 * flushing.  Use __flush_work() to avoid spurious lockdep warnings
+	 * when work_on_cpu()s are nested.
+	 */
+	__flush_work(&wfc.work);
+
 	return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);