Messages in this thread | | | Date | Mon, 9 Oct 2017 11:24:54 +0800 | From | Boqun Feng <> | Subject | Re: [RFC] workqueue: Fix irq inversion deadlock in manage_workers() |
| |
On Sun, Oct 08, 2017 at 07:03:47PM +0000, Tejun Heo wrote: > Hello, Boqun. >
Hi Tejun,
> On Sun, Oct 08, 2017 at 05:02:23PM +0800, Boqun Feng wrote: > > Josef reported a HARDIRQ-safe -> HARDIRQ-unsafe lock order detected by > > lockdep: > > > > | [ 1270.472259] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected > > | [ 1270.472783] 4.14.0-rc1-xfstests-12888-g76833e8 #110 Not tainted > > | [ 1270.473240] ----------------------------------------------------- > > | [ 1270.473710] kworker/u5:2/5157 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire: > > | [ 1270.474239] (&(&lock->wait_lock)->rlock){+.+.}, at: [<ffffffff8da253d2>] __mutex_unlock_slowpath+0xa2/0x280 > > | [ 1270.474994] > > | [ 1270.474994] and this task is already holding: > > | [ 1270.475440] (&pool->lock/1){-.-.}, at: [<ffffffff8d2992f6>] worker_thread+0x366/0x3c0 > > | [ 1270.476046] which would create a new lock dependency: > > | [ 1270.476436] (&pool->lock/1){-.-.} -> (&(&lock->wait_lock)->rlock){+.+.} > > | [ 1270.476949] > > | [ 1270.476949] but this new dependency connects a HARDIRQ-irq-safe lock: > > | [ 1270.477553] (&pool->lock/1){-.-.} > > ... > > | [ 1270.488900] to a HARDIRQ-irq-unsafe lock: > > | [ 1270.489327] (&(&lock->wait_lock)->rlock){+.+.} > > ... > > | [ 1270.494735] Possible interrupt unsafe locking scenario: > > | [ 1270.494735] > > | [ 1270.495250] CPU0 CPU1 > > | [ 1270.495600] ---- ---- > > | [ 1270.495947] lock(&(&lock->wait_lock)->rlock); > > | [ 1270.496295] local_irq_disable(); > > | [ 1270.496753] lock(&pool->lock/1); > > | [ 1270.497205] lock(&(&lock->wait_lock)->rlock); > > | [ 1270.497744] <Interrupt> > > | [ 1270.497948] lock(&pool->lock/1); > > > > , which will cause a irq inversion deadlock if the above lock scenario > > happens. > > > > The root cause of this safe -> unsafe lock order is the > > mutex_unlock(pool::manager_arb) in manage_workers() with pool::lock > > So, if I'm not mistaken, this is a regression caused by b9c16a0e1f73 > ("locking/mutex: Fix lockdep_assert_held() fail") which seems to > replace irqsave operations inside mutex to unconditional irq ones. >
Hmm.. I don't that commit replaced _irqsave with _irq ones. That commit simply exposed mutex::wait_lock to lockdep(especially for the irq inversion deadlock checking).
Even before that commit, the !DEBUG version of spin_{,un}lock_mutex() wouldn't disable interrupts. So this is not a regression, it's a real deadlock potential, which had not been detected until commit b9c16a0e1f73.
Regards, Boqun
> I suppose it's a requirement we can add but that needs to be an > explicit change with backing rationales. > > Thanks. > > -- > tejun [unhandled content-type:application/pgp-signature] | |