From: Steven Rostedt <rostedt@goodmis.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Lai Jiangshan <laijs@cn.fujitsu.com>
Subject: Re: [patch 5/6] rtmutex: Clarify the lock chain walk
Date: Fri, 30 May 2014 22:19:55 -0400 [thread overview]
Message-ID: <20140530221955.620e2b61@gandalf.local.home> (raw)
In-Reply-To: <20140522031950.168005692@linutronix.de>
On Thu, 22 May 2014 03:25:54 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:
> Add a separate local variable for the boost/deboost logic to make the
> code more readable. Add comments where appropriate.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> kernel/locking/rtmutex.c | 50 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 42 insertions(+), 8 deletions(-)
>
> Index: tip/kernel/locking/rtmutex.c
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex.c
> +++ tip/kernel/locking/rtmutex.c
> @@ -302,9 +302,10 @@ static int rt_mutex_adjust_prio_chain(st
> struct rt_mutex_waiter *orig_waiter,
> struct task_struct *top_task)
> {
> - struct rt_mutex *lock;
> struct rt_mutex_waiter *waiter, *top_waiter = orig_waiter;
> + struct rt_mutex_waiter *prerequeue_top_waiter;
> int detect_deadlock, ret = 0, depth = 0;
> + struct rt_mutex *lock;
> unsigned long flags;
>
> detect_deadlock = rt_mutex_cond_detect_deadlock(orig_waiter,
> @@ -379,6 +380,11 @@ static int rt_mutex_adjust_prio_chain(st
> }
>
> lock = waiter->lock;
> + /*
> + * We need to trylock here as we are holding task->pi_lock,
> + * which is the reverse lock order versus the other rtmutex
> + * operations.
> + */
> if (!raw_spin_trylock(&lock->wait_lock)) {
> raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> cpu_relax();
> @@ -398,7 +404,11 @@ static int rt_mutex_adjust_prio_chain(st
> goto out_unlock_pi;
> }
>
> - top_waiter = rt_mutex_top_waiter(lock);
> + /*
> + * Store the top waiter before doing any requeue operation. We
> + * need it for the boost/deboost decision below.
> + */
> + prerequeue_top_waiter = rt_mutex_top_waiter(lock);
>
> /* Requeue the waiter */
> rt_mutex_dequeue(lock, waiter);
> @@ -407,13 +417,18 @@ static int rt_mutex_adjust_prio_chain(st
>
> /* Release the task */
> raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> +
> + /*
> + * We must abort the chain walk if there is no lock owner even
> + * in the dead lock detection case, as we have nothing to
> + * follow here.
You could mention that this is the end of the chain. That is, the chain
is lock->owner->lock->owner->lock->... Once you have a lock without
owner, or a owner not blocked on a lock, that's it. Pretty self
explanatory. Don't Need to change anything. Just add "This is the end
of the chain that we are walking."
> + */
> if (!rt_mutex_owner(lock)) {
> /*
> * If the requeue above changed the top waiter, then we need
> * to wake the new top waiter up to try to get the lock.
> */
> -
> - if (top_waiter != rt_mutex_top_waiter(lock))
> + if (prerequeue_top_waiter != rt_mutex_top_waiter(lock))
> wake_up_process(rt_mutex_top_waiter(lock)->task);
> raw_spin_unlock(&lock->wait_lock);
> goto out_put_task;
> @@ -426,17 +441,31 @@ static int rt_mutex_adjust_prio_chain(st
> raw_spin_lock_irqsave(&task->pi_lock, flags);
>
> if (waiter == rt_mutex_top_waiter(lock)) {
> - /* Boost the owner */
> - rt_mutex_dequeue_pi(task, top_waiter);
> + /*
> + * The waiter became the top waiter on the
> + * lock. Remove the previous top waiter from the tasks
> + * pi waiters list and add waiter to it.
Just to be a bit more verbose:
The waiter became the new top waiter on the lock. Replace the previous
top waiter from the tasks pi waiters list with this waiter.
If anything, I like to use "Replace .. with" instead of "Remove .. add"
as for me, it gives a better idea of what is actually being done
instead of just stating how it is being implemented.
> + */
> + rt_mutex_dequeue_pi(task, prerequeue_top_waiter);
> rt_mutex_enqueue_pi(task, waiter);
> __rt_mutex_adjust_prio(task);
>
> - } else if (top_waiter == waiter) {
> - /* Deboost the owner */
> + } else if (prerequeue_top_waiter == waiter) {
> + /*
> + * The waiter was the top waiter on the lock. Remove
> + * waiter from the tasks pi waiters list and add the
> + * new top waiter to it.
Again, I would prefer to use "Replace .. with":
The waiter was the top waiter on the lock but it is no longer the
highest priority waiter. Replace the waiter from the tasks pi waiters
list with the new top waiter (the highest priority one).
> + */
> rt_mutex_dequeue_pi(task, waiter);
> waiter = rt_mutex_top_waiter(lock);
> rt_mutex_enqueue_pi(task, waiter);
> __rt_mutex_adjust_prio(task);
> +
> + } else {
> + /*
> + * Nothing changed. No need to do any priority
> + * adjustment.
> + */
> }
>
> raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> @@ -444,6 +473,11 @@ static int rt_mutex_adjust_prio_chain(st
> top_waiter = rt_mutex_top_waiter(lock);
> raw_spin_unlock(&lock->wait_lock);
>
> + /*
> + * If the current waiter is not the top waiter on the lock,
> + * then we can stop the chain walk here if we are not in full
> + * deadlock detection mode.
> + */
> if (!detect_deadlock && waiter != top_waiter)
> goto out_put_task;
>
>
Rest looks good.
-- Steve
next prev parent reply other threads:[~2014-05-31 2:19 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-22 3:25 [patch 0/6] rtmutex: Repair deadlock detector and cleanup Thomas Gleixner
2014-05-22 3:25 ` [patch 1/6] rtmutex: Fix deadlock detector for real Thomas Gleixner
2014-05-27 22:09 ` Steven Rostedt
2014-05-28 9:57 ` Thomas Gleixner
2014-05-28 19:28 ` [tip:core/urgent] " tip-bot for Thomas Gleixner
2014-05-22 3:25 ` [patch 2/6] rtmutex: Remove builtin tester Thomas Gleixner
2014-05-30 21:36 ` Steven Rostedt
2014-05-22 3:25 ` [patch 3/6] rtmutex: Cleanup deadlock detector debug logic Thomas Gleixner
2014-05-30 22:08 ` Steven Rostedt
2014-06-21 20:32 ` [tip:locking/core] " tip-bot for Thomas Gleixner
2014-05-22 3:25 ` [patch 4/6] rtmutex: Confine deadlock logic to futex Thomas Gleixner
2014-05-22 7:10 ` Peter Zijlstra
2014-05-28 20:28 ` Thomas Gleixner
2014-05-31 2:06 ` Steven Rostedt
2014-05-22 3:25 ` [patch 5/6] rtmutex: Clarify the lock chain walk Thomas Gleixner
2014-05-31 2:19 ` Steven Rostedt [this message]
2014-05-22 3:25 ` [patch 6/6] rtmutex: Avoid pointless requeueing in the deadlock detection " Thomas Gleixner
2014-05-27 22:49 ` Jason Low
2014-05-28 9:43 ` Thomas Gleixner
2014-05-31 2:21 ` Steven Rostedt
2014-06-21 20:33 ` [tip:locking/core] " tip-bot for Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140530221955.620e2b61@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=laijs@cn.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).