From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757872AbcJQNpE (ORCPT ); Mon, 17 Oct 2016 09:45:04 -0400 Received: from mail-pf0-f177.google.com ([209.85.192.177]:32811 "EHLO mail-pf0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757193AbcJQNou (ORCPT ); Mon, 17 Oct 2016 09:44:50 -0400 Date: Mon, 17 Oct 2016 21:45:01 +0800 From: Boqun Feng To: Peter Zijlstra Cc: Will Deacon , Linus Torvalds , Waiman Long , Jason Low , Ding Tianhong , Thomas Gleixner , Ingo Molnar , Imre Deak , Linux Kernel Mailing List , Davidlohr Bueso , Tim Chen , Terry Rudd , "Paul E. McKenney" , Jason Low , Chris Wilson , Daniel Vetter Subject: Re: [PATCH -v4 6/8] locking/mutex: Restructure wait loop Message-ID: <20161017134501.GA27939@tardis.cn.ibm.com> References: <20161007145243.361481786@infradead.org> <20161007150211.271490994@infradead.org> <20161013151720.GB13138@arm.com> <20161017104449.GO3117@twins.programming.kicks-ass.net> <20161017132408.GF3157@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="y0ulUmNC+osPPQO6" Content-Disposition: inline In-Reply-To: <20161017132408.GF3157@twins.programming.kicks-ass.net> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --y0ulUmNC+osPPQO6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 17, 2016 at 03:24:08PM +0200, Peter Zijlstra wrote: > On Mon, Oct 17, 2016 at 12:44:49PM +0200, Peter Zijlstra wrote: > > On Thu, Oct 13, 2016 at 04:17:21PM +0100, Will Deacon wrote: > > > Hi Peter, > > >=20 > > > I'm struggling to get my head around the handoff code after this chan= ge... > > >=20 > > > On Fri, Oct 07, 2016 at 04:52:49PM +0200, Peter Zijlstra wrote: > > > > --- a/kernel/locking/mutex.c > > > > +++ b/kernel/locking/mutex.c > > > > @@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock, > > > > =20 > > > > lock_contended(&lock->dep_map, ip); > > > > =20 > > > > + set_task_state(task, state); > > > > for (;;) { > > > > + /* > > > > + * Once we hold wait_lock, we're serialized against > > > > + * mutex_unlock() handing the lock off to us, do a trylock > > > > + * before testing the error conditions to make sure we pick up > > > > + * the handoff. > > > > + */ > > > > if (__mutex_trylock(lock, first)) > > > > - break; > > > > + goto acquired; > > > > =20 > > > > /* > > > > - * got a signal? (This code gets eliminated in the > > > > - * TASK_UNINTERRUPTIBLE case.) > > > > + * Check for signals and wound conditions while holding > > > > + * wait_lock. This ensures the lock cancellation is ordered > > > > + * against mutex_unlock() and wake-ups do not go missing. > > > > */ > > > > if (unlikely(signal_pending_state(state, task))) { > > > > ret =3D -EINTR; > > > > @@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock, > > > > goto err; > > > > } > > > > =20 > > > > - __set_task_state(task, state); > > > > spin_unlock_mutex(&lock->wait_lock, flags); > > > > schedule_preempt_disabled(); > > > > - spin_lock_mutex(&lock->wait_lock, flags); > > > > =20 > > > > if (!first && __mutex_waiter_is_first(lock, &waiter)) { > > > > first =3D true; > > > > __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF); > > > > } > > > > + > > > > + set_task_state(task, state); > > >=20 > > > With this change, we no longer hold the lock wit_hen we set the task > > > state, and it's ordered strictly *after* setting the HANDOFF flag. > > > Doesn't that mean that the unlock code can see the HANDOFF flag, issue > > > the wakeup, but then we come in and overwrite the task state? > > >=20 > > > I'm struggling to work out whether that's an issue, but it certainly > > > feels odd and is a change from the previous behaviour. > >=20 > > Right, so I think the code is fine, since in that case the > > __mutex_trylock() must see the handoff and we'll break the loop and > > (re)set the state to RUNNING. > >=20 > > But you're right in that its slightly odd. I'll reorder them and put the > > set_task_state() above the !first thing. >=20 >=20 > Humm,.. we might actually rely on this order, since the MB implied by > set_task_state() is the only thing that separates the store of > __mutex_set_flag() from the load of __mutex_trylock(), and those should > be ordered I think. >=20 But __mutex_set_flag() and __mutex_trylock() actually touch the same atomic word? So we don't need extra things to order them? Regards, Boqun > Argh, completely messed up my brain. I'll not touch it and think on this > again tomorrow. --y0ulUmNC+osPPQO6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYBNXZAAoJEEl56MO1B/q4YxgH/33f4Ldqnwql99jGZSNLkvIg yFAUvZEX4sg6MCYGn70qzsj6e4TMKVvA/1qHwm349F1PzOnjF/iG5VqR5ZwmESKE FZ2nWUjANs8JCYDOCh120TbgQmOZECGY3z07CepLYw1QgYD6xzHviKU5V1e9rP0V ISdD/hlcYpowNW+r0XUrVMclfKWSy0IGluwlqylSQztTF8TCspEwHwE3XEAmTBzA VohvJ1LvIaAggc2l1+8PZYNRA+Y/5Hgx7nPgNL/CKWl5E1+MlnfJbj/AkGLHkwnx xa/KeOMhJFNBMp2aRM2fn3pNfW4ec/cJ4oNC1LVqwrLVohV6h3KkNMpRx8yfS/c= =zK6e -----END PGP SIGNATURE----- --y0ulUmNC+osPPQO6--