* [patch 0/2] rtmutex: Fix the deadlock detector for real @ 2014-05-14 20:03 Thomas Gleixner 2014-05-14 20:03 ` [patch 1/2] rtmutex: Fix " Thomas Gleixner 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner 0 siblings, 2 replies; 9+ messages in thread From: Thomas Gleixner @ 2014-05-14 20:03 UTC (permalink / raw) To: LKML; +Cc: Steven Rostedt, Lai Jiangshan, Peter Zijlstra, Ingo Molnar Lai pointed out that the chain walk is incomplete aside of not detecting even the simple AA deadlock. The following series contains: 1) The fix for this, which replaces the hot fix I send out earlier 2) An optimization to avoid pointless de/enqueue operations in the deadlock chain walk Thanks, tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch 1/2] rtmutex: Fix deadlock detector for real 2014-05-14 20:03 [patch 0/2] rtmutex: Fix the deadlock detector for real Thomas Gleixner @ 2014-05-14 20:03 ` Thomas Gleixner 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner 1 sibling, 0 replies; 9+ messages in thread From: Thomas Gleixner @ 2014-05-14 20:03 UTC (permalink / raw) To: LKML; +Cc: Steven Rostedt, Lai Jiangshan, Peter Zijlstra, Ingo Molnar [-- Attachment #1: rtmutex-fix-deadlock-detector-for-real.patch --] [-- Type: text/plain, Size: 2179 bytes --] The current deadlock detection logic does not work reliably due to the following early exit path: /* * Drop out, when the task has no waiters. Note, * top_waiter can be NULL, when we are in the deboosting * mode! */ if (top_waiter && (!task_has_pi_waiters(task) || top_waiter != task_top_pi_waiter(task))) goto out_unlock_pi; So this not only exits when the task has no waiters, it also exits unconditionally when the current waiter is not the top priority waiter of the task. So in a nested locking scenario, it might abort the lock chain walk and therefor miss a potential deadlock. Simple fix: Continue the chain walk, when deadlock detection is enabled. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@vger.kernel.org --- kernel/locking/rtmutex.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) Index: linux-2.6/kernel/locking/rtmutex.c =================================================================== --- linux-2.6.orig/kernel/locking/rtmutex.c +++ linux-2.6/kernel/locking/rtmutex.c @@ -343,16 +343,24 @@ static int rt_mutex_adjust_prio_chain(st * top_waiter can be NULL, when we are in the deboosting * mode! */ - if (top_waiter && (!task_has_pi_waiters(task) || - top_waiter != task_top_pi_waiter(task))) - goto out_unlock_pi; + if (top_waiter) { + if (!task_has_pi_waiters(task)) + goto out_unlock_pi; + + if (top_waiter != task_top_pi_waiter(task)) { + if (!detect_deadlock) + goto out_unlock_pi; + } + } /* * When deadlock detection is off then we check, if further * priority adjustment is necessary. */ - if (!detect_deadlock && waiter->prio == task->prio) - goto out_unlock_pi; + if (waiter->prio == task->prio) { + if (!detect_deadlock) + goto out_unlock_pi; + } lock = waiter->lock; if (!raw_spin_trylock(&lock->wait_lock)) { @@ -527,6 +535,10 @@ static int task_blocks_on_rt_mutex(struc unsigned long flags; int chain_walk = 0, res; + /* Early deadlock detection */ + if (detect_deadlock && owner == task) + return -EDEADLK; + raw_spin_lock_irqsave(&task->pi_lock, flags); __rt_mutex_adjust_prio(task); waiter->task = task; ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-14 20:03 [patch 0/2] rtmutex: Fix the deadlock detector for real Thomas Gleixner 2014-05-14 20:03 ` [patch 1/2] rtmutex: Fix " Thomas Gleixner @ 2014-05-14 20:03 ` Thomas Gleixner 2014-05-15 6:47 ` Ingo Molnar ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Thomas Gleixner @ 2014-05-14 20:03 UTC (permalink / raw) To: LKML; +Cc: Steven Rostedt, Lai Jiangshan, Peter Zijlstra, Ingo Molnar [-- Attachment #1: rtmutex-avoid-pointless-requeueing.patch --] [-- Type: text/plain, Size: 3605 bytes --] In case the dead lock detector is enabled we follow the lock chain to the end in rt_mutex_adjust_prio_chain, even if we could stop earlier due to the priority/waiter constellation. But once we are not longer the top priority waiter in a certain step or the task holding the lock has already the same priority then there is no point in dequeing and enqueing along the lock chain as there is no change at all. So stop the queueing at this point. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- kernel/locking/rtmutex.c | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) Index: linux-2.6/kernel/locking/rtmutex.c =================================================================== --- linux-2.6.orig/kernel/locking/rtmutex.c +++ linux-2.6/kernel/locking/rtmutex.c @@ -284,10 +284,11 @@ 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; int detect_deadlock, ret = 0, depth = 0; + struct rt_mutex *lock; unsigned long flags; + int requeue = 1; detect_deadlock = debug_rt_mutex_detect_deadlock(orig_waiter, deadlock_detect); @@ -350,6 +351,7 @@ static int rt_mutex_adjust_prio_chain(st if (top_waiter != task_top_pi_waiter(task)) { if (!detect_deadlock) goto out_unlock_pi; + requeue = 0; } } @@ -360,6 +362,7 @@ static int rt_mutex_adjust_prio_chain(st if (waiter->prio == task->prio) { if (!detect_deadlock) goto out_unlock_pi; + requeue = 0; } lock = waiter->lock; @@ -379,19 +382,25 @@ static int rt_mutex_adjust_prio_chain(st top_waiter = rt_mutex_top_waiter(lock); - /* Requeue the waiter */ - rt_mutex_dequeue(lock, waiter); - waiter->prio = task->prio; - rt_mutex_enqueue(lock, waiter); + if (requeue) { + /* Requeue the waiter */ + rt_mutex_dequeue(lock, waiter); + waiter->prio = task->prio; + rt_mutex_enqueue(lock, waiter); + } /* 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. + */ 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)) wake_up_process(rt_mutex_top_waiter(lock)->task); raw_spin_unlock(&lock->wait_lock); @@ -404,18 +413,20 @@ static int rt_mutex_adjust_prio_chain(st get_task_struct(task); 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); - rt_mutex_enqueue_pi(task, waiter); - __rt_mutex_adjust_prio(task); - - } else if (top_waiter == waiter) { - /* Deboost the owner */ - rt_mutex_dequeue_pi(task, waiter); - waiter = rt_mutex_top_waiter(lock); - rt_mutex_enqueue_pi(task, waiter); - __rt_mutex_adjust_prio(task); + if (requeue) { + if (waiter == rt_mutex_top_waiter(lock)) { + /* Boost the owner */ + rt_mutex_dequeue_pi(task, top_waiter); + rt_mutex_enqueue_pi(task, waiter); + __rt_mutex_adjust_prio(task); + + } else if (top_waiter == waiter) { + /* Deboost the owner */ + rt_mutex_dequeue_pi(task, waiter); + waiter = rt_mutex_top_waiter(lock); + rt_mutex_enqueue_pi(task, waiter); + __rt_mutex_adjust_prio(task); + } } raw_spin_unlock_irqrestore(&task->pi_lock, flags); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner @ 2014-05-15 6:47 ` Ingo Molnar 2014-05-15 21:49 ` Steven Rostedt 2014-05-15 17:04 ` Steven Rostedt 2014-05-15 21:39 ` Steven Rostedt 2 siblings, 1 reply; 9+ messages in thread From: Ingo Molnar @ 2014-05-15 6:47 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, Steven Rostedt, Lai Jiangshan, Peter Zijlstra A couple of suggestions: 1) * Thomas Gleixner <tglx@linutronix.de> wrote: > + if (requeue) { > + if (waiter == rt_mutex_top_waiter(lock)) { So we have a 'top_waiter' local variable already at this point, and we use it here: > + /* Boost the owner */ > + rt_mutex_dequeue_pi(task, top_waiter); > + rt_mutex_enqueue_pi(task, waiter); > + __rt_mutex_adjust_prio(task); > + > + } else if (top_waiter == waiter) { To me it's a bit confusing that we have both the 'top_waiter' local variable and also evaluate 'rt_mutex_top_waiter()' directly. So what happens is that when we do the requeue, the top waiter might change. I'd really suggest to signal that via naming - i.e. add another local variable (which GCC will optimize out happily), named descriptively: orig_top_waiter = top_waiter; and use that variable after that point. > + /* Deboost the owner */ > + rt_mutex_dequeue_pi(task, waiter); > + waiter = rt_mutex_top_waiter(lock); > + rt_mutex_enqueue_pi(task, waiter); > + __rt_mutex_adjust_prio(task); > + } > } 2) Also another small flow of control side comment, because this code is apparently rather tricky, I'd suggest a bit more explicit structure to show the real flow of the logic: for example in the first reading of the above block I mistakenly read it as a usual 'if () { } else { }' block pattern - which it really isn't. Something like this would be slightly easier to understand 'at a glance', IMHO: if (requeue) { if (waiter == top_waiter) { /* Boost the owner */ rt_mutex_dequeue_pi(task, orig_top_waiter); rt_mutex_enqueue_pi(task, waiter); __rt_mutex_adjust_prio(task); } else { if (orig_top_waiter == waiter) { /* Deboost the owner */ rt_mutex_dequeue_pi(task, waiter); waiter = rt_mutex_top_waiter(lock); rt_mutex_enqueue_pi(task, waiter); __rt_mutex_adjust_prio(task); } else { /* The requeueing did not affect us, no need to boost or deboost */ } } } Assuming you agree with this structure, it's a bit more verbose, but this might be one of the cases where verbosity helps readability. (Note that I already propagated the 'orig_top_waiter' name into it.) 3) Also note how the code continues: raw_spin_unlock_irqrestore(&task->pi_lock, flags); top_waiter = rt_mutex_top_waiter(lock); raw_spin_unlock(&lock->wait_lock); if (!detect_deadlock && waiter != top_waiter) goto out_put_task; goto again; So we evaluate 'top_waiter' again - maybe we could move that line to the two branches that actually have a chance to change the top waiter, and not change it in the 'no need to requeue' case. So ... all in one, what I would suggest is something like the patch below, on top of your two patches. Totally untested and such. Thanks, Ingo =======================> Subject: locking/rtmutex: Clean up the rt_mutex_adjust_prio_chain() code flow From: Ingo Molnar <mingo@kernel.org> Date: Thu May 15 08:39:42 CEST 2014 Clean up the code flow and variable names, always precisely maintaining the 'top_waiter' and 'orig_top_waiter' values whenever they can change. This probably optimizes the !requeue case a bit as well. Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/locking/rtmutex.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) Index: tip/kernel/locking/rtmutex.c =================================================================== --- tip.orig/kernel/locking/rtmutex.c +++ tip/kernel/locking/rtmutex.c @@ -284,7 +284,7 @@ static int rt_mutex_adjust_prio_chain(st struct rt_mutex_waiter *orig_waiter, struct task_struct *top_task) { - struct rt_mutex_waiter *waiter, *top_waiter = orig_waiter; + struct rt_mutex_waiter *waiter, *top_waiter = orig_waiter, *orig_top_waiter; int detect_deadlock, ret = 0, depth = 0; struct rt_mutex *lock; unsigned long flags; @@ -380,13 +380,17 @@ static int rt_mutex_adjust_prio_chain(st goto out_unlock_pi; } - top_waiter = rt_mutex_top_waiter(lock); - if (requeue) { + orig_top_waiter = rt_mutex_top_waiter(lock); + /* Requeue the waiter */ rt_mutex_dequeue(lock, waiter); waiter->prio = task->prio; rt_mutex_enqueue(lock, waiter); + + top_waiter = rt_mutex_top_waiter(lock); + } else { + orig_top_waiter = top_waiter = rt_mutex_top_waiter(lock); } /* Release the task */ @@ -401,8 +405,8 @@ static int rt_mutex_adjust_prio_chain(st * 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)) - wake_up_process(rt_mutex_top_waiter(lock)->task); + if (top_waiter != orig_top_waiter) + wake_up_process(top_waiter->task); raw_spin_unlock(&lock->wait_lock); goto out_put_task; } @@ -414,24 +418,27 @@ static int rt_mutex_adjust_prio_chain(st raw_spin_lock_irqsave(&task->pi_lock, flags); if (requeue) { - if (waiter == rt_mutex_top_waiter(lock)) { + if (waiter == top_waiter) { /* Boost the owner */ - rt_mutex_dequeue_pi(task, top_waiter); + rt_mutex_dequeue_pi(task, orig_top_waiter); rt_mutex_enqueue_pi(task, waiter); __rt_mutex_adjust_prio(task); - } else if (top_waiter == waiter) { - /* Deboost the owner */ - rt_mutex_dequeue_pi(task, waiter); - waiter = rt_mutex_top_waiter(lock); - rt_mutex_enqueue_pi(task, waiter); - __rt_mutex_adjust_prio(task); + } else { + if (orig_top_waiter == waiter) { + /* Deboost the owner */ + rt_mutex_dequeue_pi(task, waiter); + waiter = rt_mutex_top_waiter(lock); + rt_mutex_enqueue_pi(task, waiter); + __rt_mutex_adjust_prio(task); + } else { + /* The requeueing did not affect us, no need to boost or deboost */ + } } } raw_spin_unlock_irqrestore(&task->pi_lock, flags); - top_waiter = rt_mutex_top_waiter(lock); raw_spin_unlock(&lock->wait_lock); if (!detect_deadlock && waiter != top_waiter) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-15 6:47 ` Ingo Molnar @ 2014-05-15 21:49 ` Steven Rostedt 0 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2014-05-15 21:49 UTC (permalink / raw) To: Ingo Molnar; +Cc: Thomas Gleixner, LKML, Lai Jiangshan, Peter Zijlstra On Thu, 15 May 2014 08:47:09 +0200 Ingo Molnar <mingo@kernel.org> wrote: > > A couple of suggestions: > > 1) > > * Thomas Gleixner <tglx@linutronix.de> wrote: > > > + if (requeue) { > > + if (waiter == rt_mutex_top_waiter(lock)) { > > So we have a 'top_waiter' local variable already at this point, and we > use it here: > > > + /* Boost the owner */ > > + rt_mutex_dequeue_pi(task, top_waiter); > > + rt_mutex_enqueue_pi(task, waiter); > > + __rt_mutex_adjust_prio(task); > > + > > + } else if (top_waiter == waiter) { > > To me it's a bit confusing that we have both the 'top_waiter' local > variable and also evaluate 'rt_mutex_top_waiter()' directly. > > So what happens is that when we do the requeue, the top waiter might > change. I'd really suggest to signal that via naming - i.e. add > another local variable (which GCC will optimize out happily), named > descriptively: > > orig_top_waiter = top_waiter; > > and use that variable after that point. I wouldn't use "orig_top_waiter" as all the "orig_*" variables are passed in via the parameters and do not change. Maybe: last_top_waiter? > > > + /* Deboost the owner */ > > + rt_mutex_dequeue_pi(task, waiter); > > + waiter = rt_mutex_top_waiter(lock); > > + rt_mutex_enqueue_pi(task, waiter); > > + __rt_mutex_adjust_prio(task); > > + } > > } > > 2) > > Also another small flow of control side comment, because this code is > apparently rather tricky, I'd suggest a bit more explicit structure to > show the real flow of the logic: for example in the first reading of > the above block I mistakenly read it as a usual 'if () { } else { }' > block pattern - which it really isn't. > > Something like this would be slightly easier to understand 'at a > glance', IMHO: > > if (requeue) { > if (waiter == top_waiter) { > /* Boost the owner */ > rt_mutex_dequeue_pi(task, orig_top_waiter); > rt_mutex_enqueue_pi(task, waiter); > __rt_mutex_adjust_prio(task); > > } else { > if (orig_top_waiter == waiter) { > /* Deboost the owner */ > rt_mutex_dequeue_pi(task, waiter); > waiter = rt_mutex_top_waiter(lock); > rt_mutex_enqueue_pi(task, waiter); > __rt_mutex_adjust_prio(task); > } else { > /* The requeueing did not affect us, no need to boost or deboost */ > } > } > } > > Assuming you agree with this structure, it's a bit more verbose, but > this might be one of the cases where verbosity helps readability. > (Note that I already propagated the 'orig_top_waiter' name into it.) Instead of the nesting, what about: if (requeue) { if (waiter == top_waiter) { /* Boost the owner */ rt_mutex_dequeue_pi(task, last_top_waiter); rt_mutex_enqueue_pi(task, waiter); __rt_mutex_adjust_prio(task); } else if (last_top_waiter == waiter) { /* Deboost the owner */ rt_mutex_dequeue_pi(task, waiter); waiter = rt_mutex_top_waiter(lock); rt_mutex_enqueue_pi(task, waiter); __rt_mutex_adjust_prio(task); } else { /* * The requeueing did not affect us, no need to * boost or deboost */ } } That last else should also visually keep one from thinking this is a simple "if { } else { }" construct. (Note that I already propagated the 'last_top_waiter' name into it ;-) > > 3) > > Also note how the code continues: > > raw_spin_unlock_irqrestore(&task->pi_lock, flags); > > top_waiter = rt_mutex_top_waiter(lock); > raw_spin_unlock(&lock->wait_lock); > > if (!detect_deadlock && waiter != top_waiter) > goto out_put_task; > > goto again; > > So we evaluate 'top_waiter' again - maybe we could move that line to > the two branches that actually have a chance to change the top waiter, > and not change it in the 'no need to requeue' case. This is probably a good idea and needs my change I suggested earlier (which doesn't do the wakeup if requeue == 0). -- Steve > > So ... all in one, what I would suggest is something like the patch > below, on top of your two patches. Totally untested and such. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner 2014-05-15 6:47 ` Ingo Molnar @ 2014-05-15 17:04 ` Steven Rostedt 2014-05-20 0:43 ` Thomas Gleixner 2014-05-15 21:39 ` Steven Rostedt 2 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2014-05-15 17:04 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, Lai Jiangshan, Peter Zijlstra, Ingo Molnar On Wed, 14 May 2014 20:03:27 -0000 Thomas Gleixner <tglx@linutronix.de> wrote: > In case the dead lock detector is enabled we follow the lock chain to > the end in rt_mutex_adjust_prio_chain, even if we could stop earlier > due to the priority/waiter constellation. I'm assuming that we want to detect deadlocks for all futex calls even when CONFIG_DEBUG_RT_MUTEXES is set? In kernel/locking/rtmutex_common.h: #ifdef CONFIG_DEBUG_RT_MUTEXES # include "rtmutex-debug.h" #else # include "rtmutex.h" #endif In kernel/locking/rtmutex.h: #define debug_rt_mutex_detect_deadlock(w,d) (d) In kernel/locking/rtmutex.h: static inline int debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter, int detect) { return (waiter != NULL); } Shouldn't that be: return detect || waiter != NULL; ? I know this a separate issue from this patch series, but it's something that I just noticed. > > But once we are not longer the top priority waiter in a certain step "we are no longer the top" > or the task holding the lock has already the same priority then there > is no point in dequeing and enqueing along the lock chain as there is > no change at all. > > So stop the queueing at this point. I'll continue reviewing the patch. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-15 17:04 ` Steven Rostedt @ 2014-05-20 0:43 ` Thomas Gleixner 2014-05-20 1:29 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Thomas Gleixner @ 2014-05-20 0:43 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Lai Jiangshan, Peter Zijlstra, Ingo Molnar On Thu, 15 May 2014, Steven Rostedt wrote: > On Wed, 14 May 2014 20:03:27 -0000 > Thomas Gleixner <tglx@linutronix.de> wrote: > > > In case the dead lock detector is enabled we follow the lock chain to > > the end in rt_mutex_adjust_prio_chain, even if we could stop earlier > > due to the priority/waiter constellation. > > I'm assuming that we want to detect deadlocks for all futex calls > even when CONFIG_DEBUG_RT_MUTEXES is set? > > In kernel/locking/rtmutex_common.h: > > #ifdef CONFIG_DEBUG_RT_MUTEXES > # include "rtmutex-debug.h" > #else > # include "rtmutex.h" > #endif > > In kernel/locking/rtmutex.h: > > #define debug_rt_mutex_detect_deadlock(w,d) (d) > > In kernel/locking/rtmutex.h: > > static inline int debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter, > int detect) > { > return (waiter != NULL); > } > > Shouldn't that be: return detect || waiter != NULL; > No. We do not care about whether the caller handed in detect or not. > > I know this a separate issue from this patch series, but it's > something that I just noticed. It's not really intuitive. We might make the call sites hand in constants. RTMUTEX_DETECT_DEADLOCK, RTMUTEX_IGNORE_DEADLOCK or something like that and switch it depending on CONFIG_DEBUG_RT_MUTEXES. Thanks, tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-20 0:43 ` Thomas Gleixner @ 2014-05-20 1:29 ` Steven Rostedt 0 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2014-05-20 1:29 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, Lai Jiangshan, Peter Zijlstra, Ingo Molnar On Tue, 2014-05-20 at 09:43 +0900, Thomas Gleixner wrote: > In kernel/locking/rtmutex.h: > > > > static inline int debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter, > > int detect) > > { > > return (waiter != NULL); > > } > > > > Shouldn't that be: return detect || waiter != NULL; > > > > No. We do not care about whether the caller handed in detect or not. Bah, you're right. I was getting confused between try_to_take_rt_mutex() and task_blocks_on_rt_mutex(). The former passes in a NULL waiter when taking the first time, but it's the task_blocks_on_rt_mutex() that does the test, and yes, the waiter is never NULL when boosting. I may send you a patch to add comments to these little idiosyncrasies. > > > > > I know this a separate issue from this patch series, but it's > > something that I just noticed. > > It's not really intuitive. We might make the call sites hand in > constants. RTMUTEX_DETECT_DEADLOCK, RTMUTEX_IGNORE_DEADLOCK or > something like that and switch it depending on > CONFIG_DEBUG_RT_MUTEXES. Yes it is confusing. I use to know this code really well, and now over the years, it's not something to look at quickly and produce a comprehensive response. The sad part is, I looked at it quite a bit before sending my response and I still got confused :-p -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner 2014-05-15 6:47 ` Ingo Molnar 2014-05-15 17:04 ` Steven Rostedt @ 2014-05-15 21:39 ` Steven Rostedt 2 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2014-05-15 21:39 UTC (permalink / raw) To: Thomas Gleixner; +Cc: LKML, Lai Jiangshan, Peter Zijlstra, Ingo Molnar On Wed, 14 May 2014 20:03:27 -0000 Thomas Gleixner <tglx@linutronix.de> wrote: > /* 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. > + */ > 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)) > wake_up_process(rt_mutex_top_waiter(lock)->task); Seems we can change the above if condition to: if (requeue && top_waiter != rt_mutex_top_waiter(lock)) as there was no "requeue above" that could change anything. -- Steve > raw_spin_unlock(&lock->wait_lock); > @@ -404,18 +413,20 @@ static int rt_mutex_adjust_prio_chain(st > get_task_struct(task); > 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); > - rt_mutex_enqueue_pi(task, waiter); > - __rt_mutex_adjust_prio(task); > - > - } else if (top_waiter == waiter) { > - /* Deboost the owner */ > - rt_mutex_dequeue_pi(task, waiter); > - waiter = rt_mutex_top_waiter(lock); > - rt_mutex_enqueue_pi(task, waiter); > - __rt_mutex_adjust_prio(task); > + if (requeue) { > + if (waiter == rt_mutex_top_waiter(lock)) { > + /* Boost the owner */ > + rt_mutex_dequeue_pi(task, top_waiter); > + rt_mutex_enqueue_pi(task, waiter); > + __rt_mutex_adjust_prio(task); > + > + } else if (top_waiter == waiter) { > + /* Deboost the owner */ > + rt_mutex_dequeue_pi(task, waiter); > + waiter = rt_mutex_top_waiter(lock); > + rt_mutex_enqueue_pi(task, waiter); > + __rt_mutex_adjust_prio(task); > + } > } > > raw_spin_unlock_irqrestore(&task->pi_lock, flags); > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-05-20 1:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-14 20:03 [patch 0/2] rtmutex: Fix the deadlock detector for real Thomas Gleixner 2014-05-14 20:03 ` [patch 1/2] rtmutex: Fix " Thomas Gleixner 2014-05-14 20:03 ` [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk Thomas Gleixner 2014-05-15 6:47 ` Ingo Molnar 2014-05-15 21:49 ` Steven Rostedt 2014-05-15 17:04 ` Steven Rostedt 2014-05-20 0:43 ` Thomas Gleixner 2014-05-20 1:29 ` Steven Rostedt 2014-05-15 21:39 ` Steven Rostedt
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).