Skip to contentSkip to conversation

rt_mutex: Add lockdep annotations

RFC PATCH In review Peter Zijlstra 2 participants 6 messages May 31, 2017 – Jun 6, 2017

Subsystems (2)
Library code Locking primitives
lore
Share & tools
lore

Peter Zijlstra

author maintainer
#0 lore raw

cover rt_mutex: Add lockdep annotations

Something that should've been done from the start...

---
 include/linux/rtmutex.h          |   25 ++++++-
 kernel/locking/rtmutex-debug.c   |    6 +
 kernel/locking/rtmutex-debug.h   |    2 
 kernel/locking/rtmutex.c         |   36 ++++++++--
 kernel/locking/rtmutex.h         |    2 
 lib/Kconfig.debug                |    3 
 lib/locking-selftest.c           |  133 ++++++++++++++++++++++++++++++---------
 lib/locking-selftest-rtmutex.h   |   11 +++
 8 files changed, 176 insertions(+), 42 deletions(-)
author maintainer
#1 lore raw

RFC PATCH 1/3 rt_mutex: Add lockdep annotations

Now that (PI) futexes have their own private RT-mutex interface and implementation we can easily add lockdep annotations to the existing RT-mutex interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rtmutex.h        |   25 +++++++++++++++++++++----
 kernel/locking/rtmutex-debug.c |    6 +++++-
 kernel/locking/rtmutex-debug.h |    2 +-
 kernel/locking/rtmutex.c       |   36 +++++++++++++++++++++++++++++-------
 kernel/locking/rtmutex.h       |    2 +-
 lib/Kconfig.debug              |    3 +++
 6 files changed, 60 insertions(+), 14 deletions(-)
include/linux/rtmutex.h +21 -4
@@ -37,6 +37,9 @@ struct rt_mutex {
 	int			line;
 	void			*magic;
 #endif
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lockdep_map	dep_map;
+#endif
 };
 
 struct rt_mutex_waiter;

@@ -58,19 +61,33 @@ struct hrtimer_sleeper;
 #ifdef CONFIG_DEBUG_RT_MUTEXES
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
 	, .name = #mutexname, .file = __FILE__, .line = __LINE__
-# define rt_mutex_init(mutex)			__rt_mutex_init(mutex, __func__)
+
+# define rt_mutex_init(mutex) \
+do { \
+	static struct lock_class_key __key; \
+	__rt_mutex_init(mutex, __func__, &__key); \
+} while (0)
+
  extern void rt_mutex_debug_task_free(struct task_struct *tsk);
 #else
 # define __DEBUG_RT_MUTEX_INITIALIZER(mutexname)
-# define rt_mutex_init(mutex)			__rt_mutex_init(mutex, NULL)
+# define rt_mutex_init(mutex)			__rt_mutex_init(mutex, NULL, NULL)
 # define rt_mutex_debug_task_free(t)			do { } while (0)
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define __DEP_MAP_RT_MUTEX_INITIALIZER(mutexname) \
+	, .dep_map = { .name = #mutexname }
+#else
+#define __DEP_MAP_RT_MUTEX_INITIALIZER(mutexname)
+#endif
+
 #define __RT_MUTEX_INITIALIZER(mutexname) \
 	{ .wait_lock = __RAW_SPIN_LOCK_UNLOCKED(mutexname.wait_lock) \
 	, .waiters = RB_ROOT \
 	, .owner = NULL \
-	__DEBUG_RT_MUTEX_INITIALIZER(mutexname)}
+	__DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
+	__DEP_MAP_RT_MUTEX_INITIALIZER(mutexname)}
 
 #define DEFINE_RT_MUTEX(mutexname) \
 	struct rt_mutex mutexname = __RT_MUTEX_INITIALIZER(mutexname)

@@ -86,7 +103,7 @@ static inline int rt_mutex_is_locked(str
 	return lock->owner != NULL;
 }
 
-extern void __rt_mutex_init(struct rt_mutex *lock, const char *name);
+extern void __rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key);
 extern void rt_mutex_destroy(struct rt_mutex *lock);
 
 extern void rt_mutex_lock(struct rt_mutex *lock);
kernel/locking/rtmutex-debug.c +5 -1
@@ -166,12 +166,16 @@ void debug_rt_mutex_free_waiter(struct r
 	memset(waiter, 0x22, sizeof(*waiter));
 }
 
-void debug_rt_mutex_init(struct rt_mutex *lock, const char *name)
+void debug_rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key)
 {
 	/*
 	 * Make sure we are not reinitializing a held lock:
 	 */
 	debug_check_no_locks_freed((void *)lock, sizeof(*lock));
 	lock->name = name;
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	lockdep_init_map(&lock->dep_map, name, key, 0);
+#endif
 }
kernel/locking/rtmutex-debug.h +1 -1
@@ -11,7 +11,7 @@
 
 extern void debug_rt_mutex_init_waiter(struct rt_mutex_waiter *waiter);
 extern void debug_rt_mutex_free_waiter(struct rt_mutex_waiter *waiter);
