[patch 1/2] rtmutex: Fix deadlock detector for real

[patch 1/2] rtmutex: Fix deadlock detector for real

[Thomas Gleixner]

[-- 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;

[patch 0/2] rtmutex: Fix the deadlock detector for real

[Thomas Gleixner]

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

[patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Thomas Gleixner]

[-- 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);

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Ingo Molnar]

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)

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Steven Rostedt]

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

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Steven Rostedt]

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);
> 

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Steven Rostedt]

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.
> 

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Thomas Gleixner]

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

Re: [patch 2/2] rtmutex: Avoid pointless requeueing in the deadlock detection chain walk

[Steven Rostedt]

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