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

Groups > linux.kernel > #1617035 > unrolled thread

Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt_mutex API

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 04/13] futex,rt_mutex: Provide futex specific  rt_mutex API Darren Hart <dvhart@infradead.org> - 2017-04-05 17:10 +0200
    Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific  rt_mutex API Peter Zijlstra <peterz@infradead.org> - 2017-04-06 14:20 +0200
      Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific  rt_mutex API Darren Hart <dvhart@infradead.org> - 2017-04-06 19:10 +0200

#1617035 — Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt_mutex API

On Wed, Mar 22, 2017 at 11:35:51AM +0100, Peter Zijlstra wrote:
> Part of what makes futex_unlock_pi() intricate is that
> rt_mutex_futex_unlock() -> rt_mutex_slowunlock() can drop
> rt_mutex::wait_lock.
> 
> This means we cannot rely on the atomicy of wait_lock, which we would
> like to do in order to not rely on hb->lock so much.
> 
> The reason rt_mutex_slowunlock() needs to drop wait_lock is because it
> can race with the rt_mutex fastpath, however futexes have their own
> fast path.
> 
> Since futexes already have a bunch of separate rt_mutex accessors,
> complete that set and implement a rt_mutex variant without fastpath
> for them.

Premise makes sense, I'm tripping over some detail - wondering if it is all
related...

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/futex.c                  |   30 ++++++++++-----------
>  kernel/locking/rtmutex.c        |   55 +++++++++++++++++++++++++++++-----------
>  kernel/locking/rtmutex_common.h |    9 +++++-
>  3 files changed, 62 insertions(+), 32 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -916,7 +916,7 @@ void exit_pi_state_list(struct task_stru
>  		pi_state->owner = NULL;
>  		raw_spin_unlock_irq(&curr->pi_lock);
>  
> -		rt_mutex_unlock(&pi_state->pi_mutex);
> +		rt_mutex_futex_unlock(&pi_state->pi_mutex);
>  
>  		spin_unlock(&hb->lock);
>  
> @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad
>  	pi_state->owner = new_owner;
>  	raw_spin_unlock(&new_owner->pi_lock);
>  
> -	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
> -
> -	deboost = rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
> -
>  	/*
> -	 * First unlock HB so the waiter does not spin on it once he got woken
> -	 * up. Second wake up the waiter before the priority is adjusted. If we
> -	 * deboost first (and lose our higher priority), then the task might get
> -	 * scheduled away before the wake up can take place.
> +	 * We've updated the uservalue, this unlock cannot fail.

It isn't clear to me what I should understand from this new comment. How does
the value of the uval affect whether or not the pi_state->pi_mutex can be
unlocked or not? Or are you noting that we've set FUTEX_WAITIERS so any valid
userspace operations will be forced intot he kernel and can't race with us since
we hold the hb->lock? With futexes, I think it's important that we be very
explicit in our comment blocks.

>  	 */
> +	deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
> +
> +	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
>  	spin_unlock(&hb->lock);
> -	wake_up_q(&wake_q);
> -	if (deboost)
> +
> +	if (deboost) {
> +		wake_up_q(&wake_q);

Is moving wake_up_q under deboost related to this change or is it just an
optimization since there is no need to wake unless we are deboosting ourselves -
which was true before as well?

If this is due to the rt_mutex_futex* API, I haven't made the connection.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [next] | [standalone]

#1617915

On Wed, Apr 05, 2017 at 08:02:17AM -0700, Darren Hart wrote:
> > @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad
> >  	pi_state->owner = new_owner;
> >  	raw_spin_unlock(&new_owner->pi_lock);
> >  
> >  	/*
> > +	 * We've updated the uservalue, this unlock cannot fail.
> 
> It isn't clear to me what I should understand from this new comment. How does
> the value of the uval affect whether or not the pi_state->pi_mutex can be
> unlocked or not? Or are you noting that we've set FUTEX_WAITIERS so any valid
> userspace operations will be forced intot he kernel and can't race with us since
> we hold the hb->lock? With futexes, I think it's important that we be very
> explicit in our comment blocks.

The critical point is that once you've modified uval we must not fail;
there is no way to undo things thereafter.

> >  	 */
> > +	deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
> > +
> > +	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
> >  	spin_unlock(&hb->lock);
> > +
> > +	if (deboost) {
> > +		wake_up_q(&wake_q);
> 
> Is moving wake_up_q under deboost related to this change or is it just an
> optimization since there is no need to wake unless we are deboosting ourselves -
> which was true before as well?
> 
> If this is due to the rt_mutex_futex* API, I haven't made the connection.

It's how rt_mutex does wakeups, note that later patches clean this up.

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

#1618196

On Thu, Apr 06, 2017 at 02:17:28PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2017 at 08:02:17AM -0700, Darren Hart wrote:
> > > @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad
> > >  	pi_state->owner = new_owner;
> > >  	raw_spin_unlock(&new_owner->pi_lock);
> > >  
> > >  	/*
> > > +	 * We've updated the uservalue, this unlock cannot fail.
> > 
> > It isn't clear to me what I should understand from this new comment. How does
> > the value of the uval affect whether or not the pi_state->pi_mutex can be
> > unlocked or not? Or are you noting that we've set FUTEX_WAITIERS so any valid
> > userspace operations will be forced intot he kernel and can't race with us since
> > we hold the hb->lock? With futexes, I think it's important that we be very
> > explicit in our comment blocks.
> 
> The critical point is that once you've modified uval we must not fail;
> there is no way to undo things thereafter.

Aha, "must not", OK. I interpretted "cannot" as "is incapable of failing". So
let's use something like that for the comment:

/*
 * We updated the user value and are committed to completing the unlock, we must
 * not fail.
 */

Wow... English. I tried a few versions, but cannot, may not, etc. all have
doublt meanings. :-)

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web