| Message ID | 1492092174-31734-3-git-send-email-alex.shi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | rtmutex comments update and trival fix | expand |
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
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.
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
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.
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
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.
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(¤t->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(¤t->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; } /*
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(-) -- 1.9.1