Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock

From: Darren Hart <dvhart@infradead.org>
To: Peter Zijlstra <peterz@infradead.org>
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
Date: Thu, 6 Apr 2017 10:42:48 -0700	[thread overview]
Message-ID: <20170406174248.GD7030@fury> (raw)
In-Reply-To: <20170406124248.i7ibgne76yojnizh@hirez.programming.kicks-ass.net>

On Thu, Apr 06, 2017 at 02:42:48PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2017 at 04:52:25PM -0700, Darren Hart wrote:
> > 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?
> 
> See here:
> 
>   https://lkml.kernel.org/r/20170323145606.480214279@infradead.org

Ah, DeadLine, thanks.

...

> > >  		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.
> 
> Not a contradiction; just a very specific special case. If current is
> the owner of a lock, said owner will not be going anywhere.

OK.

> > > +
> > > +		/*
> > > +		 * 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/
> 
> I don't think that survives the patch-set. That is, I cannot find it in
> the current code.

Ah right, intermediate documentation. Kudos for that! :-)

-- 
Darren Hart
VMware Open Source Technology Center