Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1617431 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from  under hb->lock Darren Hart <dvhart@infradead.org> - 2017-04-06 02:00 +0200
    Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from  under hb->lock Peter Zijlstra <peterz@infradead.org> - 2017-04-06 14:50 +0200
      Re: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from  under hb->lock Darren Hart <dvhart@infradead.org> - 2017-04-06 19:50 +0200

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

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) <peterz@infradead.org>
> ---
>  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

[toc] | [next] | [standalone]

#1617967

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


> > @@ -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.

Hurm, yeah I mostly do that. No idea what went wrong there.

> >  
> >  /*
> > @@ -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?

Nope. Only that. I think I played around with the loop at one point and
this is all that remained of that.

> >  		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.

> > +
> > +		/*
> > +		 * 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.

[toc] | [prev] | [next] | [standalone]

#1618225

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web