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

Groups > linux.kernel > #1491272 > unrolled thread

[PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

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

  [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-26 14:50 +0200
    Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top  waiter Steven Rostedt <rostedt@goodmis.org> - 2016-09-26 17:20 +0200
      Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-26 17:30 +0200
        Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top  waiter Steven Rostedt <rostedt@goodmis.org> - 2016-09-26 17:40 +0200
          Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top  waiter Steven Rostedt <rostedt@goodmis.org> - 2016-09-26 17:40 +0200
            Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-26 17:50 +0200
              Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top  waiter Thomas Gleixner <tglx@linutronix.de> - 2016-09-29 16:50 +0200
                Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-29 17:00 +0200
          Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-26 17:50 +0200
    Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Peter Zijlstra <peterz@infradead.org> - 2016-09-28 11:30 +0200
    Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-09-28 11:40 +0200

#1491272 — [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

We should deboost before waking the high-priority task, such that we
don't run two tasks with the same "state" (priority, deadline,
sched_class, etc).

In order to make sure the boosting task doesn't start running between
unlock and deboost (due to 'spurious' wakeup), we move the deboost
under the wait_lock, that way its serialized against the wait loop in
__rt_mutex_slowlock().

Doing the deboost early can however lead to priority-inversion if
current would get preempted after the deboost but before waking our
high-prio task, hence we disable preemption before doing deboost, and
enabling it after the wake up is over.

This gets us the right semantic order, but most importantly however;
this change ensures pointer stability for the next patch, where we
have rt_mutex_setprio() cache a pointer to the top-most waiter task.
If we, as before this change, do the wakeup first and then deboost,
this pointer might point into thin air.

[peterz: Changelog + patch munging]
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Juri Lelli <juri.lelli@arm.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Xunlei Pang <xlpang@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---

 kernel/futex.c                  |    5 +---
 kernel/locking/rtmutex.c        |   48 ++++++++++++++++++++--------------------
 kernel/locking/rtmutex_common.h |    1 
 3 files changed, 28 insertions(+), 26 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1374,9 +1374,8 @@ static int wake_futex_pi(u32 __user *uad
 	 * scheduled away before the wake up can take place.
 	 */
 	spin_unlock(&hb->lock);
-	wake_up_q(&wake_q);
-	if (deboost)
-		rt_mutex_adjust_prio(current);
+
+	rt_mutex_postunlock(&wake_q, deboost);
 
 	return 0;
 }
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -307,24 +307,6 @@ static void __rt_mutex_adjust_prio(struc
 }
 
 /*
- * Adjust task priority (undo boosting). Called from the exit path of
- * rt_mutex_slowunlock() and rt_mutex_slowlock().
- *
- * (Note: We do this outside of the protection of lock->wait_lock to
- * allow the lock to be taken while or before we readjust the priority
- * of task. We do not use the spin_xx_mutex() variants here as we are
- * outside of the debug path.)
- */
-void rt_mutex_adjust_prio(struct task_struct *task)
-{
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&task->pi_lock, flags);
-	__rt_mutex_adjust_prio(task);
-	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
-}
-
-/*
  * Deadlock detection is conditional:
  *
  * If CONFIG_DEBUG_RT_MUTEXES=n, deadlock detection is only conducted
@@ -987,6 +969,7 @@ static void mark_wakeup_next_waiter(stru
 	 * lock->wait_lock.
 	 */
 	rt_mutex_dequeue_pi(current, waiter);
+	__rt_mutex_adjust_prio(current);
 
 	/*
 	 * As we are waking up the top waiter, and the waiter stays
@@ -1325,6 +1308,16 @@ static bool __sched rt_mutex_slowunlock(
 	 */
 	mark_wakeup_next_waiter(wake_q, lock);
 