-extern void debug_rt_mutex_init(struct rt_mutex *lock, const char *name);
+extern void debug_rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key);
 extern void debug_rt_mutex_lock(struct rt_mutex *lock);
 extern void debug_rt_mutex_unlock(struct rt_mutex *lock);
 extern void debug_rt_mutex_proxy_lock(struct rt_mutex *lock,
kernel/locking/rtmutex.c +29 -7
@@ -1481,6 +1481,7 @@ void __sched rt_mutex_lock(struct rt_mut
 {
 	might_sleep();
 
+	mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
 	rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
 }
 EXPORT_SYMBOL_GPL(rt_mutex_lock);

@@ -1496,9 +1497,16 @@ EXPORT_SYMBOL_GPL(rt_mutex_lock);
  */
 int __sched rt_mutex_lock_interruptible(struct rt_mutex *lock)
 {
+	int ret;
+
 	might_sleep();
 
-	return rt_mutex_fastlock(lock, TASK_INTERRUPTIBLE, rt_mutex_slowlock);
+	mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	ret = rt_mutex_fastlock(lock, TASK_INTERRUPTIBLE, rt_mutex_slowlock);
+	if (ret)
+		mutex_release(&lock->dep_map, 1, _RET_IP_);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(rt_mutex_lock_interruptible);
 

@@ -1526,11 +1534,18 @@ int __sched rt_mutex_futex_trylock(struc
 int
 rt_mutex_timed_lock(struct rt_mutex *lock, struct hrtimer_sleeper *timeout)
 {
+	int ret;
+
 	might_sleep();
 
-	return rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout,
+	mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	ret = rt_mutex_timed_fastlock(lock, TASK_INTERRUPTIBLE, timeout,
 				       RT_MUTEX_MIN_CHAINWALK,
 				       rt_mutex_slowlock);
+	if (ret)
+		mutex_release(&lock->dep_map, 1, _RET_IP_);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(rt_mutex_timed_lock);
 

@@ -1547,10 +1562,16 @@ EXPORT_SYMBOL_GPL(rt_mutex_timed_lock);
  */
 int __sched rt_mutex_trylock(struct rt_mutex *lock)
 {
+	int ret;
+
 	if (WARN_ON_ONCE(in_irq() || in_nmi() || in_serving_softirq()))
 		return 0;
 
-	return rt_mutex_fasttrylock(lock, rt_mutex_slowtrylock);
+	ret = rt_mutex_fasttrylock(lock, rt_mutex_slowtrylock);
+	if (ret)
+		mutex_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(rt_mutex_trylock);
 

@@ -1561,6 +1582,7 @@ EXPORT_SYMBOL_GPL(rt_mutex_trylock);
  */
 void __sched rt_mutex_unlock(struct rt_mutex *lock)
 {
+	mutex_release(&lock->dep_map, 1, _RET_IP_);
 	rt_mutex_fastunlock(lock, rt_mutex_slowunlock);
 }
 EXPORT_SYMBOL_GPL(rt_mutex_unlock);

@@ -1620,7 +1642,6 @@ void rt_mutex_destroy(struct rt_mutex *l
 	lock->magic = NULL;
 #endif
 }
-
 EXPORT_SYMBOL_GPL(rt_mutex_destroy);
 
 /**

@@ -1632,14 +1653,15 @@ EXPORT_SYMBOL_GPL(rt_mutex_destroy);
  *
  * Initializing of a locked rt lock is not allowed
  */
-void __rt_mutex_init(struct rt_mutex *lock, const char *name)
+void __rt_mutex_init(struct rt_mutex *lock, const char *name,
+		     struct lock_class_key *key)
 {
 	lock->owner = NULL;
 	raw_spin_lock_init(&lock->wait_lock);
 	lock->waiters = RB_ROOT;
 	lock->waiters_leftmost = NULL;
 
-	debug_rt_mutex_init(lock, name);
+	debug_rt_mutex_init(lock, name, key);
 }
 EXPORT_SYMBOL_GPL(__rt_mutex_init);
 

@@ -1660,7 +1682,7 @@ EXPORT_SYMBOL_GPL(__rt_mutex_init);
 void rt_mutex_init_proxy_locked(struct rt_mutex *lock,
 				struct task_struct *proxy_owner)
 {
-	__rt_mutex_init(lock, NULL);
+	__rt_mutex_init(lock, NULL, NULL);
 	debug_rt_mutex_proxy_lock(lock, proxy_owner);
 	rt_mutex_set_owner(lock, proxy_owner);
 }
kernel/locking/rtmutex.h +1 -1
@@ -17,7 +17,7 @@
 #define debug_rt_mutex_proxy_lock(l,p)			do { } while (0)
 #define debug_rt_mutex_proxy_unlock(l)			do { } while (0)
 #define debug_rt_mutex_unlock(l)			do { } while (0)
-#define debug_rt_mutex_init(m, n)			do { } while (0)
+#define debug_rt_mutex_init(m, n, k)			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_reset_waiter(w)			do { } while (0)
lib/Kconfig.debug +3 -0
@@ -1052,6 +1052,7 @@ config DEBUG_LOCK_ALLOC
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
+	select DEBUG_RT_MUTEXES if RT_MUTEXES
 	select LOCKDEP
 	help
 	 This feature will check whether any held lock (spinlock, rwlock,

@@ -1067,6 +1068,7 @@ config PROVE_LOCKING
 	select LOCKDEP
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
+	select DEBUG_RT_MUTEXES if RT_MUTEXES
 	select DEBUG_LOCK_ALLOC
 	select TRACE_IRQFLAGS
 	default n

@@ -1121,6 +1123,7 @@ config LOCK_STAT
 	select LOCKDEP
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
+	select DEBUG_RT_MUTEXES if RT_MUTEXES
 	select DEBUG_LOCK_ALLOC
 	default n
 	help
#2 lore raw
Quoted 8 lines · Peter Zijlstra

Now that (PI) futexes have their own private RT-mutex interface and implementation we can easily add lockdep annotations to the existing RT-mutex interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rtmutex.h        |   25 +++++++++++++++++++++----
 kernel/locking/rtmutex-debug.c |    6 +++++-

Shouldn't we get rid of all that home brewn rtmutex debug muck?

Thanks,

tglx

author maintainer
#3 lore raw
Quoted 6 lines · Thomas Gleixner
Quoted 4 lines · Peter Zijlstra

Now that (PI) futexes have their own private RT-mutex interface and implementation we can easily add lockdep annotations to the existing RT-mutex interface.

Quoted 6 lines
Quoted 4 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rtmutex.h        |   25 +++++++++++++++++++++----
 kernel/locking/rtmutex-debug.c |    6 +++++-

Shouldn't we get rid of all that home brewn rtmutex debug muck?

mutex also still has its mutex-debug.* stuff. It validates a little more state. Also, and I think this is the main benefit, it has the magic bits in that detect corruption without the full space/time costs of lockdep.

Maybe we can trim the rtmutex-debug a wee bit, but then futexes don't get that coverage.

I'll put looking at this on the todo list.

author maintainer
#4 lore raw

RFC PATCH 2/3 locking/selftest: Remove the bad unlock ordering test

In reply to Peter Zijlstra

There is no such thing as a bad unlock order.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 lib/locking-selftest.c |   29 -----------------------------
 1 file changed, 29 deletions(-)
lib/locking-selftest.c +0 -29
@@ -516,34 +516,6 @@ GENERATE_TESTCASE(double_unlock_rsem)
 #undef E
 
 /*
- * Bad unlock ordering:
- */
-#define E()					\
-						\
-	LOCK(A);				\
-	LOCK(B);				\
-	UNLOCK(A); /* fail */			\
-	UNLOCK(B);
-
-/*
- * 6 testcases:
- */
-#include "locking-selftest-spin.h"
-GENERATE_TESTCASE(bad_unlock_order_spin)
-#include "locking-selftest-wlock.h"
-GENERATE_TESTCASE(bad_unlock_order_wlock)
-#include "locking-selftest-rlock.h"
-GENERATE_TESTCASE(bad_unlock_order_rlock)
-#include "locking-selftest-mutex.h"
-GENERATE_TESTCASE(bad_unlock_order_mutex)
-#include "locking-selftest-wsem.h"
-GENERATE_TESTCASE(bad_unlock_order_wsem)
-#include "locking-selftest-rsem.h"
-GENERATE_TESTCASE(bad_unlock_order_rsem)
-
-#undef E
-
-/*
  * initializing a held lock:
  */
 #define E()					\

@@ -1825,7 +1797,6 @@ void locking_selftest(void)
 	DO_TESTCASE_6R("A-B-C-D-B-C-D-A deadlock", ABCDBCDA);
 	DO_TESTCASE_6("double unlock", double_unlock);
 	DO_TESTCASE_6("initialize held", init_held);
-	DO_TESTCASE_6_SUCCESS("bad unlock order", bad_unlock_order);
 
 	printk("  --------------------------------------------------------------------------\n");
 	print_testname("recursive read-lock");
author maintainer
#5 lore raw

RFC PATCH 3/3 locking/selftest: Add RT-mutex support

In reply to Peter Zijlstra

Now that RT-mutex has lockdep annotations, add them to the selftest.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 lib/locking-selftest-rtmutex.h |   11 ++++
 lib/locking-selftest.c         |  106 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
lib/locking-selftest-rtmutex.h new file +11 -0
@@ -0,0 +1,11 @@
+#undef LOCK
+#define LOCK		RTL
+
+#undef UNLOCK
+#define UNLOCK		RTU
+
+#undef RLOCK
+#undef WLOCK
+
+#undef INIT
+#define INIT		RTI
lib/locking-selftest.c +106 -0

244 lines not on this page · raw message