Q. lockdep_assert_held() and lockdep_off/on()

Q. lockdep_assert_held() and lockdep_off/on()

[J. R. Okajima]

Hello Peter Zijlstra and Ingo Molnar,

May I ask you a question about the commit
	f607c66 2009-08-02 lockdep: Introduce lockdep_assert_held()

In short, should lockdep_assert_held() support ->lockdep_recursion?

Its current definition is
#define lockdep_assert_held(l) WARN_ON(debug_locks && !lockdep_is_held(l))

When someone somewhere calls lockdep_off() and executes some memory
allocation or something, then the functions to shrink dentry cache
happens to run. And cond_resched_lock() in __shrink_dcache_sb() may
produce a false warning.

fs/dcache.c:
__shrink_dcache_sb()
{
	:::
	spin_lock(&dcache_lru_lock);
	while (...) {
		:::
		cond_resched_lock(&dcache_lru_lock);
		:::
	}
	:::
}

The function __shrink_dcache_sb() acquires dcache_lru_lock correctly and
comfirms it by cond_resched_lock() which calls lockdep_assert_held().
When the caller already called lockdep_off(), lock_is_held() always
return 0 which leads to WARN_ON(true). Obviously the warning is false
positive.

Setting FALSE to debug_locks may be one solution, but this variable
doesn't seem to expect to return to TRUE. So it is better for
lockdep_assert_held() to test ->lockdep_recursion too I think.

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 413c754..8658138 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -341,7 +341,9 @@ extern void lockdep_trace_alloc(gfp_t mask);
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
-#define lockdep_assert_held(l)	WARN_ON_ONCE(debug_locks && !lockdep_is_held(l))
+#define lockdep_assert_held(l)	WARN_ON_ONCE(debug_locks \
+					     && !current->lockdep_recursion \
+					     && !lockdep_is_held(l))
 
 #else /* !LOCKDEP */
 

J. R. Okajima

Re: Q. lockdep_assert_held() and lockdep_off/on()

[Peter Zijlstra]

On Fri, 2011-04-08 at 22:19 +0900, J. R. Okajima wrote:
> Hello Peter Zijlstra and Ingo Molnar,
> 
> May I ask you a question about the commit
> 	f607c66 2009-08-02 lockdep: Introduce lockdep_assert_held()
> 
> In short, should lockdep_assert_held() support ->lockdep_recursion?

Probably, however

> Its current definition is
> #define lockdep_assert_held(l) WARN_ON(debug_locks && !lockdep_is_held(l))
> 
> When someone somewhere calls lockdep_off() and executes some memory

you're not supposed to do that..