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

Groups > linux.kernel > #1623056 > unrolled thread

[PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

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 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock Alex Shi <alex.shi@linaro.org> - 2017-04-13 16:10 +0200
    Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Sebastian Siewior <bigeasy@linutronix.de> - 2017-04-13 16:30 +0200
    Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Peter Zijlstra <peterz@infradead.org> - 2017-04-13 16:50 +0200
      Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Steven Rostedt <rostedt@goodmis.org> - 2017-04-13 18:20 +0200
        Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Peter Zijlstra <peterz@infradead.org> - 2017-04-13 18:30 +0200
          Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Steven Rostedt <rostedt@goodmis.org> - 2017-04-13 18:50 +0200
            Re: [PATCH 2/3] rtmutex: deboost priority conditionally when  rt-mutex unlock Peter Zijlstra <peterz@infradead.org> - 2017-04-13 19:00 +0200

#1623056 — [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

The rt_mutex_fastunlock() will deboost 'current' task when it should be.
but the rt_mutex_slowunlock() function will set the 'deboost' flag
unconditionally. That cause some unnecessary priority adjustment.

'current' release this lock, so 'current' should be a higher prio
task than the next top waiter, unless the current prio was gotten
from this top waiter, iff so, we need to deboost 'current' after
the lock release.

Signed-off-by: Alex Shi <alex.shi@linaro.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
To: linux-kernel@vger.kernel.org
To: Ingo Molnar <mingo@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/locking/rtmutex.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 6edc32e..05ff685 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1037,10 +1037,11 @@ static int task_blocks_on_rt_mutex(struct rt_mutex *lock,
  *
  * Called with lock->wait_lock held and interrupts disabled.
  */
-static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
+static bool mark_wakeup_next_waiter(struct wake_q_head *wake_q,
 				    struct rt_mutex *lock)
 {
 	struct rt_mutex_waiter *waiter;
+	bool deboost = false;
 
 	raw_spin_lock(&current->pi_lock);
 
@@ -1055,6 +1056,15 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
 	rt_mutex_dequeue_pi(current, waiter);
 
 	/*
+	 * 'current' release this lock, so 'current' should be a higher prio
+	 * task than the next top waiter, unless the current prio was gotten
+	 * from this top waiter, iff so, we need to deboost 'current' after
+	 * the lock release.
+	 */
+	if (current->prio == waiter->prio)
+		deboost = true;
+
+	/*
 	 * As we are waking up the top waiter, and the waiter stays
 	 * queued on the lock until it gets the lock, this lock
 	 * obviously has waiters. Just set the bit here and this has
@@ -1067,6 +1077,8 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
 	raw_spin_unlock(&current->pi_lock);
 
 	wake_q_add(wake_q, waiter->task);
+
+	return deboost;
 }
 
 /*
@@ -1336,6 +1348,7 @@ static bool __sched rt_mutex_slowunlock(struct rt_mutex *lock,
 					struct wake_q_head *wake_q)
 {
 	unsigned long flags;
+	bool deboost = false;
 
 	/* irqsave required to support early boot calls */
 	raw_spin_lock_irqsave(&lock->wait_lock, flags);
@@ -1389,12 +1402,12 @@ static bool __sched rt_mutex_slowunlock(struct rt_mutex *lock,
 	 *
 	 * Queue the next waiter for wakeup once we release the wait_lock.
 	 */
-	mark_wakeup_next_waiter(wake_q, lock);
+	deboost = mark_wakeup_next_waiter(wake_q, lock);
 
 	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	/* check PI boosting */
-	return true;
+	return deboost;
 }
 
 /*
-- 
1.9.1

[toc] | [next] | [standalone]

#1623078 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On 2017-04-13 22:02:53 [+0800], Alex Shi wrote:
> The rt_mutex_fastunlock() will deboost 'current' task when it should be.

without looking whether or not this patch makes sense those patches
won't apply. Please look at tip/master or tip's locking/core
	https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=locking/core

where PeterZ rewrote part of the code.

Sebastian

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

#1623090 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On Thu, Apr 13, 2017 at 10:02:53PM +0800, Alex Shi wrote:
>  	/*
> +	 * 'current' release this lock, so 'current' should be a higher prio
> +	 * task than the next top waiter, unless the current prio was gotten
> +	 * from this top waiter, iff so, we need to deboost 'current' after
> +	 * the lock release.
> +	 */
> +	if (current->prio == waiter->prio)
> +		deboost = true;

This is wrong.

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

#1623154 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On Thu, 13 Apr 2017 16:39:52 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Apr 13, 2017 at 10:02:53PM +0800, Alex Shi wrote:
> >  	/*
> > +	 * 'current' release this lock, so 'current' should be a higher prio
> > +	 * task than the next top waiter, unless the current prio was gotten
> > +	 * from this top waiter, iff so, we need to deboost 'current' after
> > +	 * the lock release.
> > +	 */
> > +	if (current->prio == waiter->prio)
> > +		deboost = true;  
> 
> This is wrong.

The comment is, especially that "iff". What if current and waiter
happen to have the same priority? Then it too doesn't need to be
deboosted.

But that said, we currently perform the deboost unconditionally. I
can't think of a case where current->prio != waiter->prio where we
should perform the deboost, because current->prio should always be <=
waiter->prio (where lower prio means higher priority). Maybe I'm missing
something.

-- Steve

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

#1623163 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On Thu, Apr 13, 2017 at 12:09:25PM -0400, Steven Rostedt wrote:
> On Thu, 13 Apr 2017 16:39:52 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, Apr 13, 2017 at 10:02:53PM +0800, Alex Shi wrote:
> > >  	/*
> > > +	 * 'current' release this lock, so 'current' should be a higher prio
> > > +	 * task than the next top waiter, unless the current prio was gotten
> > > +	 * from this top waiter, iff so, we need to deboost 'current' after
> > > +	 * the lock release.
> > > +	 */
> > > +	if (current->prio == waiter->prio)
> > > +		deboost = true;  
> > 
> > This is wrong.
> 
> The comment is, especially that "iff". What if current and waiter
> happen to have the same priority? Then it too doesn't need to be
> deboosted.

The wrongness is in comparing prio and thinking it means anything.

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

#1623182 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On Thu, 13 Apr 2017 18:21:13 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Apr 13, 2017 at 12:09:25PM -0400, Steven Rostedt wrote:
> > On Thu, 13 Apr 2017 16:39:52 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> > > On Thu, Apr 13, 2017 at 10:02:53PM +0800, Alex Shi wrote:  
> > > >  	/*
> > > > +	 * 'current' release this lock, so 'current' should be a higher prio
> > > > +	 * task than the next top waiter, unless the current prio was gotten
> > > > +	 * from this top waiter, iff so, we need to deboost 'current' after
> > > > +	 * the lock release.
> > > > +	 */
> > > > +	if (current->prio == waiter->prio)
> > > > +		deboost = true;    
> > > 
> > > This is wrong.  
> > 
> > The comment is, especially that "iff". What if current and waiter
> > happen to have the same priority? Then it too doesn't need to be
> > deboosted.  
> 
> The wrongness is in comparing prio and thinking it means anything.

Because of deadline scheduling?

-- Steve

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

#1623184 — Re: [PATCH 2/3] rtmutex: deboost priority conditionally when rt-mutex unlock

On Thu, Apr 13, 2017 at 12:40:14PM -0400, Steven Rostedt wrote:
> On Thu, 13 Apr 2017 18:21:13 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, Apr 13, 2017 at 12:09:25PM -0400, Steven Rostedt wrote:
> > > On Thu, 13 Apr 2017 16:39:52 +0200
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > >   
> > > > On Thu, Apr 13, 2017 at 10:02:53PM +0800, Alex Shi wrote:  
> > > > >  	/*
> > > > > +	 * 'current' release this lock, so 'current' should be a higher prio
> > > > > +	 * task than the next top waiter, unless the current prio was gotten
> > > > > +	 * from this top waiter, iff so, we need to deboost 'current' after
> > > > > +	 * the lock release.
> > > > > +	 */
> > > > > +	if (current->prio == waiter->prio)
> > > > > +		deboost = true;    
> > > > 
> > > > This is wrong.  
> > > 
> > > The comment is, especially that "iff". What if current and waiter
> > > happen to have the same priority? Then it too doesn't need to be
> > > deboosted.  
> > 
> > The wrongness is in comparing prio and thinking it means anything.
> 
> Because of deadline scheduling?

Yep.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web