From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754256AbdJIOUS (ORCPT ); Mon, 9 Oct 2017 10:20:18 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36881 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751636AbdJIOUQ (ORCPT ); Mon, 9 Oct 2017 10:20:16 -0400 X-Google-Smtp-Source: AOwi7QCUxHCt+UNIa3weWKDiXkJXU7b8p+9xjgp+iGVjB4hW9AVvfDZs7GzhpBZKO1zDEfHKjCtcKA== X-ME-Sender: X-Sasl-enc: RFIY6W5o3P1lOzU7fH9liveSR/tjUTcDS7bWtuXET1oJ 1507558810 Date: Mon, 9 Oct 2017 22:21:17 +0800 From: Boqun Feng To: Tejun Heo Cc: linux-kernel@vger.kernel.org, Josef Bacik , Peter Zijlstra , Lai Jiangshan Subject: Re: [PATCH workqueue/for-4.14-fixes] workqueue: replace pool->manager_arb mutex with a flag Message-ID: <20171009142117.trbabd6m656ljf7w@tardis> References: <20171008090225.27034-1-boqun.feng@gmail.com> <20171009132104.GZ3301751@devbig577.frc2.facebook.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hk6lj6yaoe4figsc" Content-Disposition: inline In-Reply-To: <20171009132104.GZ3301751@devbig577.frc2.facebook.com> User-Agent: NeoMutt/20170912 (1.9.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --hk6lj6yaoe4figsc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 09, 2017 at 01:21:04PM +0000, Tejun Heo wrote: > Josef reported a HARDIRQ-safe -> HARDIRQ-unsafe lock order detected by > lockdep: >=20 > [ 1270.472259] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detect= ed > [ 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 ac= quire: > [ 1270.474239] (&(&lock->wait_lock)->rlock){+.+.}, at: [] __mutex_unlock_slowpath+0xa2/0x280 > [ 1270.474994] > [ 1270.474994] and this task is already holding: > [ 1270.475440] (&pool->lock/1){-.-.}, at: [] worker_t= hread+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] > [ 1270.497948] lock(&pool->lock/1); >=20 > , which will cause a irq inversion deadlock if the above lock scenario > happens. >=20 > The root cause of this safe -> unsafe lock order is the > mutex_unlock(pool->manager_arb) in manage_workers() with pool->lock > held. >=20 > Unlocking mutex while holding an irq spinlock was never safe and this > problem has been around forever but it never got noticed because the > only time the mutex is usually trylocked while holding irqlock making > actual failures very unlikely and lockdep annotation missed the > condition until the recent b9c16a0e1f73 ("locking/mutex: Fix > lockdep_assert_held() fail"). >=20 > Using mutex for pool->manager_arb has always been a bit of stretch. > It primarily is an mechanism to arbitrate managership between workers > which can easily be done with a pool flag. The only reason it became > a mutex is that pool destruction path wants to exclude parallel > managing operations. >=20 > This patch replaces the mutex with a new pool flag POOL_MANAGER_ACTIVE > and make the destruction path wait for the current manager on a wait > queue. >=20 > Signed-off-by: Tejun Heo > Reported-by: Josef Bacik > Cc: Peter Zijlstra > Cc: Boqun Feng > Cc: stable@vger.kernel.org > --- > Hello, >=20 > Boqun, thanks for the patch and explanation and I shamelessly lifted > parts of your patch description. I took an alternative approach That's find ;-) > because there's no reason to make this any more complex when the issue > is essentially caused by abusing mutex. >=20 Agreed. > The patch seems to work fine but, Lai, can you please review the > patch? >=20 > Thanks. >=20 > kernel/workqueue.c | 39 ++++++++++++++++++--------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) >=20 > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index 64d0edf..8739b6de 100644 > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -68,6 +68,7 @@ enum { > * attach_mutex to avoid changing binding state while > * worker_attach_to_pool() is in progress. > */ > + POOL_MANAGER_ACTIVE =3D 1 << 0, /* being managed */ > POOL_DISASSOCIATED =3D 1 << 2, /* cpu can't serve workers */ > =20 > /* worker flags */ > @@ -165,7 +166,6 @@ struct worker_pool { > /* L: hash of busy workers */ > =20 > /* see manage_workers() for details on the two manager mutexes */ > - struct mutex manager_arb; /* manager arbitration */ > struct worker *manager; /* L: purely informational */ > struct mutex attach_mutex; /* attach/detach exclusion */ > struct list_head workers; /* A: attached workers */ > @@ -299,6 +299,7 @@ static struct workqueue_attrs *wq_update_unbound_numa= _attrs_buf; > =20 > static DEFINE_MUTEX(wq_pool_mutex); /* protects pools and workqueues lis= t */ > static DEFINE_SPINLOCK(wq_mayday_lock); /* protects wq->maydays list */ > +static DECLARE_WAIT_QUEUE_HEAD(wq_manager_wait); /* wait for manager to = go away */ I think this wait_queue_head better be a per-pool one rather than shared among pools? > =20 > static LIST_HEAD(workqueues); /* PR: list of all workqueues */ > static bool workqueue_freezing; /* PL: have wqs started freezing? */ > @@ -801,7 +802,7 @@ static bool need_to_create_worker(struct worker_pool = *pool) > /* Do we have too many workers and should some go away? */ > static bool too_many_workers(struct worker_pool *pool) > { > - bool managing =3D mutex_is_locked(&pool->manager_arb); > + bool managing =3D pool->flags & POOL_MANAGER_ACTIVE; > int nr_idle =3D pool->nr_idle + managing; /* manager is considered idle= */ > int nr_busy =3D pool->nr_workers - nr_idle; > =20 > @@ -1980,24 +1981,17 @@ static bool manage_workers(struct worker *worker) > { > struct worker_pool *pool =3D worker->pool; > =20 > - /* > - * Anyone who successfully grabs manager_arb wins the arbitration > - * and becomes the manager. mutex_trylock() on pool->manager_arb > - * failure while holding pool->lock reliably indicates that someone > - * else is managing the pool and the worker which failed trylock > - * can proceed to executing work items. This means that anyone > - * grabbing manager_arb is responsible for actually performing > - * manager duties. If manager_arb is grabbed and released without > - * actual management, the pool may stall indefinitely. > - */ > - if (!mutex_trylock(&pool->manager_arb)) > + if (pool->flags & POOL_MANAGER_ACTIVE) > return false; > + > + pool->flags |=3D POOL_MANAGER_ACTIVE; > pool->manager =3D worker; > =20 > maybe_create_worker(pool); > =20 > pool->manager =3D NULL; > - mutex_unlock(&pool->manager_arb); > + pool->flags &=3D ~POOL_MANAGER_ACTIVE; > + wake_up(&wq_manager_wait); > return true; > } > =20 > @@ -3248,7 +3242,6 @@ static int init_worker_pool(struct worker_pool *poo= l) > setup_timer(&pool->mayday_timer, pool_mayday_timeout, > (unsigned long)pool); > =20 > - mutex_init(&pool->manager_arb); > mutex_init(&pool->attach_mutex); > INIT_LIST_HEAD(&pool->workers); > =20 > @@ -3318,13 +3311,14 @@ static void put_unbound_pool(struct worker_pool *= pool) > hash_del(&pool->hash_node); > =20 > /* > - * Become the manager and destroy all workers. Grabbing > - * manager_arb prevents @pool's workers from blocking on > - * attach_mutex. > + * Become the manager and destroy all workers. Becoming manager > + * prevents @pool's workers from blocking on attach_mutex. > */ > - mutex_lock(&pool->manager_arb); > - > spin_lock_irq(&pool->lock); > + wait_event_lock_irq(wq_manager_wait, > + !(pool->flags & POOL_MANAGER_ACTIVE), pool->lock); > + pool->flags |=3D POOL_MANAGER_ACTIVE; > + > while ((worker =3D first_idle_worker(pool))) > destroy_worker(worker); > WARN_ON(pool->nr_workers || pool->nr_idle); > @@ -3338,7 +3332,10 @@ static void put_unbound_pool(struct worker_pool *p= ool) > if (pool->detach_completion) > wait_for_completion(pool->detach_completion); > =20 > - mutex_unlock(&pool->manager_arb); > + spin_lock_irq(&pool->lock); > + pool->flags &=3D ~POOL_MANAGER_ACTIVE; > + wake_up(&wq_manager_wait); > + spin_unlock_irq(&pool->lock); > =20 Is the above code necesarry? IIUC, we are going to free the pool entirely, so whether manager is active is pointless here and no one is waiting for the ->flags of *this* pool to be !POOL_MANAGER_ACTIVE. Am I missing something subtle here? Regards, Boqun > /* shut down the timers */ > del_timer_sync(&pool->idle_timer); --hk6lj6yaoe4figsc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlnbhdoACgkQSXnow7UH +rgC8gf+O60prHDVG5n3WpnbL5yIXKV3Y6D0viJDkI4ldmzrRapd+l0j15qgiTV1 YBFqClN8+XanHs67QmvgDPKd2QAXUwX22qpNyTTw+Smnz6GW/FvbJzKZzBrUSwsa tu2ZFozpR+aRcJ8yspB2Ofblhuf9SRA/Ku6c+5CUrsGRuWejzdKC6fyb4VjIuphb L7UEkIVCYURwZLOLW883atNzQOB8Nwkxjambr0B+FPgvUuwOoVdj3H0AHFcL8U6u gD6uqReenqlu2jxheKQRwj3XwatuhKxlz36Xb46vWNaaRP70V7Nb3+k7ToDL0B57 Lqjrbs0xs7eLyRePT1jjZlx4HgU6DA== =p/os -----END PGP SIGNATURE----- --hk6lj6yaoe4figsc--