2011-04-08 13:20:18

by J. R. Okajima

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

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

2011-04-08 13:32:38

by Peter Zijlstra

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

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..