* [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
@ 2010-03-31 10:51 Amerigo Wang
2010-03-31 11:25 ` Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: Amerigo Wang @ 2010-03-31 10:51 UTC (permalink / raw)
To: linux-kernel
Cc: Tejun Heo, Rusty Russell, Oleg Nesterov, Amerigo Wang, akpm,
Ingo Molnar
This fixes a lockdep warning when invoking destroy_workqueue(),
because the lockdep annotations are invoked under cpu_add_remove_lock.
So, move the lockdep annotations before taking cpu_add_remove_lock
in destroy_workqueue(), this will not affect the original purpose
of adding them for destroy_workqueue() etc.
However, it will affect another caller of cleanup_workqueue_thread(),
that is, workqueue_cpu_callback(). This should be fine, because there
are no other cases than cpu hotplug could call it.
Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index dee4865..0f050e2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1025,9 +1025,6 @@ static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
if (cwq->thread == NULL)
return;
- lock_map_acquire(&cwq->wq->lockdep_map);
- lock_map_release(&cwq->wq->lockdep_map);
-
flush_cpu_workqueue(cwq);
/*
* If the caller is CPU_POST_DEAD and cwq->worklist was not empty,
@@ -1055,6 +1052,9 @@ void destroy_workqueue(struct workqueue_struct *wq)
const struct cpumask *cpu_map = wq_cpu_map(wq);
int cpu;
+ lock_map_acquire(&wq->lockdep_map);
+ lock_map_release(&wq->lockdep_map);
+
cpu_maps_update_begin();
spin_lock(&workqueue_lock);
list_del(&wq->list);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-03-31 10:51 [Patch] workqueue: move lockdep annotations up to destroy_workqueue() Amerigo Wang
@ 2010-03-31 11:25 ` Oleg Nesterov
2010-04-01 2:45 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2010-03-31 11:25 UTC (permalink / raw)
To: Amerigo Wang; +Cc: linux-kernel, Tejun Heo, Rusty Russell, akpm, Ingo Molnar
On 03/31, Amerigo Wang wrote:
>
> This fixes a lockdep warning when invoking destroy_workqueue(),
> because the lockdep annotations are invoked under cpu_add_remove_lock.
Confused. Why does lockdep complains?
> So, move the lockdep annotations before taking cpu_add_remove_lock
> in destroy_workqueue(), this will not affect the original purpose
> of adding them for destroy_workqueue() etc.
>
> However, it will affect another caller of cleanup_workqueue_thread(),
> that is, workqueue_cpu_callback(). This should be fine, because there
> are no other cases than cpu hotplug could call it.
OK, but nobody should take cpu_maps_update_begin() under wq->lockdep_map,
in particular work->func() must not.
I must have missed something, but it seems to me this patch tries to
supress the valid warning.
Could you please clarify?
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-03-31 11:25 ` Oleg Nesterov
@ 2010-04-01 2:45 ` Cong Wang
2010-04-01 3:56 ` Tejun Heo
2010-04-01 16:36 ` Oleg Nesterov
0 siblings, 2 replies; 14+ messages in thread
From: Cong Wang @ 2010-04-01 2:45 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Tejun Heo, Rusty Russell, akpm, Ingo Molnar
Oleg Nesterov wrote:
> On 03/31, Amerigo Wang wrote:
>> This fixes a lockdep warning when invoking destroy_workqueue(),
>> because the lockdep annotations are invoked under cpu_add_remove_lock.
>
> Confused. Why does lockdep complains?
Because lockdep annotations are added to prevent other locks are taken
before calling these functions.
>
>> So, move the lockdep annotations before taking cpu_add_remove_lock
>> in destroy_workqueue(), this will not affect the original purpose
>> of adding them for destroy_workqueue() etc.
>>
>> However, it will affect another caller of cleanup_workqueue_thread(),
>> that is, workqueue_cpu_callback(). This should be fine, because there
>> are no other cases than cpu hotplug could call it.
>
> OK, but nobody should take cpu_maps_update_begin() under wq->lockdep_map,
> in particular work->func() must not.
>
> I must have missed something, but it seems to me this patch tries to
> supress the valid warning.
>
> Could you please clarify?
>
Sure, below is the whole warning. Please teach me how this is valid.
Thanks.
Mar 31 16:15:00 dhcp-66-70-5 kernel: bonding: bond0: released all slaves
Mar 31 16:15:00 dhcp-66-70-5 kernel:
Mar 31 16:15:00 dhcp-66-70-5 kernel: =======================================================
Mar 31 16:15:00 dhcp-66-70-5 kernel: [ INFO: possible circular locking dependency detected ]
Mar 31 16:15:00 dhcp-66-70-5 kernel: 2.6.34-rc3 #90
Mar 31 16:15:00 dhcp-66-70-5 kernel: -------------------------------------------------------
Mar 31 16:15:00 dhcp-66-70-5 kernel: modprobe/5264 is trying to acquire lock:
Mar 31 16:15:00 dhcp-66-70-5 kernel: ((bond_dev->name)){+.+...}, at: [<ffffffff8108524a>] cleanup_workqueue_thread+0x2b/0x10b
Mar 31 16:15:00 dhcp-66-70-5 kernel:
Mar 31 16:15:00 dhcp-66-70-5 kernel: but task is already holding lock:
Mar 31 16:15:00 dhcp-66-70-5 kernel: (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
Mar 31 16:15:00 dhcp-66-70-5 kernel:
Mar 31 16:15:00 dhcp-66-70-5 kernel: which lock already depends on the new lock.
Mar 31 16:15:01 dhcp-66-70-5 kernel:
Mar 31 16:15:01 dhcp-66-70-5 kernel:
Mar 31 16:15:01 dhcp-66-70-5 kernel: the existing dependency chain (in reverse order) is:
Mar 31 16:15:01 dhcp-66-70-5 kernel:
Mar 31 16:15:01 dhcp-66-70-5 kernel: -> #3 (cpu_add_remove_lock){+.+.+.}:
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810853cd>] destroy_workqueue+0x41/0x107
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
Mar 31 16:15:02 dhcp-66-70-5 kernel:
Mar 31 16:15:02 dhcp-66-70-5 kernel: -> #2 (rtnl_mutex){+.+.+.}:
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffffa0836779>] bond_mii_monitor+0x39f/0x74b [bonding]
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108654f>] worker_thread+0x2da/0x46c
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108b1ea>] kthread+0xdd/0xec
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
Mar 31 16:15:02 dhcp-66-70-5 kernel:
Mar 31 16:15:02 dhcp-66-70-5 kernel: -> #1 ((&(&bond->mii_work)->work)){+.+...}:
Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81086542>] worker_thread+0x2cd/0x46c
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8108b1ea>] kthread+0xdd/0xec
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
Mar 31 16:15:03 dhcp-66-70-5 kernel:
Mar 31 16:15:03 dhcp-66-70-5 kernel: -> #0 ((bond_dev->name)){+.+...}:
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a6696>] validate_chain+0xaee/0x1540
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
Mar 31 16:15:03 dhcp-66-70-5 kernel:
Mar 31 16:15:03 dhcp-66-70-5 kernel: other info that might help us debug this:
Mar 31 16:15:03 dhcp-66-70-5 kernel:
Mar 31 16:15:03 dhcp-66-70-5 kernel: 2 locks held by modprobe/5264:
Mar 31 16:15:03 dhcp-66-70-5 kernel: #0: (rtnl_mutex){+.+.+.}, at: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
Mar 31 16:15:03 dhcp-66-70-5 kernel: #1: (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
Mar 31 16:15:03 dhcp-66-70-5 kernel:
Mar 31 16:15:03 dhcp-66-70-5 kernel: stack backtrace:
Mar 31 16:15:03 dhcp-66-70-5 kernel: Pid: 5264, comm: modprobe Not tainted 2.6.34-rc3 #90
Mar 31 16:15:03 dhcp-66-70-5 kernel: Call Trace:
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a50c1>] print_circular_bug+0x169/0x17e
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a6696>] validate_chain+0xaee/0x1540
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8108524a>] ? cleanup_workqueue_thread+0x2b/0x10b
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8108524a>] ? cleanup_workqueue_thread+0x2b/0x10b
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0833a19>] ? bond_remove_proc_entry+0x0/0x109 [bonding]
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81003a8c>] ? sysret_check+0x27/0x62
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a3268>] ? trace_hardirqs_on_caller+0x264/0x29f
Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 2:45 ` Cong Wang
@ 2010-04-01 3:56 ` Tejun Heo
2010-04-01 4:09 ` Cong Wang
2010-04-01 16:36 ` Oleg Nesterov
1 sibling, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2010-04-01 3:56 UTC (permalink / raw)
To: Cong Wang; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Hello, guys.
On 04/01/2010 11:45 AM, Cong Wang wrote:
>> OK, but nobody should take cpu_maps_update_begin() under wq->lockdep_map,
>> in particular work->func() must not.
>>
>> I must have missed something, but it seems to me this patch tries to
>> supress the valid warning.
>>
>> Could you please clarify?
>
> Sure, below is the whole warning. Please teach me how this is valid.
I still have some trouble interpreting lockdep warnings. Please
correct me if I get something wrong.
> modprobe/5264 is trying to acquire lock:
> ((bond_dev->name)){+.+...}, at: [<ffffffff8108524a>] cleanup_workqueue_thread+0x2b/0x10b
>
> but task is already holding lock:
> (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
This (cpu hotplug -> wq) is the expected sequence. Plug cpu
hotplugging and then flush cpu workqueues.
> which lock already depends on the new lock.
But lockdep says the other way around has already happened.
> the existing dependency chain (in reverse order) is:
>
> -> #3 (cpu_add_remove_lock){+.+.+.}:
> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
> [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
> [<ffffffff810853cd>] destroy_workqueue+0x41/0x107
> [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
> [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
> [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
> [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
> [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
> [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
> [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
> [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
> [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
This is bond_uninit() calling destroy_workqueue() but I don't get how
this thread would be already holding wq lock.
> -> #2 (rtnl_mutex){+.+.+.}:
> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
> [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
> [<ffffffffa0836779>] bond_mii_monitor+0x39f/0x74b [bonding]
> [<ffffffff8108654f>] worker_thread+0x2da/0x46c
> [<ffffffff8108b1ea>] kthread+0xdd/0xec
> [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
>
> -> #1 ((&(&bond->mii_work)->work)){+.+...}:
> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> [<ffffffff81086542>] worker_thread+0x2cd/0x46c
> [<ffffffff8108b1ea>] kthread+0xdd/0xec
> [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
These two are form a workqueue worker thread and I don't quite
understand why they are here.
> -> #0 ((bond_dev->name)){+.+...}:
> [<ffffffff810a6696>] validate_chain+0xaee/0x1540
> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
> [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
> [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
> [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
> [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
> [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
> [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
> [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
> [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
> [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
> [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
This seems to be from the original thread of frame#3. It's grabbing
wq lock here but the problem is that the lock will be released
immediately, so bond_dev->name (the wq) can't be held by the time it
reaches frame#3. How is this dependency chain completed? Is it
somehow transitive through rtnl_mutex?
> other info that might help us debug this:
>
> 2 locks held by modprobe/5264:
> #0: (rtnl_mutex){+.+.+.}, at: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
> #1: (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
Isn't there a circular dependency here? bonding_exit() calls
destroy_workqueue() under rtnl_mutex but destroy_workqueue() should
flush works which could be trying to grab rtnl_lock. Or am I
completely misunderstanding locking here?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 3:56 ` Tejun Heo
@ 2010-04-01 4:09 ` Cong Wang
2010-04-01 4:14 ` Tejun Heo
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2010-04-01 4:09 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Tejun Heo wrote:
> Hello, guys.
>
> On 04/01/2010 11:45 AM, Cong Wang wrote:
>>> OK, but nobody should take cpu_maps_update_begin() under wq->lockdep_map,
>>> in particular work->func() must not.
>>>
>>> I must have missed something, but it seems to me this patch tries to
>>> supress the valid warning.
>>>
>>> Could you please clarify?
>> Sure, below is the whole warning. Please teach me how this is valid.
>
> I still have some trouble interpreting lockdep warnings. Please
> correct me if I get something wrong.
>
>> modprobe/5264 is trying to acquire lock:
>> ((bond_dev->name)){+.+...}, at: [<ffffffff8108524a>] cleanup_workqueue_thread+0x2b/0x10b
>>
>> but task is already holding lock:
>> (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
>
> This (cpu hotplug -> wq) is the expected sequence. Plug cpu
> hotplugging and then flush cpu workqueues.
>
>> which lock already depends on the new lock.
>
> But lockdep says the other way around has already happened.
>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #3 (cpu_add_remove_lock){+.+.+.}:
>> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
>> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
>> [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
>> [<ffffffff810853cd>] destroy_workqueue+0x41/0x107
>> [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
>> [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
>> [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
>> [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
>> [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
>> [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
>> [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
>> [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
>> [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
>
> This is bond_uninit() calling destroy_workqueue() but I don't get how
> this thread would be already holding wq lock.
destroy_workqueue() does hold wq lock and then releases it.
>
>> -> #2 (rtnl_mutex){+.+.+.}:
>> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
>> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
>> [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
>> [<ffffffffa0836779>] bond_mii_monitor+0x39f/0x74b [bonding]
>> [<ffffffff8108654f>] worker_thread+0x2da/0x46c
>> [<ffffffff8108b1ea>] kthread+0xdd/0xec
>> [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
>>
>> -> #1 ((&(&bond->mii_work)->work)){+.+...}:
>> [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
>> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> [<ffffffff81086542>] worker_thread+0x2cd/0x46c
>> [<ffffffff8108b1ea>] kthread+0xdd/0xec
>> [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
>
> These two are form a workqueue worker thread and I don't quite
> understand why they are here.
>
>> -> #0 ((bond_dev->name)){+.+...}:
>> [<ffffffff810a6696>] validate_chain+0xaee/0x1540
>> [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
>> [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
>> [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
>> [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
>> [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
>> [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
>> [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
>> [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
>> [<ffffffffa0845ca6>] bonding_exit+0x5a/0x76 [bonding]
>> [<ffffffff810b7749>] sys_delete_module+0x306/0x3b1
>> [<ffffffff81003a5b>] system_call_fastpath+0x16/0x1b
>
> This seems to be from the original thread of frame#3. It's grabbing
> wq lock here but the problem is that the lock will be released
> immediately, so bond_dev->name (the wq) can't be held by the time it
> reaches frame#3. How is this dependency chain completed? Is it
> somehow transitive through rtnl_mutex?
wq lock is held *after* cpu_add_remove_lock, lockdep also said this,
the process is trying to hold wq lock while having cpu_add_remove_lock.
>
>> other info that might help us debug this:
>>
>> 2 locks held by modprobe/5264:
>> #0: (rtnl_mutex){+.+.+.}, at: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
>> #1: (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810631d1>] cpu_maps_update_begin+0x1e/0x27
>
> Isn't there a circular dependency here? bonding_exit() calls
> destroy_workqueue() under rtnl_mutex but destroy_workqueue() should
> flush works which could be trying to grab rtnl_lock. Or am I
> completely misunderstanding locking here?
Sure, that is why I sent another patch for bonding. :)
After this patch, another lockdep warning appears, it is exactly what you expect.
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 4:09 ` Cong Wang
@ 2010-04-01 4:14 ` Tejun Heo
2010-04-01 4:28 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2010-04-01 4:14 UTC (permalink / raw)
To: Cong Wang; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Hello,
On 04/01/2010 01:09 PM, Cong Wang wrote:
>> This seems to be from the original thread of frame#3. It's grabbing
>> wq lock here but the problem is that the lock will be released
>> immediately, so bond_dev->name (the wq) can't be held by the time it
>> reaches frame#3. How is this dependency chain completed? Is it
>> somehow transitive through rtnl_mutex?
>
> wq lock is held *after* cpu_add_remove_lock, lockdep also said this,
> the process is trying to hold wq lock while having cpu_add_remove_lock.
Yeah yeah, I'm just failing to see how the other direction is
completed. ie. where does the kernel try to grab cpu_add_remove_lock
*after* grabbing wq lock?
>> Isn't there a circular dependency here? bonding_exit() calls
>> destroy_workqueue() under rtnl_mutex but destroy_workqueue() should
>> flush works which could be trying to grab rtnl_lock. Or am I
>> completely misunderstanding locking here?
>
> Sure, that is why I sent another patch for bonding. :)
Ah... great. :-)
> After this patch, another lockdep warning appears, it is exactly what
> you expect.
Hmmm... can you please try to see whether this circular locking
warning involving wq->lockdep_map is reproducible w/ the bonding
locking fixed? I still can't see where wq -> cpu_add_remove_lock
dependency is created.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 4:14 ` Tejun Heo
@ 2010-04-01 4:28 ` Cong Wang
2010-04-01 4:59 ` Tejun Heo
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2010-04-01 4:28 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Tejun Heo wrote:
> Hello,
>
> On 04/01/2010 01:09 PM, Cong Wang wrote:
>>> This seems to be from the original thread of frame#3. It's grabbing
>>> wq lock here but the problem is that the lock will be released
>>> immediately, so bond_dev->name (the wq) can't be held by the time it
>>> reaches frame#3. How is this dependency chain completed? Is it
>>> somehow transitive through rtnl_mutex?
>> wq lock is held *after* cpu_add_remove_lock, lockdep also said this,
>> the process is trying to hold wq lock while having cpu_add_remove_lock.
>
> Yeah yeah, I'm just failing to see how the other direction is
> completed. ie. where does the kernel try to grab cpu_add_remove_lock
> *after* grabbing wq lock?
>
>>> Isn't there a circular dependency here? bonding_exit() calls
>>> destroy_workqueue() under rtnl_mutex but destroy_workqueue() should
>>> flush works which could be trying to grab rtnl_lock. Or am I
>>> completely misunderstanding locking here?
>> Sure, that is why I sent another patch for bonding. :)
>
> Ah... great. :-)
>
>> After this patch, another lockdep warning appears, it is exactly what
>> you expect.
>
> Hmmm... can you please try to see whether this circular locking
> warning involving wq->lockdep_map is reproducible w/ the bonding
> locking fixed? I still can't see where wq -> cpu_add_remove_lock
> dependency is created.
>
I thought this is obvious.
Here it is:
void destroy_workqueue(struct workqueue_struct *wq)
{
const struct cpumask *cpu_map = wq_cpu_map(wq);
int cpu;
cpu_maps_update_begin(); <----------------- Hold cpu_add_remove_lock here
spin_lock(&workqueue_lock);
list_del(&wq->list);
spin_unlock(&workqueue_lock);
for_each_cpu(cpu, cpu_map)
cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu)); <------ See below
cpu_maps_update_done(); <----------------- Release cpu_add_remove_lock here
...
static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
{
/*
* Our caller is either destroy_workqueue() or CPU_POST_DEAD,
* cpu_add_remove_lock protects cwq->thread.
*/
if (cwq->thread == NULL)
return;
lock_map_acquire(&cwq->wq->lockdep_map); <-------------- Lockdep complains here.
lock_map_release(&cwq->wq->lockdep_map);
...
Am I missing something??
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 4:28 ` Cong Wang
@ 2010-04-01 4:59 ` Tejun Heo
2010-04-01 5:20 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2010-04-01 4:59 UTC (permalink / raw)
To: Cong Wang; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Hello,
On 04/01/2010 01:28 PM, Cong Wang wrote:
>> Hmmm... can you please try to see whether this circular locking
>> warning involving wq->lockdep_map is reproducible w/ the bonding
>> locking fixed? I still can't see where wq -> cpu_add_remove_lock
>> dependency is created.
>>
>
> I thought this is obvious.
>
> Here it is:
>
> void destroy_workqueue(struct workqueue_struct *wq)
> {
> const struct cpumask *cpu_map = wq_cpu_map(wq);
> int cpu;
>
> cpu_maps_update_begin(); <----------------- Hold
> cpu_add_remove_lock here
> spin_lock(&workqueue_lock);
> list_del(&wq->list);
> spin_unlock(&workqueue_lock);
>
> for_each_cpu(cpu, cpu_map)
> cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu));
> <------ See below
> cpu_maps_update_done(); <----------------- Release
> cpu_add_remove_lock here
>
> ...
> static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
> {
> /*
> * Our caller is either destroy_workqueue() or CPU_POST_DEAD,
> * cpu_add_remove_lock protects cwq->thread.
> */
> if (cwq->thread == NULL)
> return;
>
> lock_map_acquire(&cwq->wq->lockdep_map); <-------------- Lockdep
> complains here.
> lock_map_release(&cwq->wq->lockdep_map);
> ...
Yeap, the above is cpu_add_remove_lock -> wq->lockdep_map dependency.
I can see that but I'm failing to see where the dependency the other
direction is created.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 4:59 ` Tejun Heo
@ 2010-04-01 5:20 ` Cong Wang
2010-04-01 6:05 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2010-04-01 5:20 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Tejun Heo wrote:
> Hello,
>
> On 04/01/2010 01:28 PM, Cong Wang wrote:
>>> Hmmm... can you please try to see whether this circular locking
>>> warning involving wq->lockdep_map is reproducible w/ the bonding
>>> locking fixed? I still can't see where wq -> cpu_add_remove_lock
>>> dependency is created.
>>>
>> I thought this is obvious.
>>
>> Here it is:
>>
>> void destroy_workqueue(struct workqueue_struct *wq)
>> {
>> const struct cpumask *cpu_map = wq_cpu_map(wq);
>> int cpu;
>>
>> cpu_maps_update_begin(); <----------------- Hold
>> cpu_add_remove_lock here
>> spin_lock(&workqueue_lock);
>> list_del(&wq->list);
>> spin_unlock(&workqueue_lock);
>>
>> for_each_cpu(cpu, cpu_map)
>> cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu));
>> <------ See below
>> cpu_maps_update_done(); <----------------- Release
>> cpu_add_remove_lock here
>>
>> ...
>> static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
>> {
>> /*
>> * Our caller is either destroy_workqueue() or CPU_POST_DEAD,
>> * cpu_add_remove_lock protects cwq->thread.
>> */
>> if (cwq->thread == NULL)
>> return;
>>
>> lock_map_acquire(&cwq->wq->lockdep_map); <-------------- Lockdep
>> complains here.
>> lock_map_release(&cwq->wq->lockdep_map);
>> ...
>
> Yeap, the above is cpu_add_remove_lock -> wq->lockdep_map dependency.
> I can see that but I'm failing to see where the dependency the other
> direction is created.
>
Hmm, it looks like I misunderstand lock_map_acquire()? From the changelog,
I thought it was added to complain its caller is holding a lock when invoking
it, thus cpu_add_remove_lock is not an exception.
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 5:20 ` Cong Wang
@ 2010-04-01 6:05 ` Cong Wang
2010-04-01 6:07 ` Cong Wang
2010-04-01 6:28 ` Tejun Heo
0 siblings, 2 replies; 14+ messages in thread
From: Cong Wang @ 2010-04-01 6:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Cong Wang wrote:
> Tejun Heo wrote:
>> Hello,
>>
>> On 04/01/2010 01:28 PM, Cong Wang wrote:
>>>> Hmmm... can you please try to see whether this circular locking
>>>> warning involving wq->lockdep_map is reproducible w/ the bonding
>>>> locking fixed? I still can't see where wq -> cpu_add_remove_lock
>>>> dependency is created.
>>>>
>>> I thought this is obvious.
>>>
>>> Here it is:
>>>
>>> void destroy_workqueue(struct workqueue_struct *wq)
>>> {
>>> const struct cpumask *cpu_map = wq_cpu_map(wq);
>>> int cpu;
>>>
>>> cpu_maps_update_begin(); <----------------- Hold
>>> cpu_add_remove_lock here
>>> spin_lock(&workqueue_lock);
>>> list_del(&wq->list);
>>> spin_unlock(&workqueue_lock);
>>>
>>> for_each_cpu(cpu, cpu_map)
>>> cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq,
>>> cpu)); <------ See below
>>> cpu_maps_update_done(); <----------------- Release
>>> cpu_add_remove_lock here
>>>
>>> ...
>>> static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
>>> {
>>> /*
>>> * Our caller is either destroy_workqueue() or CPU_POST_DEAD,
>>> * cpu_add_remove_lock protects cwq->thread.
>>> */
>>> if (cwq->thread == NULL)
>>> return;
>>>
>>> lock_map_acquire(&cwq->wq->lockdep_map); <-------------- Lockdep
>>> complains here.
>>> lock_map_release(&cwq->wq->lockdep_map);
>>> ...
>>
>> Yeap, the above is cpu_add_remove_lock -> wq->lockdep_map dependency.
>> I can see that but I'm failing to see where the dependency the other
>> direction is created.
>>
>
> Hmm, it looks like I misunderstand lock_map_acquire()? From the changelog,
> I thought it was added to complain its caller is holding a lock when
> invoking
> it, thus cpu_add_remove_lock is not an exception.
>
Oh, I see, wq->lockdep_map is acquired again in run_workqueue(), so I was wrong. :)
I think you and Oleg are right, the lockdep warning is not irrelevant.
Sorry for the noise, ignore this patch please.
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 6:05 ` Cong Wang
@ 2010-04-01 6:07 ` Cong Wang
2010-04-01 6:28 ` Tejun Heo
1 sibling, 0 replies; 14+ messages in thread
From: Cong Wang @ 2010-04-01 6:07 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Cong Wang wrote:
> Cong Wang wrote:
>> Tejun Heo wrote:
>>> Hello,
>>>
>>> On 04/01/2010 01:28 PM, Cong Wang wrote:
>>>>> Hmmm... can you please try to see whether this circular locking
>>>>> warning involving wq->lockdep_map is reproducible w/ the bonding
>>>>> locking fixed? I still can't see where wq -> cpu_add_remove_lock
>>>>> dependency is created.
>>>>>
>>>> I thought this is obvious.
>>>>
>>>> Here it is:
>>>>
>>>> void destroy_workqueue(struct workqueue_struct *wq)
>>>> {
>>>> const struct cpumask *cpu_map = wq_cpu_map(wq);
>>>> int cpu;
>>>>
>>>> cpu_maps_update_begin(); <----------------- Hold
>>>> cpu_add_remove_lock here
>>>> spin_lock(&workqueue_lock);
>>>> list_del(&wq->list);
>>>> spin_unlock(&workqueue_lock);
>>>>
>>>> for_each_cpu(cpu, cpu_map)
>>>> cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq,
>>>> cpu)); <------ See below
>>>> cpu_maps_update_done(); <----------------- Release
>>>> cpu_add_remove_lock here
>>>>
>>>> ...
>>>> static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq)
>>>> {
>>>> /*
>>>> * Our caller is either destroy_workqueue() or CPU_POST_DEAD,
>>>> * cpu_add_remove_lock protects cwq->thread.
>>>> */
>>>> if (cwq->thread == NULL)
>>>> return;
>>>>
>>>> lock_map_acquire(&cwq->wq->lockdep_map); <--------------
>>>> Lockdep
>>>> complains here.
>>>> lock_map_release(&cwq->wq->lockdep_map);
>>>> ...
>>>
>>> Yeap, the above is cpu_add_remove_lock -> wq->lockdep_map dependency.
>>> I can see that but I'm failing to see where the dependency the other
>>> direction is created.
>>>
>>
>> Hmm, it looks like I misunderstand lock_map_acquire()? From the
>> changelog,
>> I thought it was added to complain its caller is holding a lock when
>> invoking
>> it, thus cpu_add_remove_lock is not an exception.
>>
>
> Oh, I see, wq->lockdep_map is acquired again in run_workqueue(), so I
> was wrong. :)
> I think you and Oleg are right, the lockdep warning is not irrelevant.
>
Oops, typo, I meant "is irrelevant." ;)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 6:05 ` Cong Wang
2010-04-01 6:07 ` Cong Wang
@ 2010-04-01 6:28 ` Tejun Heo
1 sibling, 0 replies; 14+ messages in thread
From: Tejun Heo @ 2010-04-01 6:28 UTC (permalink / raw)
To: Cong Wang; +Cc: Oleg Nesterov, linux-kernel, Rusty Russell, akpm, Ingo Molnar
Hello,
On 04/01/2010 03:05 PM, Cong Wang wrote:
>> Hmm, it looks like I misunderstand lock_map_acquire()? From the
>> changelog, I thought it was added to complain its caller is holding
>> a lock when invoking it, thus cpu_add_remove_lock is not an
>> exception.
Oh, that just tells the code is trying to grab a pseudo lock. It's
not really a lock but to lockdep it looks like one and lockdep can use
it to compute problem cases.
> Oh, I see, wq->lockdep_map is acquired again in run_workqueue(), so
> I was wrong. :) I think you and Oleg are right, the lockdep warning
> is not irrelevant.
Yeah, I think the circular dependency you reported on wq->lockdep_map
is completed only through dependency through rtnl_mutex. If you fix
rtnl_mutex locking, it should go away too.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 2:45 ` Cong Wang
2010-04-01 3:56 ` Tejun Heo
@ 2010-04-01 16:36 ` Oleg Nesterov
2010-04-02 5:00 ` Cong Wang
1 sibling, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2010-04-01 16:36 UTC (permalink / raw)
To: Cong Wang; +Cc: linux-kernel, Tejun Heo, Rusty Russell, akpm, Ingo Molnar
On 04/01, Cong Wang wrote:
>
>> I must have missed something, but it seems to me this patch tries to
>> supress the valid warning.
>>
>> Could you please clarify?
>
> Sure, below is the whole warning. Please teach me how this is valid.
Oh, I can never understand the output from lockdep, it is much more
clever than me ;)
But at first glance,
> Mar 31 16:15:02 dhcp-66-70-5 kernel: -> #2 (rtnl_mutex){+.+.+.}:
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffffa0836779>] bond_mii_monitor+0x39f/0x74b [bonding]
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108654f>] worker_thread+0x2da/0x46c
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108b1ea>] kthread+0xdd/0xec
> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
OK, so work->func() takes rtnl_mutex.
This means it is not safe to do flush_workqueue() or destroy_workqueue()
under rtnl_lock(). This is known fact.
> Mar 31 16:15:03 dhcp-66-70-5 kernel: -> #0 ((bond_dev->name)){+.+...}:
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a6696>] validate_chain+0xaee/0x1540
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
However, rtnl_link_unregister() takes rtnl_mutex and then bond_uninit()
does cleanup_workqueue_thread().
So, looks like this warning is valid, this path can deadlock if
destroy_workqueue() is called when bond->mii_work is queued.
Lockdep decided to blaim cpu_add_remove_lock in this chain.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Patch] workqueue: move lockdep annotations up to destroy_workqueue()
2010-04-01 16:36 ` Oleg Nesterov
@ 2010-04-02 5:00 ` Cong Wang
0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2010-04-02 5:00 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Tejun Heo, Rusty Russell, akpm, Ingo Molnar
Oleg Nesterov wrote:
> On 04/01, Cong Wang wrote:
>>> I must have missed something, but it seems to me this patch tries to
>>> supress the valid warning.
>>>
>>> Could you please clarify?
>> Sure, below is the whole warning. Please teach me how this is valid.
>
> Oh, I can never understand the output from lockdep, it is much more
> clever than me ;)
>
> But at first glance,
>
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: -> #2 (rtnl_mutex){+.+.+.}:
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a6bc1>] validate_chain+0x1019/0x1540
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff815523f8>] mutex_lock_nested+0x64/0x4e9
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8147af16>] rtnl_lock+0x1e/0x27
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffffa0836779>] bond_mii_monitor+0x39f/0x74b [bonding]
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108654f>] worker_thread+0x2da/0x46c
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff8108b1ea>] kthread+0xdd/0xec
>> Mar 31 16:15:02 dhcp-66-70-5 kernel: [<ffffffff81004894>] kernel_thread_helper+0x4/0x10
>
> OK, so work->func() takes rtnl_mutex.
>
> This means it is not safe to do flush_workqueue() or destroy_workqueue()
> under rtnl_lock(). This is known fact.
>
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: -> #0 ((bond_dev->name)){+.+...}:
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a6696>] validate_chain+0xaee/0x1540
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810a7e75>] __lock_acquire+0xd8d/0xe55
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff810aa3a4>] lock_acquire+0x160/0x1af
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085278>] cleanup_workqueue_thread+0x59/0x10b
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81085428>] destroy_workqueue+0x9c/0x107
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffffa0839d32>] bond_uninit+0x524/0x58a [bonding]
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8146967b>] rollback_registered_many+0x205/0x2e3
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff81469783>] unregister_netdevice_many+0x2a/0x75
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147ada3>] __rtnl_kill_links+0x8b/0x9d
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147adea>] __rtnl_link_unregister+0x35/0x72
>> Mar 31 16:15:03 dhcp-66-70-5 kernel: [<ffffffff8147b293>] rtnl_link_unregister+0x2c/0x43
>
> However, rtnl_link_unregister() takes rtnl_mutex and then bond_uninit()
> does cleanup_workqueue_thread().
>
> So, looks like this warning is valid, this path can deadlock if
> destroy_workqueue() is called when bond->mii_work is queued.
Yeah, this is right.
>
>
> Lockdep decided to blaim cpu_add_remove_lock in this chain.
>
Yes, this is what makes me confused. ;)
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-04-02 4:57 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-31 10:51 [Patch] workqueue: move lockdep annotations up to destroy_workqueue() Amerigo Wang
2010-03-31 11:25 ` Oleg Nesterov
2010-04-01 2:45 ` Cong Wang
2010-04-01 3:56 ` Tejun Heo
2010-04-01 4:09 ` Cong Wang
2010-04-01 4:14 ` Tejun Heo
2010-04-01 4:28 ` Cong Wang
2010-04-01 4:59 ` Tejun Heo
2010-04-01 5:20 ` Cong Wang
2010-04-01 6:05 ` Cong Wang
2010-04-01 6:07 ` Cong Wang
2010-04-01 6:28 ` Tejun Heo
2010-04-01 16:36 ` Oleg Nesterov
2010-04-02 5:00 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).