Re: [PATCH] lockdep: Have assert functions test for actual interrupts disabled

[Peter Zijlstra]

On Thu, Sep 06, 2018 at 10:17:01AM -0400, Steven Rostedt wrote:
> I still think checking if IRQS are really disabled or not when lockdep
> thinks it is (or not) is valuable and doesn't cause any other problems.

Since check_flags() is a relatively cheap thing I would rather do
something like so..

---
 include/linux/lockdep.h  |  6 ++++--
 kernel/locking/lockdep.c | 36 +++++++++++++++++++++++-------------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index b0d0b51c4d85..24ff6302c04b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -596,15 +596,17 @@ do {									\
 	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
 } while (0)
 
+extern bool lockdep_check_flags(void);
+
 #define lockdep_assert_irqs_enabled()	do {				\
 		WARN_ONCE(debug_locks && !current->lockdep_recursion &&	\
-			  !current->hardirqs_enabled,			\
+			  !current->hardirqs_enabled && lockdep_check_flags(), \
 			  "IRQs not enabled as expected\n");		\
 	} while (0)
 
 #define lockdep_assert_irqs_disabled()	do {				\
 		WARN_ONCE(debug_locks && !current->lockdep_recursion &&	\
-			  current->hardirqs_enabled,			\
+			  current->hardirqs_enabled && lockdep_check_flags(), \
 			  "IRQs not disabled as expected\n");		\
 	} while (0)
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e406c5fdb41e..6cc58a16f2fe 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3807,10 +3807,9 @@ static void __lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie
 /*
  * Check whether we follow the irq-flags state precisely:
  */
-static void check_flags(unsigned long flags)
+void __lockdep_check_flags(unsigned long flags)
 {
-#if defined(CONFIG_PROVE_LOCKING) && defined(CONFIG_DEBUG_LOCKDEP) && \
-    defined(CONFIG_TRACE_IRQFLAGS)
+#if defined(CONFIG_PROVE_LOCKING) && defined(CONFIG_TRACE_IRQFLAGS)
 	if (!debug_locks)
 		return;
 
@@ -3844,6 +3843,17 @@ static void check_flags(unsigned long flags)
 #endif
 }
 
+bool lockdep_check_flags(void)
+{
+	unsigned long flags;
+
+	raw_local_irq_save(flags);
+	__lockdep_check_flags(flags);
+	raw_local_irq_restore(flags);
+
+	return true;
+}
+
 void lock_set_class(struct lockdep_map *lock, const char *name,
 		    struct lock_class_key *key, unsigned int subclass,
 		    unsigned long ip)
@@ -3855,7 +3865,7 @@ void lock_set_class(struct lockdep_map *lock, const char *name,
 
 	raw_local_irq_save(flags);
 	current->lockdep_recursion = 1;
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 	if (__lock_set_class(lock, name, key, subclass, ip))
 		check_chain_key(current);
 	current->lockdep_recursion = 0;
@@ -3872,7 +3882,7 @@ void lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 
 	raw_local_irq_save(flags);
 	current->lockdep_recursion = 1;
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 	if (__lock_downgrade(lock, ip))
 		check_chain_key(current);
 	current->lockdep_recursion = 0;
@@ -3894,7 +3904,7 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 
 	current->lockdep_recursion = 1;
 	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
@@ -3914,7 +3924,7 @@ void lock_release(struct lockdep_map *lock, int nested,
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 	current->lockdep_recursion = 1;
 	trace_lock_release(lock, ip);
 	if (__lock_release(lock, nested, ip))
@@ -3933,7 +3943,7 @@ int lock_is_held_type(const struct lockdep_map *lock, int read)
 		return 1; /* avoid false negative lockdep_assert_held() */
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 
 	current->lockdep_recursion = 1;
 	ret = __lock_is_held(lock, read);
@@ -3953,7 +3963,7 @@ struct pin_cookie lock_pin_lock(struct lockdep_map *lock)
 		return cookie;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 
 	current->lockdep_recursion = 1;
 	cookie = __lock_pin_lock(lock);
@@ -3972,7 +3982,7 @@ void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 
 	current->lockdep_recursion = 1;
 	__lock_repin_lock(lock, cookie);
@@ -3989,7 +3999,7 @@ void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie cookie)
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 
 	current->lockdep_recursion = 1;
 	__lock_unpin_lock(lock, cookie);
@@ -4130,7 +4140,7 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 	current->lockdep_recursion = 1;
 	trace_lock_contended(lock, ip);
 	__lock_contended(lock, ip);
@@ -4150,7 +4160,7 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
 		return;
 
 	raw_local_irq_save(flags);
-	check_flags(flags);
+	__lockdep_check_flags(flags);
 	current->lockdep_recursion = 1;
 	__lock_acquired(lock, ip);
 	current->lockdep_recursion = 0;