* [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock
@ 2023-02-28 22:49 Brennan Lamoreaux (VMware)
2023-02-28 23:03 ` Srivatsa S. Bhat
2023-03-13 9:36 ` Sebastian Andrzej Siewior
0 siblings, 2 replies; 4+ messages in thread
From: Brennan Lamoreaux (VMware) @ 2023-02-28 22:49 UTC (permalink / raw)
To: linux-rt-users, linux-kernel
Cc: blamoreaux, frederic.martinsons, srivatsa, vsirnapalli,
amakhalov, keerthanak, ankitja, bordoloih, srivatsab,
Brennan Lamoreaux (VMware),
Daniel Wagner, Sebastian Andrzej Siewior, Tejun Heo
Upstream commit d8bb65ab70f7 ("workqueue: Use rcuwait for wq_manager_wait")
replaced the waitqueue with rcuwait in the workqueue code. This change
involved removing the acquisition of pool->lock in put_unbound_pool(),
as it also adds the function wq_manager_inactive() which acquires this same
lock and is called one line later as a parameter to rcu_wait_event().
However, the backport of this commit in the PREEMPT_RT patchset
4.19.255-rt114 (patch 347) missed the removal of the acquisition of
pool->lock in put_unbound_pool(). This leads to a deadlock due to
recursive locking of pool->lock, as shown below in lockdep:
[ 252.083713] WARNING: possible recursive locking detected
[ 252.083718] 4.19.269-3.ph3-rt #1-photon Not tainted
[ 252.083721] --------------------------------------------
[ 252.083733] kworker/2:0/33 is trying to acquire lock:
[ 252.083747] 000000000b7b1ceb (&pool->lock/1){....}, at:
put_unbound_pool+0x10d/0x260
[ 252.083857]
but task is already holding lock:
[ 252.083860] 000000000b7b1ceb (&pool->lock/1){....}, at:
put_unbound_pool+0xbd/0x260
[ 252.083876]
other info that might help us debug this:
[ 252.083897] Possible unsafe locking scenario:
[ 252.083900] CPU0
[ 252.083903] ----
[ 252.083904] lock(&pool->lock/1);
[ 252.083911] lock(&pool->lock/1);
[ 252.083919]
*** DEADLOCK ***
[ 252.083921] May be due to missing lock nesting notation
Fix this deadlock by removing the pool->lock acquisition in
put_unbound_pool().
Signed-off-by: Brennan Lamoreaux (VMware) <brennanlamoreaux@gmail.com>
Cc: Daniel Wagner <wagi@monom.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Tejun Heo <tj@kernel.org>
---
kernel/workqueue.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index a9f3cc02bdc1..55ebdd56a5de 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3394,7 +3394,6 @@ static void put_unbound_pool(struct worker_pool *pool)
* Because of how wq_manager_inactive() works, we will hold the
* spinlock after a successful wait.
*/
- raw_spin_lock_irq(&pool->lock);
rcuwait_wait_event(&manager_wait, wq_manager_inactive(pool),
TASK_UNINTERRUPTIBLE);
pool->flags |= POOL_MANAGER_ACTIVE;
--
2.35.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock
2023-02-28 22:49 [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock Brennan Lamoreaux (VMware)
@ 2023-02-28 23:03 ` Srivatsa S. Bhat
2023-03-13 9:36 ` Sebastian Andrzej Siewior
1 sibling, 0 replies; 4+ messages in thread
From: Srivatsa S. Bhat @ 2023-02-28 23:03 UTC (permalink / raw)
To: Brennan Lamoreaux (VMware), linux-rt-users, linux-kernel
Cc: blamoreaux, frederic.martinsons, vsirnapalli, amakhalov,
keerthanak, ankitja, bordoloih, srivatsab, Daniel Wagner,
Sebastian Andrzej Siewior, Tejun Heo
On 2/28/23 2:49 PM, Brennan Lamoreaux (VMware) wrote:
> Upstream commit d8bb65ab70f7 ("workqueue: Use rcuwait for wq_manager_wait")
> replaced the waitqueue with rcuwait in the workqueue code. This change
> involved removing the acquisition of pool->lock in put_unbound_pool(),
> as it also adds the function wq_manager_inactive() which acquires this same
> lock and is called one line later as a parameter to rcu_wait_event().
>
> However, the backport of this commit in the PREEMPT_RT patchset
> 4.19.255-rt114 (patch 347) missed the removal of the acquisition of
> pool->lock in put_unbound_pool(). This leads to a deadlock due to
> recursive locking of pool->lock, as shown below in lockdep:
>
> [ 252.083713] WARNING: possible recursive locking detected
> [ 252.083718] 4.19.269-3.ph3-rt #1-photon Not tainted
> [ 252.083721] --------------------------------------------
> [ 252.083733] kworker/2:0/33 is trying to acquire lock:
> [ 252.083747] 000000000b7b1ceb (&pool->lock/1){....}, at:
> put_unbound_pool+0x10d/0x260
>
> [ 252.083857]
> but task is already holding lock:
> [ 252.083860] 000000000b7b1ceb (&pool->lock/1){....}, at:
> put_unbound_pool+0xbd/0x260
>
> [ 252.083876]
> other info that might help us debug this:
> [ 252.083897] Possible unsafe locking scenario:
>
> [ 252.083900] CPU0
> [ 252.083903] ----
> [ 252.083904] lock(&pool->lock/1);
> [ 252.083911] lock(&pool->lock/1);
> [ 252.083919]
> *** DEADLOCK ***
>
> [ 252.083921] May be due to missing lock nesting notation
>
> Fix this deadlock by removing the pool->lock acquisition in
> put_unbound_pool().
>
> Signed-off-by: Brennan Lamoreaux (VMware) <brennanlamoreaux@gmail.com>
> Cc: Daniel Wagner <wagi@monom.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Tejun Heo <tj@kernel.org>
Reviewed-by: Srivatsa S. Bhat (VMware) <srivatsa@csail.mit.edu>
> ---
> kernel/workqueue.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index a9f3cc02bdc1..55ebdd56a5de 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3394,7 +3394,6 @@ static void put_unbound_pool(struct worker_pool *pool)
> * Because of how wq_manager_inactive() works, we will hold the
> * spinlock after a successful wait.
> */
> - raw_spin_lock_irq(&pool->lock);
> rcuwait_wait_event(&manager_wait, wq_manager_inactive(pool),
> TASK_UNINTERRUPTIBLE);
> pool->flags |= POOL_MANAGER_ACTIVE;
>
Regards,
Srivatsa
VMware Photon OS
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock
2023-02-28 22:49 [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock Brennan Lamoreaux (VMware)
2023-02-28 23:03 ` Srivatsa S. Bhat
@ 2023-03-13 9:36 ` Sebastian Andrzej Siewior
2023-03-16 7:08 ` Daniel Wagner
1 sibling, 1 reply; 4+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-03-13 9:36 UTC (permalink / raw)
To: Brennan Lamoreaux (VMware), Daniel Wagner
Cc: linux-rt-users, linux-kernel, blamoreaux, frederic.martinsons,
srivatsa, vsirnapalli, amakhalov, keerthanak, ankitja, bordoloih,
srivatsab, Tejun Heo
On 2023-02-28 14:49:38 [-0800], Brennan Lamoreaux (VMware) wrote:
> Upstream commit d8bb65ab70f7 ("workqueue: Use rcuwait for wq_manager_wait")
> replaced the waitqueue with rcuwait in the workqueue code. This change
> involved removing the acquisition of pool->lock in put_unbound_pool(),
> as it also adds the function wq_manager_inactive() which acquires this same
> lock and is called one line later as a parameter to rcu_wait_event().
Daniel, I double checked and this patch is correct - the backport was
faulty. Could you please pick it up and release an update?
Sebastian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock
2023-03-13 9:36 ` Sebastian Andrzej Siewior
@ 2023-03-16 7:08 ` Daniel Wagner
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Wagner @ 2023-03-16 7:08 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Brennan Lamoreaux (VMware),
linux-rt-users, linux-kernel, blamoreaux, frederic.martinsons,
srivatsa, vsirnapalli, amakhalov, keerthanak, ankitja, bordoloih,
srivatsab, Tejun Heo
On Mon, Mar 13, 2023 at 10:36:41AM +0100, Sebastian Andrzej Siewior wrote:
> On 2023-02-28 14:49:38 [-0800], Brennan Lamoreaux (VMware) wrote:
> > Upstream commit d8bb65ab70f7 ("workqueue: Use rcuwait for wq_manager_wait")
> > replaced the waitqueue with rcuwait in the workqueue code. This change
> > involved removing the acquisition of pool->lock in put_unbound_pool(),
> > as it also adds the function wq_manager_inactive() which acquires this same
> > lock and is called one line later as a parameter to rcu_wait_event().
>
> Daniel, I double checked and this patch is correct - the backport was
> faulty. Could you please pick it up and release an update?
Sure. I've updated the v4.19-rt branch and added this patch. Running local tests
now.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-16 7:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 22:49 [PATCH 4.19-rt] workqueue: Fix deadlock due to recursive locking of pool->lock Brennan Lamoreaux (VMware)
2023-02-28 23:03 ` Srivatsa S. Bhat
2023-03-13 9:36 ` Sebastian Andrzej Siewior
2023-03-16 7:08 ` Daniel Wagner
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).