Re: [patch V4 09/10] rtmutex: Cleanup deadlock detector debug logic
From: Steven Rostedt
Date: Fri Jun 13 2014 - 13:19:38 EST
On Wed, 11 Jun 2014 18:44:08 -0000
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> The conditions under which deadlock detection is conducted are unclear
> and undocumented.
>
> Add constants instead of using 0/1 and provide a selection function
> which hides the additional debug dependency from the calling code.
>
> Add comments where needed.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
> Link: http://lkml.kernel.org/r/20140522031949.947264874@xxxxxxxxxxxxx
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> kernel/locking/rtmutex-debug.c | 5 +-
> kernel/locking/rtmutex-debug.h | 7 ++--
> kernel/locking/rtmutex.c | 70 +++++++++++++++++++++++++++-------------
> kernel/locking/rtmutex.h | 7 +++-
> kernel/locking/rtmutex_common.h | 15 ++++++++
> 5 files changed, 76 insertions(+), 28 deletions(-)
>
> Index: tip/kernel/locking/rtmutex-debug.c
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex-debug.c
> +++ tip/kernel/locking/rtmutex-debug.c
> @@ -66,12 +66,13 @@ void rt_mutex_debug_task_free(struct tas
> * the deadlock. We print when we return. act_waiter can be NULL in
> * case of a remove waiter operation.
> */
> -void debug_rt_mutex_deadlock(int detect, struct rt_mutex_waiter *act_waiter,
> +void debug_rt_mutex_deadlock(enum rtmutex_chainwalk chwalk,
> + struct rt_mutex_waiter *act_waiter,
> struct rt_mutex *lock)
> {
> struct task_struct *task;
>
> - if (!debug_locks || detect || !act_waiter)
> + if (!debug_locks || chwalk == RT_MUTEX_FULL_CHAINWALK || !act_waiter)
> return;
>
> task = rt_mutex_owner(act_waiter->lock);
> Index: tip/kernel/locking/rtmutex-debug.h
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex-debug.h
> +++ tip/kernel/locking/rtmutex-debug.h
> @@ -20,14 +20,15 @@ extern void debug_rt_mutex_unlock(struct
> extern void debug_rt_mutex_proxy_lock(struct rt_mutex *lock,
> struct task_struct *powner);
> extern void debug_rt_mutex_proxy_unlock(struct rt_mutex *lock);
> -extern void debug_rt_mutex_deadlock(int detect, struct rt_mutex_waiter *waiter,
> +extern void debug_rt_mutex_deadlock(enum rtmutex_chainwalk chwalk,
> + struct rt_mutex_waiter *waiter,
> struct rt_mutex *lock);
> extern void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter);
> # define debug_rt_mutex_reset_waiter(w) \
> do { (w)->deadlock_lock = NULL; } while (0)
>
> -static inline int debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter,
> - int detect)
> +static inline bool debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *waiter,
> + enum rtmutex_chainwalk walk)
> {
> return (waiter != NULL);
> }
> Index: tip/kernel/locking/rtmutex.c
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex.c
> +++ tip/kernel/locking/rtmutex.c
> @@ -308,6 +308,25 @@ static void rt_mutex_adjust_prio(struct
> }
>
> /*
> + * Deadlock detection is conditional:
> + *
> + * If CONFIG_DEBUG_RT_MUTEXES=n, deadlock detection is only conducted
> + * if the detect argument is == RT_MUTEX_FULL_CHAINWALK.
> + *
> + * If CONFIG_DEBUG_RT_MUTEXES=y, deadlock detection is always
> + * conducted independent of the detect argument.
> + *
> + * If the waiter argument is NULL this indicates the deboost path and
> + * deadlock detection is disabled independent of the detect argument
> + * and the config settings.
> + */
> +static bool rt_mutex_cond_detect_deadlock(struct rt_mutex_waiter *waiter,
> + enum rtmutex_chainwalk chwalk)
> +{
/*
* This is just a wrapper function for the following call,
* because debug_rt_mutex_detect_deadlock() smells like a magic
* debug feature and I wanted to keep the cond function in the
* main source file along with the comments instead of having
* two of the same in the headers.
*/
;-)
(found an error below)
> + return debug_rt_mutex_detect_deadlock(waiter, chwalk);
> +}
> +
> +/*
> * Max number of times we'll walk the boosting chain:
> */
> int max_lock_depth = 1024;
> @@ -381,7 +400,7 @@ static inline struct rt_mutex *task_bloc
> * goto again;
> */
> static int rt_mutex_adjust_prio_chain(struct task_struct *task,
> - int deadlock_detect,
> + enum rtmutex_chainwalk chwalk,
> struct rt_mutex *orig_lock,
> struct rt_mutex *next_lock,
> struct rt_mutex_waiter *orig_waiter,
> @@ -389,12 +408,12 @@ static int rt_mutex_adjust_prio_chain(st
> {
> struct rt_mutex_waiter *waiter, *top_waiter = orig_waiter;
> struct rt_mutex_waiter *prerequeue_top_waiter;
> - int detect_deadlock, ret = 0, depth = 0;
> + int ret = 0, depth = 0;
> struct rt_mutex *lock;
> + bool detect_deadlock;
> unsigned long flags;
>
> - detect_deadlock = debug_rt_mutex_detect_deadlock(orig_waiter,
> - deadlock_detect);
> + detect_deadlock = rt_mutex_cond_detect_deadlock(orig_waiter, chwalk);
>
> /*
> * The (de)boosting is a step by step approach with a lot of
> @@ -520,7 +539,7 @@ static int rt_mutex_adjust_prio_chain(st
> * walk, we detected a deadlock.
> */
> if (lock == orig_lock || rt_mutex_owner(lock) == top_task) {
> - debug_rt_mutex_deadlock(deadlock_detect, orig_waiter, lock);
> + debug_rt_mutex_deadlock(chwalk, orig_waiter, lock);
> raw_spin_unlock(&lock->wait_lock);
> ret = -EDEADLK;
> goto out_unlock_pi;
> @@ -784,7 +803,7 @@ takeit:
> static int task_blocks_on_rt_mutex(struct rt_mutex *lock,
> struct rt_mutex_waiter *waiter,
> struct task_struct *task,
> - int detect_deadlock)
> + enum rtmutex_chainwalk chwalk)
> {
> struct task_struct *owner = rt_mutex_owner(lock);
> struct rt_mutex_waiter *top_waiter = waiter;
> @@ -830,7 +849,7 @@ static int task_blocks_on_rt_mutex(struc
> __rt_mutex_adjust_prio(owner);
> if (owner->pi_blocked_on)
> chain_walk = 1;
> - } else if (debug_rt_mutex_detect_deadlock(waiter, detect_deadlock)) {
> + } else if (rt_mutex_cond_detect_deadlock(waiter, chwalk)) {
> chain_walk = 1;
> }
>
> @@ -855,7 +874,7 @@ static int task_blocks_on_rt_mutex(struc
>
> raw_spin_unlock(&lock->wait_lock);
>
> - res = rt_mutex_adjust_prio_chain(owner, detect_deadlock, lock,
> + res = rt_mutex_adjust_prio_chain(owner, chwalk, lock,
> next_lock, waiter, task);
>
> raw_spin_lock(&lock->wait_lock);
> @@ -963,7 +982,8 @@ static void remove_waiter(struct rt_mute
>
> raw_spin_unlock(&lock->wait_lock);
>
> - rt_mutex_adjust_prio_chain(owner, 0, lock, next_lock, NULL, current);
> + rt_mutex_adjust_prio_chain(owner, RT_MUTEX_MIN_CHAINWALK, lock,
> + next_lock, NULL, current);
>
> raw_spin_lock(&lock->wait_lock);
> }
> @@ -993,7 +1013,8 @@ void rt_mutex_adjust_pi(struct task_stru
> /* gets dropped in rt_mutex_adjust_prio_chain()! */
> get_task_struct(task);
>
> - rt_mutex_adjust_prio_chain(task, 0, NULL, next_lock, NULL, task);
> + rt_mutex_adjust_prio_chain(task, RT_MUTEX_MIN_CHAINWALK, NULL, NULL,
You dropped next_lock here. Was that intentional? I doubt it.
-- Steve
> + NULL, task);
> }
>
> /**
> @@ -1071,7 +1092,7 @@ static void rt_mutex_handle_deadlock(int
> static int __sched
> rt_mutex_slowlock(struct rt_mutex *lock, int state,
> struct hrtimer_sleeper *timeout,
> - int detect_deadlock)
> + enum rtmutex_chainwalk chwalk)
> {
> struct rt_mutex_waiter waiter;
> int ret = 0;
> @@ -1097,7 +1118,7 @@ rt_mutex_slowlock(struct rt_mutex *lock,
> timeout->task = NULL;
> }
>
> - ret = task_blocks_on_rt_mutex(lock, &waiter, current, detect_deadlock);
> + ret = task_blocks_on_rt_mutex(lock, &waiter, current, chwalk);
>
> if (likely(!ret))
> ret = __rt_mutex_slowlock(lock, state, timeout, &waiter);
> @@ -1106,7 +1127,7 @@ rt_mutex_slowlock(struct rt_mutex *lock,
>
> if (unlikely(ret)) {
> remove_waiter(lock, &waiter);
> - rt_mutex_handle_deadlock(ret, detect_deadlock, &waiter);
> + rt_mutex_handle_deadlock(ret, chwalk, &waiter);
> }
>
> /*
> @@ -1234,27 +1255,29 @@ static inline int
> rt_mutex_fastlock(struct rt_mutex *lock, int state,
> int (*slowfn)(struct rt_mutex *lock, int state,
> struct hrtimer_sleeper *timeout,
> - int detect_deadlock))
> + enum rtmutex_chainwalk chwalk))
> {
> if (likely(rt_mutex_cmpxchg(lock, NULL, current))) {
> rt_mutex_deadlock_account_lock(lock, current);
> return 0;
> } else
> - return slowfn(lock, state, NULL, 0);
> + return slowfn(lock, state, NULL, RT_MUTEX_MIN_CHAINWALK);
> }
>
> static inline int
> rt_mutex_timed_fastlock(struct rt_mutex *lock, int state,
> - struct hrtimer_sleeper *timeout, int detect_deadlock,
> + struct hrtimer_sleeper *timeout,
> + enum rtmutex_chainwalk chwalk,
> int (*slowfn)(struct rt_mutex *lock, int state,
> struct hrtimer_sleeper *timeout,
> - int detect_deadlock))
> + enum rtmutex_chainwalk chwalk))
> {
> - if (!detect_deadlock && likely(rt_mutex_cmpxchg(lock, NULL, current))) {
> + if (chwalk == RT_MUTEX_MIN_CHAINWALK &&
> + likely(rt_mutex_cmpxchg(lock, NULL, current))) {
> rt_mutex_deadlock_account_lock(lock, current);
> return 0;
> } else
> - return slowfn(lock, state, timeout, detect_deadlock);
> + return slowfn(lock, state, timeout, chwalk);
> }
>
> static inline int
> @@ -1316,7 +1339,8 @@ int __rt_mutex_timed_lock(struct rt_mute
> {
> might_sleep();
>
> - return rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout, 1,
> + return rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout,
> + RT_MUTEX_FULL_CHAINWALK,
> rt_mutex_slowlock);
> }
>
> @@ -1338,7 +1362,8 @@ rt_mutex_timed_lock(struct rt_mutex *loc
> {
> might_sleep();
>
> - return rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout, 0,
> + return rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout,
> + RT_MUTEX_MIN_CHAINWALK,
> rt_mutex_slowlock);
> }
> EXPORT_SYMBOL_GPL(rt_mutex_timed_lock);
> @@ -1467,7 +1492,8 @@ int rt_mutex_start_proxy_lock(struct rt_
> }
>
> /* We enforce deadlock detection for futexes */
> - ret = task_blocks_on_rt_mutex(lock, waiter, task, 1);
> + ret = task_blocks_on_rt_mutex(lock, waiter, task,
> + RT_MUTEX_FULL_CHAINWALK);
>
> if (ret && !rt_mutex_owner(lock)) {
> /*
> Index: tip/kernel/locking/rtmutex.h
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex.h
> +++ tip/kernel/locking/rtmutex.h
> @@ -22,10 +22,15 @@
> #define debug_rt_mutex_init(m, n) do { } while (0)
> #define debug_rt_mutex_deadlock(d, a ,l) do { } while (0)
> #define debug_rt_mutex_print_deadlock(w) do { } while (0)
> -#define debug_rt_mutex_detect_deadlock(w,d) (d)
> #define debug_rt_mutex_reset_waiter(w) do { } while (0)
>
> static inline void rt_mutex_print_deadlock(struct rt_mutex_waiter *w)
> {
> WARN(1, "rtmutex deadlock detected\n");
> }
> +
> +static inline bool debug_rt_mutex_detect_deadlock(struct rt_mutex_waiter *w,
> + enum rtmutex_chainwalk walk)
> +{
> + return walk == RT_MUTEX_FULL_CHAINWALK;
> +}
> Index: tip/kernel/locking/rtmutex_common.h
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex_common.h
> +++ tip/kernel/locking/rtmutex_common.h
> @@ -102,6 +102,21 @@ static inline struct task_struct *rt_mut
> }
>
> /*
> + * Constants for rt mutex functions which have a selectable deadlock
> + * detection.
> + *
> + * RT_MUTEX_MIN_CHAINWALK: Stops the lock chain walk when there are
> + * no further PI adjustments to be made.
> + *
> + * RT_MUTEX_FULL_CHAINWALK: Invoke deadlock detection with a full
> + * walk of the lock chain.
> + */
> +enum rtmutex_chainwalk {
> + RT_MUTEX_MIN_CHAINWALK,
> + RT_MUTEX_FULL_CHAINWALK,
> +};
> +
> +/*
> * PI-futex support (proxy locking functions, etc.):
> */
> extern struct task_struct *rt_mutex_next_owner(struct rt_mutex *lock);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/