+	/*
+	 * We should deboost before waking the top waiter task such that
+	 * we don't run two tasks with the 'same' priority. This however
+	 * can lead to prio-inversion if we would get preempted after
+	 * the deboost but before waking our high-prio task, hence the
+	 * preempt_disable before unlock. Pairs with preempt_enable() in
+	 * rt_mutex_postunlock();
+	 */
+	preempt_disable();
+
 	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	/* check PI boosting */
@@ -1390,14 +1383,23 @@ rt_mutex_fastunlock(struct rt_mutex *loc
 	} else {
 		bool deboost = slowfn(lock, &wake_q);
 
-		wake_up_q(&wake_q);
-
-		/* Undo pi boosting if necessary: */
-		if (deboost)
-			rt_mutex_adjust_prio(current);
+		rt_mutex_postunlock(&wake_q, deboost);
 	}
 }
 
+
+/*
+ * Undo pi boosting (if necessary) and wake top waiter.
+ */
+void rt_mutex_postunlock(struct wake_q_head *wake_q, bool deboost)
+{
+	wake_up_q(wake_q);
+
+	/* Pairs with preempt_disable() in rt_mutex_slowunlock() */
+	if (deboost)
+		preempt_enable();
+}
+
 /**
  * rt_mutex_lock - lock a rt_mutex
  *
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -111,6 +111,7 @@ extern int rt_mutex_finish_proxy_lock(st
 extern int rt_mutex_timed_futex_lock(struct rt_mutex *l, struct hrtimer_sleeper *to);
 extern bool rt_mutex_futex_unlock(struct rt_mutex *lock,
 				  struct wake_q_head *wqh);
+extern void rt_mutex_postunlock(struct wake_q_head *wake_q, bool deboost);
 extern void rt_mutex_adjust_prio(struct task_struct *task);
 
 #ifdef CONFIG_DEBUG_RT_MUTEXES

[toc] | [next] | [standalone]

#1491348 — Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

On Mon, 26 Sep 2016 14:32:14 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1374,9 +1374,8 @@ static int wake_futex_pi(u32 __user *uad
>  	 * scheduled away before the wake up can take place.
>  	 */
>  	spin_unlock(&hb->lock);
> -	wake_up_q(&wake_q);
> -	if (deboost)
> -		rt_mutex_adjust_prio(current);
> +
> +	rt_mutex_postunlock(&wake_q, deboost);

Hmm...

