From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756321AbdDEXwp (ORCPT ); Wed, 5 Apr 2017 19:52:45 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:40057 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756111AbdDEXwg (ORCPT ); Wed, 5 Apr 2017 19:52:36 -0400 Date: Wed, 5 Apr 2017 16:52:25 -0700 From: Darren Hart To: Peter Zijlstra Cc: tglx@linutronix.de, mingo@kernel.org, juri.lelli@arm.com, rostedt@goodmis.org, xlpang@redhat.com, bigeasy@linutronix.de, linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com, jdesfossez@efficios.com, bristot@redhat.com Subject: Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock Message-ID: <20170405235225.GD13494@fury> References: <20170322103547.756091212@infradead.org> <20170322104151.900002056@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170322104151.900002056@infradead.org> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 22, 2017 at 11:35:55AM +0100, Peter Zijlstra wrote: > There's a number of 'interesting' problems, all caused by holding > hb->lock while doing the rt_mutex_unlock() equivalient. > > Notably: > > - a PI inversion on hb->lock; and, > > - a DL crash because of pointer instability. A DL crash? What is this? Can you elaborate a bit? > > Because of all the previous patches that: > > - allow us to do rt_mutex_futex_unlock() without dropping wait_lock; > which in turn allows us to rely on wait_lock atomicy. > > - changed locking rules to cover {uval,pi_state} with wait_lock. > > - simplified the waiter conundrum. > > We can now quite simply pull rt_mutex_futex_unlock() out from under > hb->lock, a pi_state reference and wait_lock are sufficient. OK, owe. I think I've traced most of this through. I have a few gray areas still, and will continue through the series to see if that addresses them. A few thoughts as they occurred to me below. > > Signed-off-by: Peter Zijlstra (Intel) > --- > kernel/futex.c | 154 +++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 100 insertions(+), 54 deletions(-) > > --- a/kernel/futex.c > +++ b/kernel/futex.c ... > @@ -1380,48 +1387,40 @@ static void mark_wake_futex(struct wake_ > smp_store_release(&q->lock_ptr, NULL); > } > > -static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter, > - struct futex_hash_bucket *hb) > +/* > + * Caller must hold a reference on @pi_state. > + */ > +static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state) > { > - struct task_struct *new_owner; > - struct futex_pi_state *pi_state = top_waiter->pi_state; > u32 uninitialized_var(curval), newval; > + struct task_struct *new_owner; > + bool deboost = false; > DEFINE_WAKE_Q(wake_q); > - bool deboost; Nit: Based on what I've seen from Thomas and others, I ask for declarations in decreasing order of line length. So deboost should have stayed where it was. > > /* > @@ -2232,7 +2229,8 @@ static int fixup_pi_state_owner(u32 __us > /* > * We are here either because we stole the rtmutex from the > * previous highest priority waiter or we are the highest priority > - * waiter but failed to get the rtmutex the first time. > + * waiter but have failed to get the rtmutex the first time. > + * > * We have to replace the newowner TID in the user space variable. > * This must be atomic as we have to preserve the owner died bit here. > * > @@ -2249,7 +2247,7 @@ static int fixup_pi_state_owner(u32 __us > if (get_futex_value_locked(&uval, uaddr)) > goto handle_fault; > > - while (1) { > + for (;;) { As far as I'm aware, there is no difference and both are used throughout the kernel (with the while version having 50% more instances). Is there more to this than personal preference? > newval = (uval & FUTEX_OWNER_DIED) | newtid; > > if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) > @@ -2345,6 +2343,10 @@ static int fixup_owner(u32 __user *uaddr > /* > * Got the lock. We might not be the anticipated owner if we > * did a lock-steal - fix up the PI-state in that case: > + * > + * We can safely read pi_state->owner without holding wait_lock > + * because we now own the rt_mutex, only the owner will attempt > + * to change it. This seems to contradict the Serialization and lifetime rules: + * pi_mutex->wait_lock: + * + * {uval, pi_state} + * + * (and pi_mutex 'obviously') It would seem that simply holding pi_mutex is sufficient for serialization on pi_state->owner then. ... > @@ -2738,10 +2748,36 @@ static int futex_unlock_pi(u32 __user *u > */ > top_waiter = futex_top_waiter(hb, &key); > if (top_waiter) { > - ret = wake_futex_pi(uaddr, uval, top_waiter, hb); > + struct futex_pi_state *pi_state = top_waiter->pi_state; > + > + ret = -EINVAL; > + if (!pi_state) > + goto out_unlock; > + > + /* > + * If current does not own the pi_state then the futex is > + * inconsistent and user space fiddled with the futex value. > + */ > + if (pi_state->owner != current) > + goto out_unlock; > + > + /* > + * Grab a reference on the pi_state and drop hb->lock. > + * > + * The reference ensures pi_state lives, dropping the hb->lock > + * is tricky.. wake_futex_pi() will take rt_mutex::wait_lock to > + * close the races against futex_lock_pi(), but in case of > + * _any_ fail we'll abort and retry the whole deal. s/fail/failure/ -- Darren Hart VMware Open Source Technology Center