Messages in this thread Patch in this message | | | Date | Mon, 19 Nov 2012 14:54:12 +0100 | From | Oleg Nesterov <> | Subject | Q: __lockdep_no_validate__ (Was: [PATCH -mm 0/3] percpu_rw_semaphore: lockdep + config) |
| |
On 11/18, Oleg Nesterov wrote: > > On 11/11, Oleg Nesterov wrote: > > > It turns out, lockdep annotations are not that simple due to internal > locks used by percpu_rw_semaphore. To clarify, it is actually simple > but lockdep_set_novalidate_class() doesn't seem to actually work, and > more importantly, it must not be used according to checkpatch.pl.
Still, is __lockdep_no_validate__ logic correct? I am just curious.
Consider the following code,
DEFINE_MUTEX(m1); DEFINE_MUTEX(m2); DEFINE_MUTEX(mx);
static void trigger_lockdep_bug(bool novalidate) { if (novalidate) lockdep_set_novalidate_class(&mx);
// m1 -> mx -> m2 mutex_lock(&m1); mutex_lock(&mx); mutex_lock(&m2); mutex_unlock(&m2); mutex_unlock(&mx); mutex_unlock(&m1);
// m2 -> m1 ; should trigger the warning
mutex_lock(&m2); mutex_lock(&m1); mutex_unlock(&m1); mutex_unlock(&m2);
}
trigger_lockdep_bug(false) works correctly, but novalidate => true confuses (I think) lockdep and it doesn't detect the trivial deadlock.
check_prev_add(m1, mx) still adds the new dependency, but then it is ignored because of __lockdep_no_validate__ check.
Certainly I do not understand this code (and I am sure I will never understand it even if I try ;) But perhaps something like below makes sense? Or I misunderstood the purpose of lockdep_set_novalidate_class?
Thanks,
Oleg.
--- x/kernel/lockdep.c +++ x/kernel/lockdep.c @@ -1935,7 +1939,8 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next) * Only non-recursive-read entries get new dependencies * added: */ - if (hlock->read != 2) { + if (hlock->read != 2 && + hlock->instance->key != &__lockdep_no_validate__) { if (!check_prev_add(curr, hlock, next, distance, trylock_loop)) return 0;
| |