>  
>  	return 0;
>  }
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1325,6 +1308,16 @@ static bool __sched rt_mutex_slowunlock(
>  	 */
>  	mark_wakeup_next_waiter(wake_q, lock);
>  
> +	/*
> +	 * We should deboost before waking the top waiter task such that
> +	 * we don't run two tasks with the 'same' priority. This however
> +	 * can lead to prio-inversion if we would get preempted after
> +	 * the deboost but before waking our high-prio task, hence the
> +	 * preempt_disable before unlock. Pairs with preempt_enable() in
> +	 * rt_mutex_postunlock();

There's a preempt_enable() in rt_mutex_postunlock()? Does
wake_futex_pi() know that?

-- Steve


> +	 */
> +	preempt_disable();
> +
>  	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
>  
>  	/* check PI boosting */
> @@ -1390,14 +1383,23 @@ rt_mutex_fastunlock(struct rt_mutex *loc
>  	} else {
>  		bool deboost = slowfn(lock, &wake_q);
>  
> -		wake_up_q(&wake_q);
> -
> -		/* Undo pi boosting if necessary: */
> -		if (deboost)
> -			rt_mutex_adjust_prio(current);
> +		rt_mutex_postunlock(&wake_q, deboost);
>  	}
>  }
>  
> +
> +/*
> + * Undo pi boosting (if necessary) and wake top waiter.
> + */
> +void rt_mutex_postunlock(struct wake_q_head *wake_q, bool deboost)
> +{
> +	wake_up_q(wake_q);
> +
> +	/* Pairs with preempt_disable() in rt_mutex_slowunlock() */
> +	if (deboost)
> +		preempt_enable();
> +}
> +
>  /**
>   * rt_mutex_lock - lock a rt_mutex
>   *
> --- a/kernel/locking/rtmutex_common.h
> +++ b/kernel/locking/rtmutex_common.h
> @@ -111,6 +111,7 @@ extern int rt_mutex_finish_proxy_lock(st
>  extern int rt_mutex_timed_futex_lock(struct rt_mutex *l, struct hrtimer_sleeper *to);
>  extern bool rt_mutex_futex_unlock(struct rt_mutex *lock,
>  				  struct wake_q_head *wqh);
> +extern void rt_mutex_postunlock(struct wake_q_head *wake_q, bool deboost);
>  extern void rt_mutex_adjust_prio(struct task_struct *task);
>  
>  #ifdef CONFIG_DEBUG_RT_MUTEXES
> 

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

#1491356

On Mon, Sep 26, 2016 at 11:15:11AM -0400, Steven Rostedt wrote:
> On Mon, 26 Sep 2016 14:32:14 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -1374,9 +1374,8 @@ static int wake_futex_pi(u32 __user *uad
> >  	 * scheduled away before the wake up can take place.
> >  	 */
> >  	spin_unlock(&hb->lock);
> > -	wake_up_q(&wake_q);
> > -	if (deboost)
> > -		rt_mutex_adjust_prio(current);
> > +
> > +	rt_mutex_postunlock(&wake_q, deboost);
> 
> Hmm...
> 
> >  
> >  	return 0;
> >  }
> > --- a/kernel/locking/rtmutex.c
> > +++ b/kernel/locking/rtmutex.c
> > @@ -1325,6 +1308,16 @@ static bool __sched rt_mutex_slowunlock(
> >  	 */
> >  	mark_wakeup_next_waiter(wake_q, lock);
> >  
> > +	/*
> > +	 * We should deboost before waking the top waiter task such that
> > +	 * we don't run two tasks with the 'same' priority. This however
> > +	 * can lead to prio-inversion if we would get preempted after
> > +	 * the deboost but before waking our high-prio task, hence the
> > +	 * preempt_disable before unlock. Pairs with preempt_enable() in
> > +	 * rt_mutex_postunlock();
> 
> There's a preempt_enable() in rt_mutex_postunlock()? Does
> wake_futex_pi() know that?
> 

Not sure I see your point. rt_mutex_futex_unlock() calls
rt_mutex_slowunlock() which does the preempt_disable(), we then pass the
return of that into deboost, which we pass into rt_mutex_postunlock()
and everything should be balanced.

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

#1491368 — Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

On Mon, 26 Sep 2016 17:22:28 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> > > +	/*
> > > +	 * We should deboost before waking the top waiter task such that
> > > +	 * we don't run two tasks with the 'same' priority. This however
> > > +	 * can lead to prio-inversion if we would get preempted after
> > > +	 * the deboost but before waking our high-prio task, hence the
> > > +	 * preempt_disable before unlock. Pairs with preempt_enable() in
> > > +	 * rt_mutex_postunlock();  
> > 
> > There's a preempt_enable() in rt_mutex_postunlock()? Does
> > wake_futex_pi() know that?
> >   
> 
> Not sure I see your point. rt_mutex_futex_unlock() calls
> rt_mutex_slowunlock() which does the preempt_disable(), we then pass the
> return of that into deboost, which we pass into rt_mutex_postunlock()
> and everything should be balanced.

Can we please add more comments explaining this. Having side effects of
functions disabling preemption, passing a bool saying that it did, and
needing to call another function (somewhat seemingly unrelated) to
re-enable preemption, just seems a bit of a stretch for maintainable
code.

Especially now that the code after the spin_unlock(&hb->lock) is now a
critical section (preemption is disable). There's nothing obvious in
futex.c that says it is.

Just think about looking at this code in another 5 years. Are you going
to remember all this?

-- Steve

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

#1491372 — Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

On Mon, 26 Sep 2016 11:35:03 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Especially now that the code after the spin_unlock(&hb->lock) is now a
> critical section (preemption is disable). There's nothing obvious in
> futex.c that says it is.

Not to mention, this looks like it will break PREEMPT_RT as wake_up_q()
calls sleepable spin locks.

-- Steve

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

#1491378

On Mon, Sep 26, 2016 at 11:37:27AM -0400, Steven Rostedt wrote:
> On Mon, 26 Sep 2016 11:35:03 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Especially now that the code after the spin_unlock(&hb->lock) is now a
> > critical section (preemption is disable). There's nothing obvious in
> > futex.c that says it is.
> 
> Not to mention, this looks like it will break PREEMPT_RT as wake_up_q()
> calls sleepable spin locks.

What locks would that be?

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

#1493519 — Re: [PATCH -v2 1/9] rtmutex: Deboost before waking up the top waiter

On Mon, 26 Sep 2016, Peter Zijlstra wrote:

> On Mon, Sep 26, 2016 at 11:37:27AM -0400, Steven Rostedt wrote:
> > On Mon, 26 Sep 2016 11:35:03 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Especially now that the code after the spin_unlock(&hb->lock) is now a
> > > critical section (preemption is disable). There's nothing obvious in
> > > futex.c that says it is.
> > 
> > Not to mention, this looks like it will break PREEMPT_RT as wake_up_q()
> > calls sleepable spin locks.
> 
> What locks would that be?

None :)

It still breaks RT in the futex case due to:

   deboost = rt_mutex_futex_unlock();

   spin_unlock(&hb->lock);
	....
	migrate_enable();
	    if (in_atomic())
		return;

So the migrate_disable() which was emitted by spin_lock(&hb->lock) will not
be cleaned up and we leak the migrate disable count. We can work around
that, but it's not pretty.

As a related note, Sebastian decoded another possible priority inversion
issue in the futex mess.

      T1 holds futex

      T2 blocks on futex and boosts T1

      T1 unlocks futex and holds hb->lock

      T1 unlocks rt mutex, so T1 has no more pi waiters

      T3 blocks on hb->lock and adds itself to the pi waiters list of T1

      T1 unlocks hb->lock and deboosts itself

      T4 preempts T1 so the wakeup of T2 gets delayed .....

We tried to fix it with a preempt_disable() and that's where we ran into
that migrate_enable() hickup. We have a non deboosting variant for
spin_unlock() for now, but we'll have to revisit that anyway ...

Thanks,

	tglx

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

#1493527

On Thu, Sep 29, 2016 at 10:43:54AM -0400, Thomas Gleixner wrote:
> On Mon, 26 Sep 2016, Peter Zijlstra wrote:
> 
> > On Mon, Sep 26, 2016 at 11:37:27AM -0400, Steven Rostedt wrote:
> > > On Mon, 26 Sep 2016 11:35:03 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > > Especially now that the code after the spin_unlock(&hb->lock) is now a
> > > > critical section (preemption is disable). There's nothing obvious in
> > > > futex.c that says it is.
> > > 
> > > Not to mention, this looks like it will break PREEMPT_RT as wake_up_q()
> > > calls sleepable spin locks.
> > 
> > What locks would that be?
> 
> None :)
> 
> It still breaks RT in the futex case due to:
> 
>    deboost = rt_mutex_futex_unlock();
> 
>    spin_unlock(&hb->lock);
> 	....

Yeah, noticed that already. Am currently trying to untangle the pi_state
locking rules.

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

#1491375

On Mon, Sep 26, 2016 at 11:35:03AM -0400, Steven Rostedt wrote:
> On Mon, 26 Sep 2016 17:22:28 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > > +	/*
> > > > +	 * We should deboost before waking the top waiter task such that
> > > > +	 * we don't run two tasks with the 'same' priority. This however
> > > > +	 * can lead to prio-inversion if we would get preempted after
> > > > +	 * the deboost but before waking our high-prio task, hence the
> > > > +	 * preempt_disable before unlock. Pairs with preempt_enable() in
> > > > +	 * rt_mutex_postunlock();  
> > > 
> > > There's a preempt_enable() in rt_mutex_postunlock()? Does
> > > wake_futex_pi() know that?
> > >   
> > 
> > Not sure I see your point. rt_mutex_futex_unlock() calls
> > rt_mutex_slowunlock() which does the preempt_disable(), we then pass the
> > return of that into deboost, which we pass into rt_mutex_postunlock()
> > and everything should be balanced.
> 
> Can we please add more comments explaining this. Having side effects of
> functions disabling preemption, passing a bool saying that it did, and
> needing to call another function (somewhat seemingly unrelated) to
> re-enable preemption, just seems a bit of a stretch for maintainable
> code.
> 
> Especially now that the code after the spin_unlock(&hb->lock) is now a
> critical section (preemption is disable). There's nothing obvious in
> futex.c that says it is.
> 
> Just think about looking at this code in another 5 years. Are you going
> to remember all this?

There's some cleanups later in the series that should clear this up.

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

#1492459

On Wed, Sep 28, 2016 at 11:07:40AM +0200, Sebastian Andrzej Siewior wrote:
> On 2016-09-26 14:32:14 [+0200], Peter Zijlstra wrote:
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -1374,9 +1374,8 @@ static int wake_futex_pi(u32 __user *uad
> >  	 * scheduled away before the wake up can take place.
> >  	 */
> >  	spin_unlock(&hb->lock);
> > -	wake_up_q(&wake_q);
> > -	if (deboost)
> > -		rt_mutex_adjust_prio(current);
> > +
> > +	rt_mutex_postunlock(&wake_q, deboost);
> 
> This breaks -RT. Before that spin_unlock() you do a preempt_disable()
> which means you had one spinlock with enabled preemption and now you get
> one unlock with disabled preemption. And this breaks migrate_disable() /
> enable (because we take the fast path in the in_atomic() case).

Oh crud, the hb lock is not raw :/

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

#1492491

On 2016-09-26 14:32:14 [+0200], Peter Zijlstra wrote:
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1374,9 +1374,8 @@ static int wake_futex_pi(u32 __user *uad
>  	 * scheduled away before the wake up can take place.
>  	 */
>  	spin_unlock(&hb->lock);
> -	wake_up_q(&wake_q);
> -	if (deboost)
> -		rt_mutex_adjust_prio(current);
> +
> +	rt_mutex_postunlock(&wake_q, deboost);

This breaks -RT. Before that spin_unlock() you do a preempt_disable()
which means you had one spinlock with enabled preemption and now you get
one unlock with disabled preemption. And this breaks migrate_disable() /
enable (because we take the fast path in the in_atomic() case).

>  	return 0;
>  }
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -307,24 +307,6 @@ static void __rt_mutex_adjust_prio(struc
>  }
>  
>  /*
> - * Adjust task priority (undo boosting). Called from the exit path of
> - * rt_mutex_slowunlock() and rt_mutex_slowlock().
> - *
> - * (Note: We do this outside of the protection of lock->wait_lock to
> - * allow the lock to be taken while or before we readjust the priority
> - * of task. We do not use the spin_xx_mutex() variants here as we are
> - * outside of the debug path.)
> - */
> -void rt_mutex_adjust_prio(struct task_struct *task)
> -{
> -	unsigned long flags;
> -
> -	raw_spin_lock_irqsave(&task->pi_lock, flags);
> -	__rt_mutex_adjust_prio(task);
> -	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> -}

I don't see this function getting back somewhere in this patch. There is
one occurrence left in kernel/locking/rtmutex_common.h

Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web