Subject: lockdep: allow for preemption between lock_acquire and lock_acquired From: Peter Zijlstra In order to allow lockdep to use the regular preempt spinlocks that enable preemption and local irqs we need to teach lockdep about that. The big problem is that we can generate a false dependency due to the to be acquired already being on the hold list. We can't delay that operation because we need to first tell lockdep we're to acquire the lock and then take it in order to be told about actual deadlocks before they happen. The solution is to mark the lock pending and ignore all pending locks in the dependency stuff - we then use the lockstat lock_acquired() hook to clear the pending status. This does mean we need to call lock_acquired() for prove_locking too. It is safe to assume the pending lock is the top lock, because the preempted context must not leave any locks behind. Signed-off-by: Peter Zijlstra --- include/linux/lockdep.h | 13 ++- kernel/lockdep.c | 162 +++++++++++++++++++++++++++++++----------------- 2 files changed, 116 insertions(+), 59 deletions(-) Index: linux-2.6/include/linux/lockdep.h =================================================================== --- linux-2.6.orig/include/linux/lockdep.h +++ linux-2.6/include/linux/lockdep.h @@ -228,6 +228,7 @@ struct held_lock { int read; int check; int hardirqs_off; + int pending; }; /* @@ -301,6 +302,8 @@ extern void lockdep_init_map(struct lock extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, int trylock, int read, int check, unsigned long ip); +extern void lock_acquired(struct lockdep_map *lock); + extern void lock_release(struct lockdep_map *lock, int nested, unsigned long ip); @@ -319,6 +322,7 @@ static inline void lockdep_on(void) } # define lock_acquire(l, s, t, r, c, i) do { } while (0) +# define lock_acquired(lockdep_map) do { } while (0) # define lock_release(l, n, i) do { } while (0) # define lockdep_init() do { } while (0) # define lockdep_info() do { } while (0) @@ -346,7 +350,6 @@ struct lock_class_key { }; #ifdef CONFIG_LOCK_STAT extern void lock_contended(struct lockdep_map *lock, unsigned long ip); -extern void lock_acquired(struct lockdep_map *lock); #define LOCK_CONTENDED(_lock, try, lock) \ do { \ @@ -360,10 +363,12 @@ do { \ #else /* CONFIG_LOCK_STAT */ #define lock_contended(lockdep_map, ip) do {} while (0) -#define lock_acquired(lockdep_map) do {} while (0) -#define LOCK_CONTENDED(_lock, try, lock) \ - lock(_lock) +#define LOCK_CONTENDED(_lock, try, lock) \ +do { \ + lock(_lock); \ + lock_acquired(&(_lock)->dep_map); \ +} while (0) #endif /* CONFIG_LOCK_STAT */ Index: linux-2.6/kernel/lockdep.c =================================================================== --- linux-2.6.orig/kernel/lockdep.c +++ linux-2.6/kernel/lockdep.c @@ -1315,6 +1315,12 @@ check_prev_add(struct task_struct *curr, int ret; /* + * don't build dependancies on locks that aren't fully held yet. + */ + if (prev->pending) + return 1; + + /* * Prove that the new -> dependency would not * create a circular dependency in the graph. (We do this by * forward-recursing into the graph starting at , and @@ -1407,6 +1413,7 @@ check_prevs_add(struct task_struct *curr */ if (!depth) goto out_bug; + /* * At least two relevant locks must exist for this * to be a head: @@ -1976,6 +1983,9 @@ mark_held_locks(struct task_struct *curr for (i = 0; i < curr->lockdep_depth; i++) { hlock = curr->held_locks + i; + if (hlock->pending) + continue; + if (hardirq) { if (hlock->read) usage_bit = LOCK_ENABLED_HARDIRQS_READ; @@ -2411,6 +2421,7 @@ static int __lock_acquire(struct lockdep hlock->read = read; hlock->check = check; hlock->hardirqs_off = hardirqs_off; + hlock->pending = 1; #ifdef CONFIG_LOCK_STAT hlock->waittime_stamp = 0; hlock->holdtime_stamp = sched_clock(); @@ -2438,13 +2449,21 @@ static int __lock_acquire(struct lockdep return 0; chain_key = curr->curr_chain_key; + /* + * If the top lock is pending - pretend its not there. + */ + if (depth && curr->held_locks[depth-1].pending) { + chain_key = curr->held_locks[depth-1].prev_chain_key; + depth--; + } + if (!depth) { if (DEBUG_LOCKS_WARN_ON(chain_key != 0)) return 0; chain_head = 1; } - hlock->prev_chain_key = chain_key; + hlock->prev_chain_key = curr->curr_chain_key; if (separate_irq_context(curr, hlock)) { chain_key = 0; chain_head = 1; @@ -2760,27 +2779,22 @@ static void __lock_contended(struct lockdep_map *lock, unsigned long ip) { struct task_struct *curr = current; - struct held_lock *hlock, *prev_hlock; + struct held_lock *hlock; struct lock_class_stats *stats; unsigned int depth; - int i, point; + int point; depth = curr->lockdep_depth; if (DEBUG_LOCKS_WARN_ON(!depth)) return; - prev_hlock = NULL; - for (i = depth-1; i >= 0; i--) { - hlock = curr->held_locks + i; - /* - * We must not cross into another context: - */ - if (prev_hlock && prev_hlock->irq_context != hlock->irq_context) - break; - if (hlock->instance == lock) - goto found_it; - prev_hlock = hlock; - } + /* + * must always be the topmost lock.. + */ + hlock = curr->held_locks + depth-1; + if (hlock->instance == lock) + goto found_it; + print_lock_contention_bug(curr, lock, ip); return; @@ -2797,37 +2811,33 @@ found_it: put_lock_stats(stats); } -static void -__lock_acquired(struct lockdep_map *lock) +void lock_contended(struct lockdep_map *lock, unsigned long ip) { - struct task_struct *curr = current; - struct held_lock *hlock, *prev_hlock; - struct lock_class_stats *stats; - unsigned int depth; - u64 now; - s64 waittime = 0; - int i, cpu; + unsigned long flags; - depth = curr->lockdep_depth; - if (DEBUG_LOCKS_WARN_ON(!depth)) + if (unlikely(!lock_stat)) return; - prev_hlock = NULL; - for (i = depth-1; i >= 0; i--) { - hlock = curr->held_locks + i; - /* - * We must not cross into another context: - */ - if (prev_hlock && prev_hlock->irq_context != hlock->irq_context) - break; - if (hlock->instance == lock) - goto found_it; - prev_hlock = hlock; - } - print_lock_contention_bug(curr, lock, _RET_IP_); - return; + if (unlikely(current->lockdep_recursion)) + return; + + raw_local_irq_save(flags); + check_flags(flags); + current->lockdep_recursion = 1; + __lock_contended(lock, ip); + current->lockdep_recursion = 0; + raw_local_irq_restore(flags); +} +EXPORT_SYMBOL_GPL(lock_contended); + +static void +__lockstat_acquired(struct lockdep_map *lock, struct held_lock *hlock) +{ + struct lock_class_stats *stats; + s64 waittime = 0; + u64 now; + int cpu; -found_it: cpu = smp_processor_id(); if (hlock->waittime_stamp) { now = sched_clock(); @@ -2849,30 +2859,73 @@ found_it: lock->cpu = cpu; } -void lock_contended(struct lockdep_map *lock, unsigned long ip) +#else + +static void +__lockstat_acquired(struct lockdep_map *lock, struct held_lock *hlock) { - unsigned long flags; +} - if (unlikely(!lock_stat)) - return; +#endif - if (unlikely(current->lockdep_recursion)) +static int +print_lock_acquired_bug(struct task_struct *curr, struct lockdep_map *lock, + unsigned long ip) +{ + if (!debug_locks_off()) + return 0; + if (debug_locks_silent) + return 0; + + printk("\n===============================\n"); + printk( "[ BUG: bad acquired detected! ]\n"); + printk( "-------------------------------\n"); + printk("%s/%d is trying to contend lock (", + curr->comm, task_pid_nr(curr)); + print_lockdep_cache(lock); + printk(") at:\n"); + print_ip_sym(ip); + printk("but there are no locks held!\n"); + printk("\nother info that might help us debug this:\n"); + lockdep_print_held_locks(curr); + + printk("\nstack backtrace:\n"); + dump_stack(); + + return 0; +} + +static void +__lock_acquired(struct lockdep_map *lock) +{ + struct task_struct *curr = current; + struct held_lock *hlock; + unsigned int depth; + + depth = curr->lockdep_depth; + if (DEBUG_LOCKS_WARN_ON(!depth)) return; - raw_local_irq_save(flags); - check_flags(flags); - current->lockdep_recursion = 1; - __lock_contended(lock, ip); - current->lockdep_recursion = 0; - raw_local_irq_restore(flags); + /* + * must always be the topmost lock.. + */ + hlock = curr->held_locks + depth-1; + if (hlock->instance == lock) + goto found_it; + + print_lock_acquired_bug(curr, lock, _RET_IP_); + return; + +found_it: + hlock->pending = 0; + __lockstat_acquired(lock, hlock); } -EXPORT_SYMBOL_GPL(lock_contended); void lock_acquired(struct lockdep_map *lock) { unsigned long flags; - if (unlikely(!lock_stat)) + if (unlikely(!lock_stat && !prove_locking)) return; if (unlikely(current->lockdep_recursion)) @@ -2886,7 +2939,6 @@ void lock_acquired(struct lockdep_map *l raw_local_irq_restore(flags); } EXPORT_SYMBOL_GPL(lock_acquired); -#endif /* * Used by the testsuite, sanitize the validator state