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

[Darren Hart <dvhart@infradead.org>]

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