rtmutex: Fix the deadlock detector for real

A couple of suggestions:

1)

Review

Quoted 2 lines · Thomas Gleixner

(quoted diff) +2 -0

+	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:

Review

Quoted 6 lines

(quoted diff) +6 -0

+			/* 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.

kernel/locking/rtmutex.c static int rt_mutex_adjust_prio_chain(st · line 437

Quoted 8 lines

kernel/locking/rtmutex.c +6 -0

+			/* 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.

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.

Quoted 3 lines · On Wed, 14 May 2014 20:03:27 -0000 Thomas Gleixner <[email protected]> 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.

In kernel/locking/rtmutex.h:

Quoted 7 lines

static inline int debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter,
int detect)

{
        return (waiter != NULL);
}

Quoted 2 lines

Shouldn't that be: return detect || waiter != NULL;

No. We do not care about whether the caller handed in detect or not.

Quoted 4 lines

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.

kernel/locking/rtmutex.c +3 -0

 	/* 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

kernel/locking/rtmutex.c +14 -12

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