lockdep: Support deadlock detection for recursive read locks
Subsystems (3)
v4 cover lockdep: Support deadlock detection for recursive read locks
Hi Ingo and Peter,
This is V4 for recursive read lock support in lockdep. I add the explanation about reasoning in patch #16 and would like to check that with you first in case I made fundamental mistakes.
Changes since V3:
* Reduce the unnecessary cost in structure lock_list as suggested
by Peter Zijlstra
* Add documentation for explanation of the reasoning in recursive
read lock deadlock detection.
* Add comments before bfs() describing the greedy search we use in
BFS for strong dependency paths.
* Add myself as a reviewer for LOCKING PRIMITIVES so that if there
is any performance regression, log misunderstanding or false positives, people know who to blame^Wask help from.
V1: https://marc.info/?l=linux-kernel&m=150393341825453
V2: https://marc.info/?l=linux-kernel&m=150468649417950
V3: https://marc.info/?l=linux-kernel&m=150637795424969
As Peter pointed out:
https://marc.info/?l=linux-kernel&m=150349072023540
The lockdep current has a limit support for recursive read locks, the deadlock case as follow could not be detected:
read_lock(A); lock(B); lock(B); write_lock(A);
I got some inspiration from Gautham R Shenoy:
https://lwn.net/Articles/332801/
, and came up with this series.
The basic idea is:
* Add recursive read locks into the graph
* Classify dependencies into -(RR)->, -(NR)->, -(RN)->,
-(NN)->, where R stands for recursive read lock, N stands for other locks(i.e. non-recursive read locks and write locks).
* Define strong dependency paths as the paths of dependencies
don't have two adjacent dependencies as -(*R)-> and -(R*)->.
* Extend __bfs() to only traverse on strong dependency paths.
* If __bfs() finds a strong dependency circle, then a deadlock is
reported.
The whole series consists of 17 patches:
1. Do a clean up on the return value of __bfs() and its friends.
2. Make __bfs() able to visit every dependency until a match is
found. The old version of __bfs() could only visit each lock class once, and this is insufficient if we are going to add recursive read locks into the dependency graph.
3. Redefine LOCK*_STATE*, now LOCK*_STATE_RR stand for recursive
read lock only and LOCK*_STATE stand for write lock and non-recursive read lock.
4-5 Extend __bfs() to be able to traverse the stong dependency
patchs after recursive read locks added into the graph.
6-8 Adjust check_redundant(), check_noncircular() and
check_irq_usage() with recursive read locks into consideration.
9. Finally add recursive read locks into the dependency graph.
10-11 Adjust lock cache chain key generation with recursive read locks
into consideration, and provide a test case.
12-13 Add more test cases.
14. Revert commit d82fed752942 ("locking/lockdep/selftests: Fix
mixed read-write ABBA tests"),
15. Reduce the extra size cost of lock_list to zero
16. Add documentation for recursive read lock deadlock detection
reasoning
17. Add myself as a LOCKING PRIMITIVES reviewer.
This series passed all the lockdep selftest cases(including those I introduce).
Test and comments are welcome!
Regards,
Boqun
RFC v4 1/17 lockdep: Demagic the return value of BFS
__bfs() could return four magic numbers:
1: search succeeds, but none match.
0: search succeeds, find one match.
-1: search fails because of the cq is full. -2: search fails because a invalid node is found.
This patch cleans things up by using a enum type for the return value of __bfs() and its friends, this improves the code readability of the code, and further, could help if we want to extend the BFS.
--- kernel/locking/lockdep.c | 136 ++++++++++++++++++++++++++++------------------- 1 file changed, 80 insertions(+), 56 deletions(-)
kernel/locking/lockdep.c +80 -56
@@ -979,21 +979,52 @@ static inline int get_lock_depth(struct lock_list *child) }return depth; } +/* +* Return values of a bfs search: +* +* BFS_E* indicates an error +* BFS_R* indicates a result(match or not) +* +* BFS_EINVALIDNODE: Find a invalid node in the graph. +* +* BFS_EQUEUEFULL: The queue is full while doing the bfs. +* +* BFS_RMATCH: Find the matched node in the graph, and put that node * into +* *@target_entry. +* +* BFS_RNOMATCH: Haven't found the matched node and keep *@target_entry +* _unchanged_. +*/ +enum bfs_result { +BFS_EINVALIDNODE =-2 , +BFS_EQUEUEFULL =-1 , +BFS_RMATCH =0 , +BFS_RNOMATCH =1 , +}; -static int __bfs (struct lock_list *source_entry, -void *data, -int (*match)(struct lock_list *entry,void *data), -struct lock_list **target_entry, -int forward) +/* +* bfs_result < 0 means error +*/ + +static inline bool bfs_error (enum bfs_result res) +{ +return res <0 ; +} + +static enum bfs_result __bfs (struct lock_list *source_entry, +void *data, +int (*match)(struct lock_list *entry,void *data), +struct lock_list **target_entry, +int forward) {struct lock_list *entry;struct list_head *head;struct circular_queue *cq = &lock_cq; -int ret =1 ; +enum bfs_result ret =BFS_RNOMATCH ;if (match (source_entry, data)) { *target_entry = source_entry; - ret =0 ; + ret =BFS_RMATCH ; goto exit; }
@@ -1014,7 +1045,7 @@ static int __bfs(struct lock_list *source_entry,__cq_dequeue (cq, (unsigned long *)&lock);if (!lock->class) { - ret =-2 ; + ret =BFS_EINVALIDNODE ; goto exit; }
@@ -1031,12 +1062,12 @@ static int __bfs(struct lock_list *source_entry,mark_lock_accessed (entry, lock);if (match (entry, data)) { *target_entry = entry; - ret =0 ; + ret =BFS_RMATCH ; goto exit; }if (__cq_enqueue (cq, (unsigned long )entry)) { - ret =-1 ; + ret =BFS_EQUEUEFULL ; goto exit; } cq_depth =__cq_get_elem_count (cq);
@@ -1049,19 +1080,21 @@ static int __bfs(struct lock_list *source_entry,return ret; } -static inline int __bfs_forwards (struct lock_list *src_entry, -void *data, -int (*match)(struct lock_list *entry,void *data), -struct lock_list **target_entry) +static inline enum bfs_result +__bfs_forwards (struct lock_list *src_entry, +void *data, +int (*match)(struct lock_list *entry,void *data), +struct lock_list **target_entry) {return __bfs (src_entry, data, match, target_entry,1 ); } -static inline int __bfs_backwards (struct lock_list *src_entry, -void *data, -int (*match)(struct lock_list *entry,void *data), -struct lock_list **target_entry) +static inline enum bfs_result +__bfs_backwards (struct lock_list *src_entry, +void *data, +int (*match)(struct lock_list *entry,void *data), +struct lock_list **target_entry) {return __bfs (src_entry, data, match, target_entry,0 );
@@ -1294,13 +1327,13 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class)/* * Prove that the dependency graph starting at <entry> can not -* lead to <target>. Print an error and return 0 if it does. +* lead to <target>. Print an error and return BFS_RMATCH if it does. */ -static noinline int +static noinline enum bfs_resultcheck_noncircular (struct lock_list *root,struct lock_class *target, -struct lock_list **target_entry) +struct lock_list **target_entry) { -int result; +enum bfs_result result;debug_atomic_inc (nr_cyclic_checks);
@@ -1309,11 +1342,11 @@ check_noncircular(struct lock_list *root, struct lock_class *target,return result; } -static noinline int +static noinline enum bfs_resultcheck_redundant (struct lock_list *root,struct lock_class *target,struct lock_list **target_entry) { -int result; +enum bfs_result result;debug_atomic_inc (nr_redundant_checks);
@@ -1342,15 +1375,12 @@ static inline int usage_match(struct lock_list *entry, void *bit)* * Return 0 if such a node exists in the subgraph, and put that node * into *@target_entry. -* -* Return 1 otherwise and keep *@target_entry unchanged. -* Return <0 on error. */ -static int +static enum bfs_result find_usage_forwards (struct lock_list *root,enum lock_usage_bit bit,struct lock_list **target_entry) { -int result; +enum bfs_result result;debug_atomic_inc (nr_find_usage_forwards_checks);
@@ -1362,18 +1392,12 @@ find_usage_forwards(struct lock_list *root, enum lock_usage_bit bit,/* * Find a node in the backwards-direction dependency sub-graph starting * at @root->class that matches @bit. -* -* Return 0 if such a node exists in the subgraph, and put that node -* into *@target_entry. -* -* Return 1 otherwise and keep *@target_entry unchanged. -* Return <0 on error. */ -static int +static enum bfs_result find_usage_backwards (struct lock_list *root,enum lock_usage_bit bit,struct lock_list **target_entry) { -int result; +enum bfs_result result;debug_atomic_inc (nr_find_usage_backwards_checks);
@@ -1581,18 +1605,18 @@ check_usage(struct task_struct *curr, struct held_lock *prev, this.class =hlock_class (prev); ret =find_usage_backwards (&this, bit_backwards, &target_entry); -if (ret <0 ) +if (bfs_error (ret))return print_bfs_bug (ret); -if (ret ==1 ) -return ret; +if (ret ==BFS_RNOMATCH ) +return 1 ; that.parent =NULL ; that.class =hlock_class (next); ret =find_usage_forwards (&that, bit_forwards, &target_entry1); -if (ret <0 ) +if (bfs_error (ret))return print_bfs_bug (ret); -if (ret ==1 ) -return ret; +if (ret ==BFS_RNOMATCH ) +return 1 ; return print_bad_irq_dependency(curr, &this, &that, target_entry, target_entry1,
@@ -1829,10 +1853,10 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev, struct held_lock *next, int distance, struct stack_trace *trace, int (*save)(struct stack_trace *trace)) { - struct lock_list *uninitialized_var(target_entry); struct lock_list *entry; +enum bfs_result ret;struct lock_list this; -int ret; +struct lock_list *uninitialized_var (target_entry );/* * Prove that the new <prev> -> <next> dependency would not
@@ -1846,7 +1870,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev, this.class =hlock_class (next); this.parent =NULL ; ret =check_noncircular (&this,hlock_class (prev), &target_entry); -if (unlikely (!ret)) { +if (unlikely (ret ==BFS_RMATCH )) {if (!trace->entries) {/* * If @save fails here, the printing might trigger
@@ -1857,7 +1881,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev, }return print_circular_bug (&this, target_entry, next, prev, trace); } -else if (unlikely (ret <0 )) +else if (unlikely (bfs_error (ret)))return print_bfs_bug (ret);if (!check_prev_add_irq (curr, prev, next))
@@ -1895,11 +1919,11 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev, this.class =hlock_class (prev); this.parent =NULL ; ret =check_redundant (&this,hlock_class (next), &target_entry); -if (!ret) { +if (ret ==BFS_RMATCH ) {debug_atomic_inc (nr_redundant);return 2 ; } -if (ret <0 ) +if (bfs_error (ret))return print_bfs_bug (ret);
@@ -2628,16 +2652,16 @@ static int check_usage_forwards(struct task_struct *curr, struct held_lock *this, enum lock_usage_bit bit, const char *irqclass) { - int ret; + enum bfs_result ret;struct lock_list root;struct lock_list *uninitialized_var (target_entry ); root.parent =NULL ; root.class =hlock_class (this); ret =find_usage_forwards (&root, bit, &target_entry); -if (ret <0 ) +if (bfs_error (ret))return print_bfs_bug (ret); -if (ret ==1 ) +if (ret ==BFS_RNOMATCH )return ret; return print_irq_inversion_bug(curr, &root, target_entry,
@@ -2652,17 +2676,17 @@ static int check_usage_backwards(struct task_struct *curr, struct held_lock *this, enum lock_usage_bit bit, const char *irqclass) { - int ret; + enum bfs_result ret;struct lock_list root;struct lock_list *uninitialized_var (target_entry ); root.parent =NULL ; root.class =hlock_class (this); ret =find_usage_backwards (&root, bit, &target_entry); -if (ret <0 ) +if (bfs_error (ret))return print_bfs_bug (ret); -if (ret ==1 ) -return ret; +if (ret ==BFS_RNOMATCH ) +return 1 ;return print_irq_inversion_bug (curr, &root, target_entry, this,0 , irqclass);
Signature · 1 line
2.15.1
RFC v4 2/17 lockdep: Make __bfs() visit every dependency until a match
In reply to Boqun FengCurrently, __bfs() will do a breadth-first search in the dependency graph and visit each lock class in the graph exactly once, so for example, in the following graph:
A ---------> B | ^ | | +----------> C
a __bfs() call starts at A, will visit B through dependency A -> B and visit C through dependency A -> C and that's it, IOW, __bfs() will not visit dependency C -> B.
This is OK for now, as we only have strong dependencies in the dependency graph, so whenever there is a traverse path from A to B in __bfs(), it means A has strong dependency to B(IOW, B depends on A strongly). So no need to visit all dependencies in the graph.
However, as we are going to add recursive-read lock into the dependency graph, afterwards, not all the paths mean strong dependencies, in the same example above, dependency A -> B may be a weak dependency and traverse A -> C -> B may be a strong dependency path. And with the old way of __bfs()(i.e. visiting every lock class exactly once), we will miss the strong dependency path, which will result into failing to find a deadlock. To cure this for the future, we need to find a way for __bfs() to visit each dependency, rather than each class, exactly once in the search until we find a match.
The solution is simple:
We used to mark lock_class::lockdep_dependency_gen_id to indicate a class has been visited in __bfs(), now we change the semantics a little
bit: we now mark lock_class::lockdep_dependency_gen_id to indicate _all the dependencies_ in its lock_{after,before} have been visited in the __bfs()(note we only take one direction in a __bfs() search). In this way, every dependency is guaranteed to be visited until we find a match.
--- kernel/locking/lockdep.c | 61 +++++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 27 deletions(-)
kernel/locking/lockdep.c +34 -27
@@ -943,24 +943,20 @@ static inline unsigned int __cq_get_elem_count(struct circular_queue *cq)return (cq->rear - cq->front) &CQ_MASK ; } -static inline void mark_lock_accessed (struct lock_list *lock, -struct lock_list *parent) +static inline void mark_lock_list_accessed (struct lock_class *class) { -unsigned long nr; + class->dep_gen_id = lockdep_dependency_gen_id; +} - nr = lock - list_entries; -WARN_ON (nr >= nr_list_entries);/* Out-of-bounds, input fail */ +static inline void visit_lock_entry (struct lock_list *lock, +struct lock_list *parent) +{ lock->parent = parent; - lock->class->dep_gen_id = lockdep_dependency_gen_id; } -static inline unsigned long lock_accessed (struct lock_list *lock) +static inline unsigned long lock_list_accessed (struct lock_class *class) { -unsigned long nr; - - nr = lock - list_entries; -WARN_ON (nr >= nr_list_entries);/* Out-of-bounds, input fail */ -return lock->class->dep_gen_id == lockdep_dependency_gen_id; +return class->dep_gen_id == lockdep_dependency_gen_id; }static inline struct lock_list *get_lock_parent (struct lock_list *child)
@@ -1049,6 +1045,18 @@ static enum bfs_result __bfs(struct lock_list *source_entry, goto exit; } +/* +* If we have visited all the dependencies from this @lock to +* others(iow, if we have visited all lock_list entries in +* @lock->class->locks_{after,before}, we skip, otherwise go +* and visit all the dependencies in the list and mark this +* list accessed. +*/ +if (lock_list_accessed (lock->class)) +continue ; +else +mark_lock_list_accessed (lock->class); +if (forward) head = &lock->class->locks_after;else
@@ -1057,23 +1065,22 @@ static enum bfs_result __bfs(struct lock_list *source_entry,DEBUG_LOCKS_WARN_ON (!irqs_disabled ());list_for_each_entry_rcu (entry, head, entry) { -if (!lock_accessed (entry)) { -unsigned int cq_depth; -mark_lock_accessed (entry, lock); -if (match (entry, data)) { - *target_entry = entry; - ret =BFS_RMATCH ; - goto exit; - } +unsigned int cq_depth; -if (__cq_enqueue (cq, (unsigned long )entry)) { - ret =BFS_EQUEUEFULL ; - goto exit; - } - cq_depth =__cq_get_elem_count (cq); -if (max_bfs_queue_depth < cq_depth) - max_bfs_queue_depth = cq_depth; +visit_lock_entry (entry, lock); +if (match (entry, data)) { + *target_entry = entry; + ret =BFS_RMATCH ; + goto exit; + } + +if (__cq_enqueue (cq, (unsigned long )entry)) { + ret =BFS_EQUEUEFULL ; + goto exit; } + cq_depth =__cq_get_elem_count (cq); +if (max_bfs_queue_depth < cq_depth) + max_bfs_queue_depth = cq_depth; } } exit:
Signature · 1 line
2.15.1
RFC v4 3/17 lockdep: Redefine LOCK_*_STATE* bits
In reply to Boqun FengThere are three types of lock acquisitions: write, non-recursive read and recursive read, among which write locks and non-recursive read locks have no difference from a viewpoint for deadlock detections, because a write acquisition of the corresponding lock on an independent CPU or task makes a non-recursive read lock act as a write lock in the sense of deadlock. So we could treat them as the same type(named as "non-recursive lock") in lockdep.
As in the irq lock inversion detection(safe->unsafe deadlock detection), we used to differ write lock with read lock(non-recursive and recursive ones), such a classification could be improved as non-recursive read lock behaves the same as write lock, so this patch redefines the meanings of LOCK_{USED_IN, ENABLED}_STATE*.
old:
LOCK_* : stands for write lock
LOCK_*_READ: stands for read lock(non-recursive and recursive)
new:
LOCK_* : stands for non-recursive(write lock and non-recursive read lock)
LOCK_*_RR: stands for recursive read lock
Such a change is needed for a future improvement on recursive read related irq inversion deadlock detection.
--- Documentation/locking/lockdep-design.txt | 6 +++--- kernel/locking/lockdep.c | 28 ++++++++++++++-------------- kernel/locking/lockdep_internals.h | 16 ++++++++-------- kernel/locking/lockdep_proc.c | 12 ++++++------ 4 files changed, 31 insertions(+), 31 deletions(-)
Documentation/locking/lockdep-design.txt +3 -3
@@ -30,9 +30,9 @@ State The validator tracks lock-class usage history into 4n + 1 separate state bits: - 'ever held in STATE context' -- 'ever held as readlock in STATE context' +- 'ever held as recursive readlock in STATE context' - 'ever held with STATE enabled' -- 'ever held as readlock with STATE enabled' +- 'ever held as recurisve readlock with STATE enabled' Where STATE can be either one of (kernel/locking/lockdep_states.h) - hardirq
@@ -51,7 +51,7 @@ locking error messages, inside curlies. A contrived example: (&sio_locks[i].lock){-.-...}, at: [<c02867fd>] mutex_lock+0x21/0x24 -The bit position indicates STATE, STATE-read, for each of the states listed +The bit position indicates STATE, STATE-RR, for each of the states listed above, and the character displayed in each indicates: '.' acquired while irqs disabled and not in irq context
kernel/locking/lockdep.c +14 -14
@@ -447,10 +447,10 @@ DEFINE_PER_CPU(struct lockdep_stats, lockdep_stats);*/ #define __USAGE (__STATE) \ - [LOCK_USED_IN_##__STATE] = "IN-"__stringify(__STATE)"-W", \ - [LOCK_ENABLED_##__STATE] = __stringify(__STATE)"-ON-W", \ - [LOCK_USED_IN_##__STATE##_READ] = "IN-"__stringify(__STATE)"-R",\ - [LOCK_ENABLED_##__STATE##_READ] = __stringify(__STATE)"-ON-R", + [LOCK_USED_IN_##__STATE] = "IN-"__stringify(__STATE), \ + [LOCK_ENABLED_##__STATE] = __stringify(__STATE)"-ON", \ + [LOCK_USED_IN_##__STATE##_RR] = "IN-"__stringify(__STATE)"-RR", \ + [LOCK_ENABLED_##__STATE##_RR] = __stringify(__STATE)"-ON-RR",static const char *usage_str[] = {
@@ -491,7 +491,7 @@ void get_usage_chars(struct lock_class *class, char usage[LOCK_USAGE_CHARS])#define LOCKDEP_STATE (__STATE) \ usage[i++] = get_usage_char(class, LOCK_USED_IN_##__STATE); \ - usage[i++] = get_usage_char(class, LOCK_USED_IN_##__STATE##_READ); + usage[i++] = get_usage_char(class, LOCK_USED_IN_##__STATE##_RR);#include "lockdep_states.h" #undef LOCKDEP_STATE
@@ -1640,7 +1640,7 @@ static const char *state_names[] = {static const char *state_rnames[] = { #define LOCKDEP_STATE(__STATE) \ - __stringify(__STATE)"-READ", + __stringify(__STATE)"-RR", #include "lockdep_states.h" #undef LOCKDEP_STATE };
@@ -3034,14 +3034,14 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock)* mark the lock as used in these contexts: */ if (!hlock->trylock) { -if (hlock->read) { +if (hlock->read ==2 ) {if (curr->hardirq_context)if (!mark_lock (curr, hlock, -LOCK_USED_IN_HARDIRQ_READ )) +LOCK_USED_IN_HARDIRQ_RR ))return 0 ;if (curr->softirq_context)if (!mark_lock (curr, hlock, -LOCK_USED_IN_SOFTIRQ_READ )) +LOCK_USED_IN_SOFTIRQ_RR ))return 0 ; }else {if (curr->hardirq_context)
@@ -3053,13 +3053,13 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock) } } if (!hlock->hardirqs_off) { - if (hlock->read) { + if (hlock->read == 2) { if (!mark_lock(curr, hlock, - LOCK_ENABLED_HARDIRQ_READ)) + LOCK_ENABLED_HARDIRQ_RR)) return 0; if (curr->softirqs_enabled) if (!mark_lock(curr, hlock, - LOCK_ENABLED_SOFTIRQ_READ)) + LOCK_ENABLED_SOFTIRQ_RR)) return 0; } else { if (!mark_lock(curr, hlock,
@@ -3165,9 +3165,9 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this,switch (new_bit) {#define LOCKDEP_STATE (__STATE) \ case LOCK_USED_IN_##__STATE: \ - case LOCK_USED_IN_##__STATE##_READ: \ + case LOCK_USED_IN_##__STATE##_RR: \ case LOCK_ENABLED_##__STATE: \ - case LOCK_ENABLED_##__STATE##_READ: + case LOCK_ENABLED_##__STATE##_RR:#include "lockdep_states.h" #undef LOCKDEP_STATE ret =mark_lock_irq (curr, this, new_bit);
kernel/locking/lockdep_internals.h +8 -8
@@ -13,9 +13,9 @@ enum lock_usage_bit { #define LOCKDEP_STATE(__STATE) \ LOCK_USED_IN_##__STATE, \ - LOCK_USED_IN_##__STATE##_READ, \ + LOCK_USED_IN_##__STATE##_RR, \ LOCK_ENABLED_##__STATE, \ - LOCK_ENABLED_##__STATE##_READ, + LOCK_ENABLED_##__STATE##_RR,#include "lockdep_states.h" #undef LOCKDEP_STATE LOCK_USED,
@@ -30,9 +30,9 @@ enum lock_usage_bit { enum { #define LOCKDEP_STATE(__STATE) \ __LOCKF(USED_IN_##__STATE) \ - __LOCKF(USED_IN_##__STATE##_READ) \ + __LOCKF(USED_IN_##__STATE##_RR) \ __LOCKF(ENABLED_##__STATE) \ - __LOCKF(ENABLED_##__STATE##_READ) + __LOCKF(ENABLED_##__STATE##_RR)#include "lockdep_states.h" #undef LOCKDEP_STATE__LOCKF (USED )
@@ -41,10 +41,10 @@ enum {#define LOCKF_ENABLED_IRQ (LOCKF_ENABLED_HARDIRQ | LOCKF_ENABLED_SOFTIRQ)#define LOCKF_USED_IN_IRQ (LOCKF_USED_IN_HARDIRQ | LOCKF_USED_IN_SOFTIRQ) -#define LOCKF_ENABLED_IRQ_READ \ - (LOCKF_ENABLED_HARDIRQ_READ | LOCKF_ENABLED_SOFTIRQ_READ) -#define LOCKF_USED_IN_IRQ_READ \ - (LOCKF_USED_IN_HARDIRQ_READ | LOCKF_USED_IN_SOFTIRQ_READ) +#define LOCKF_ENABLED_IRQ_RR \ + (LOCKF_ENABLED_HARDIRQ_RR | LOCKF_ENABLED_SOFTIRQ_RR) +#define LOCKF_USED_IN_IRQ_RR \ + (LOCKF_USED_IN_HARDIRQ_RR | LOCKF_USED_IN_SOFTIRQ_RR)/* * CONFIG_LOCKDEP_SMALL is defined for sparc. Sparc requires .text,
kernel/locking/lockdep_proc.c +6 -6
@@ -252,17 +252,17 @@ static int lockdep_stats_show(struct seq_file *m, void *v) nr_hardirq_safe++;if (class->usage_mask &LOCKF_ENABLED_HARDIRQ ) nr_hardirq_unsafe++; -if (class->usage_mask &LOCKF_USED_IN_IRQ_READ ) +if (class->usage_mask &LOCKF_USED_IN_IRQ_RR ) nr_irq_read_safe++; -if (class->usage_mask &LOCKF_ENABLED_IRQ_READ ) +if (class->usage_mask &LOCKF_ENABLED_IRQ_RR ) nr_irq_read_unsafe++; -if (class->usage_mask &LOCKF_USED_IN_SOFTIRQ_READ ) +if (class->usage_mask &LOCKF_USED_IN_SOFTIRQ_RR ) nr_softirq_read_safe++; -if (class->usage_mask &LOCKF_ENABLED_SOFTIRQ_READ ) +if (class->usage_mask &LOCKF_ENABLED_SOFTIRQ_RR ) nr_softirq_read_unsafe++; -if (class->usage_mask &LOCKF_USED_IN_HARDIRQ_READ ) +if (class->usage_mask &LOCKF_USED_IN_HARDIRQ_RR ) nr_hardirq_read_safe++; -if (class->usage_mask &LOCKF_ENABLED_HARDIRQ_READ ) +if (class->usage_mask &LOCKF_ENABLED_HARDIRQ_RR ) nr_hardirq_read_unsafe++;#ifdef CONFIG_PROVE_LOCKING
Signature · 1 line
2.15.1
RFC v4 4/17 lockdep: Introduce lock_list::dep
In reply to Boqun FengTo add recursive read locks into the dependency graph, we need to store the types of dependencies for the BFS later. There are four kinds of dependencies:
* Non-recursive -> Non-recursive dependencies(NN)
e.g. write_lock(prev) -> write_lock(next), we can also write this as "prev --(NN)--> next".
* Recursive -> Non-recursive dependencies(RN)
e.g. read_lock(prev) -> write_lock(next), we can also write this as "prev --(RN)--> next".
* Non-recursive -> recursive dependencies(NR)
e.g. write_lock(prev) -> read_lock(next), we can also write this as "prev --(NR)--> next".
* Recursive -> recursive dependencies(RR)
e.g. read_lock(prev) -> read_lock(next), we can also write this as "prev --(RR)--> next".
Given a pair of two locks, four kinds of dependencies could all exist between them, so we use 4 bit for the presence of each kind(stored in lock_list::dep). Helper functions and marcos are also introduced to convert a pair of locks into ::dep bit and maintain the addition of different kinds of dependencies.
--- include/linux/lockdep.h | 2 ++ kernel/locking/lockdep.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 3 deletions(-)
include/linux/lockdep.h +2 -0
@@ -187,6 +187,8 @@ struct lock_list {struct lock_class *class;struct stack_trace trace;int distance; +/* bitmap of different dependencies from head to this */ +u16 dep;/* * The parent field is used to implement breadth-first search, and the
kernel/locking/lockdep.c +45 -3
84 lines not on this page · raw message
Signature · 1 line
2.15.1
RFC v4 5/17 lockdep: Extend __bfs() to work with multiple kinds of dependencies
In reply to Boqun FengNow we have four kinds of dependencies in the dependency graph, and not all the pathes carry strong dependencies, for example:
Given lock A, B, C, if we have: CPU1 CPU2 ============= ============== write_lock(A); read_lock(B); read_lock(B); write_lock(C); then we have dependencies A--(NR)-->B, and B--(RN)-->C, (NR and RN are to indicate the dependency kind), A actually doesn't have strong dependency to C(IOW, C doesn't depend on A), to see this, let's say we have a third CPU3 doing: CPU3: ============= write_lock(C); write_lock(A); , this is not a deadlock. However if we change the read_lock() on CPU2 to a write_lock(), it's a deadlock then. So A --(NR)--> B --(RN)--> C is not a strong dependency path but A --(NR)--> B --(NN)-->C is a strong dependency path.
We can generalize this as: If a path of dependencies doesn't have two adjacent dependencies as (*R)--L-->(R*), where L is some lock, it is a strong dependency path, otherwise it's not.
Now our mission is to make __bfs() traverse only the strong dependency paths, which is simple: we record whether we have -(*R)-> at the current tail of the path in lock_list::is_rr, and whenever we pick a dependency in the traverse, we 1) make sure we don't pick a -(R*)-> dependency if our current tail is -(*R)-> and 2) greedily pick a -(*N)-> as hard as possible.
With this extension for __bfs(), we now need to initialize the root of __bfs() properly(with a correct ->is_rr), to do so, we introduce some helper functions, which also cleans up a little bit for the __bfs() root initialization code.
--- include/linux/lockdep.h | 2 + kernel/locking/lockdep.c | 116 ++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 101 insertions(+), 17 deletions(-)
include/linux/lockdep.h +2 -0
@@ -189,6 +189,8 @@ struct lock_list {int distance;/* bitmap of different dependencies from head to this */ u16 dep; +/* used by BFS to record whether this is picked as a recursive read */ +u16 is_rr;/* * The parent field is used to implement breadth-first search, and the
kernel/locking/lockdep.c +99 -17
187 lines not on this page · raw message
Signature · 1 line
2.15.1
RFC v4 6/17 lockdep: Support deadlock detection for recursive read in check_noncircular()
In reply to Boqun FengCurrently, lockdep only has limit support for deadlock detection for recursive read locks.
The basic idea of the detection is:
Since we make __bfs() able to traverse only the strong dependency paths, so we report a circular deadlock if we could find a circle of a strong dependency path.
--- kernel/locking/lockdep.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
kernel/locking/lockdep.c +12 -4
@@ -1338,6 +1338,14 @@ static inline int class_equal(struct lock_list *entry, void *data)return entry->class == data; } +static inline int hlock_conflict (struct lock_list *entry,void *data) +{ +struct held_lock *hlock = (struct held_lock *)data; + +return hlock_class (hlock) == entry->class && + (hlock->read !=2 || !entry->is_rr); +} + static noinline int print_circular_bug(struct lock_list *this, struct lock_list *target, struct held_lock *check_src,
@@ -1450,18 +1458,18 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class) }/* -* Prove that the dependency graph starting at <entry> can not +* Prove that the dependency graph starting at <root> can not * lead to <target>. Print an error and return BFS_RMATCH if it does. */ static noinline enum bfs_result -check_noncircular (struct lock_list *root,struct lock_class *target, +check_noncircular (struct lock_list *root,struct held_lock *target,struct lock_list **target_entry) {enum bfs_result result;debug_atomic_inc (nr_cyclic_checks); - result =__bfs_forwards (root, target, class_equal, target_entry); + result =__bfs_forwards (root, target, hlock_conflict, target_entry);return result; }
@@ -1989,7 +1997,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,* keep the stackframe size of the recursive functions low: */ bfs_init_root (&this, next); - ret =check_noncircular (&this,hlock_class (prev), &target_entry); + ret =check_noncircular (&this, prev, &target_entry);if (unlikely (ret ==BFS_RMATCH )) {if (!trace->entries) {/*
Signature · 1 line
2.15.1
RFC v4 7/17 lockdep: Adjust check_redundant() for recursive read change
In reply to Boqun FengAs we have four kinds of dependencies now, check_redundant() should only report redundant if we have a dependency path which is equal or _stronger_ than the current dependency. For example if in check_prev_add() we have:
prev->read == 2 && next->read != 2
, we should only report redundant if we find a path like:
prev--(RN)-->....--(*N)-->next
and if we have:
prev->read == 2 && next->read == 2
, we could report redundant if we find a path like:
prev--(RN)-->....--(*N)-->next
or
prev--(RN)-->....--(*R)-->next
To do so, we need to pass the recursive-read status of @next into check_redundant(). This patch changes the parameter of check_redundant() and the match function to achieve this.
--- kernel/locking/lockdep.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
kernel/locking/lockdep.c +8 -5
@@ -1333,9 +1333,12 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,return 0 ; } -static inline int class_equal (struct lock_list *entry,void *data) +static inline int hlock_equal (struct lock_list *entry,void *data) { -return entry->class == data; +struct held_lock *hlock = (struct held_lock *)data; + +return hlock_class (hlock) == entry->class && + (hlock->read ==2 || !entry->is_rr); }static inline int hlock_conflict (struct lock_list *entry,void *data)
@@ -1475,14 +1478,14 @@ check_noncircular(struct lock_list *root, struct held_lock *target, } static noinline enum bfs_result -check_redundant (struct lock_list *root,struct lock_class *target, +check_redundant (struct lock_list *root,struct held_lock *target,struct lock_list **target_entry) {enum bfs_result result;debug_atomic_inc (nr_redundant_checks); - result =__bfs_forwards (root, target, class_equal, target_entry); + result =__bfs_forwards (root, target, hlock_equal, target_entry);return result; }
@@ -2055,7 +2058,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,* Is the <prev> -> <next> link redundant? */ bfs_init_root (&this, prev); - ret =check_redundant (&this,hlock_class (next), &target_entry); + ret =check_redundant (&this, next, &target_entry);if (ret ==BFS_RMATCH ) {debug_atomic_inc (nr_redundant);return 2 ;
Signature · 1 line
2.15.1
RFC v4 8/17 lockdep: Fix recursive read lock related safe->unsafe detection
In reply to Boqun FengThere are four cases for recursive read lock realted deadlocks:
(--(X..Y)--> means a strong dependency path starts with a --(X*)--> dependency and ends with a --(*Y)-- dependency.)
1. An irq-safe lock L1 has a dependency --(*..*)--> to an
irq-unsafe lock L2.
2. An irq-read-safe lock L1 has a dependency --(N..*)--> to an
irq-unsafe lock L2.
3. An irq-safe lock L1 has a dependency --(*..N)--> to an
irq-read-unsafe lock L2.
4. An irq-read-safe lock L1 has a dependency --(N..N)--> to an
irq-read-unsafe lock L2.
The current check_usage() only checks 1) and 2), so this patch adds checks for 3) and 4) and makes sure when find_usage_{back,for}wards find an irq-read-{,un}safe lock, the traverse path should ends at a dependency --(*N)-->. Note when we search backwards, --(*N)--> indicates a real dependency --(N*)-->.
--- kernel/locking/lockdep.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
kernel/locking/lockdep.c +16 -1
@@ -1499,7 +1499,14 @@ check_redundant(struct lock_list *root, struct held_lock *target,static inline int usage_match (struct lock_list *entry,void *bit) { -return entry->class->usage_mask & (1 << (enum lock_usage_bit )bit); +enum lock_usage_bit ub = (enum lock_usage_bit )bit; + + +if (ub &1 ) +return entry->class->usage_mask & (1 << ub) && + !entry->is_rr; +else +return entry->class->usage_mask & (1 << ub); }
@@ -1810,6 +1817,10 @@ static int check_irq_usage(struct task_struct *curr, struct held_lock *prev, exclusive_bit(bit), state_name(bit)))return 0 ; +if (!check_usage (curr, prev, next, bit, +exclusive_bit (bit) +1 ,state_name (bit))) +return 0 ; + bit++;/* _READ */ /*
@@ -1822,6 +1833,10 @@ static int check_irq_usage(struct task_struct *curr, struct held_lock *prev, exclusive_bit(bit), state_name(bit)))return 0 ; +if (!check_usage (curr, prev, next, bit, +exclusive_bit (bit) +1 ,state_name (bit))) +return 0 ; +return 1 ; }
Signature · 1 line
2.15.1
RFC v4 9/17 lockdep: Add recursive read locks into dependency graph
In reply to Boqun FengSince we have all the fundamental to handle recursive read locks, we now add them into the dependency graph.
--- kernel/locking/lockdep.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
kernel/locking/lockdep.c +1 -15
@@ -2033,16 +2033,6 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,if (!check_prev_add_irq (curr, prev, next))return 0 ; -/* -* For recursive read-locks we do all the dependency checks, -* but we dont store read-triggered dependencies (only -* write-triggered dependencies). This ensures that only the -* write-side dependencies matter, and that if for example a -* write-lock never takes any other locks, then the reads are -* equivalent to a NOP. -*/ -if (next->read ==2 || prev->read ==2 ) -return 1 ;/* * Is the <prev> -> <next> dependency already present? *
@@ -2146,11 +2136,7 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)int distance = curr->lockdep_depth - depth +1 ; hlock = curr->held_locks + depth -1 ; -/* -* Only non-recursive-read entries get new dependencies -* added: -*/ -if (hlock->read !=2 && hlock->check) { +if (hlock->check) {int ret =check_prev_add (curr, hlock, next, distance, &trace, save_trace);if (!ret)return 0 ;
Signature · 1 line
2.15.1
RFC v4 10/17 lockdep/selftest: Add a R-L/L-W test case specific to chain cache behavior
In reply to Boqun FengAs our chain cache doesn't differ read/write locks, so even we can detect a read-lock/lock-write deadlock in check_noncircular(), we can still be fooled if a read-lock/lock-read case(which is not a deadlock) comes first.
So introduce this test case to test specific to the chain cache behavior on detecting recursive read lock related deadlocks.
--- lib/locking-selftest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
lib/locking-selftest.c +47 -0
@@ -395,6 +395,49 @@ static void rwsem_ABBA1(void)MU (Y1 );// should fail } +/* +* read_lock(A) +* spin_lock(B) +* spin_lock(B) +* write_lock(A) +* +* This test case is aimed at poking whether the chain cache prevents us from +* detecting a read-lock/lock-write deadlock: if the chain cache doesn't differ +* read/write locks, the following case may happen +* +* { read_lock(A)->lock(B) dependency exists } +* +* P0: +* lock(B); +* read_lock(A); +* +* { Not a deadlock, B -> A is added in the chain cache } +* +* P1: +* lock(B); +* write_lock(A); +* +* { B->A found in chain cache, not reported as a deadlock } +* +*/ +static void rlock_chaincache_ABBA1 (void ) +{ +RL (X1 ); +L (Y1 ); +U (Y1 ); +RU (X1 ); + +L (Y1 ); +RL (X1 ); +RU (X1 ); +U (Y1 ); + +L (Y1 ); +WL (X1 ); +WU (X1 ); +U (Y1 );// should fail +} +/* * read_lock(A) * spin_lock(B)
@@ -2055,6 +2098,10 @@ void locking_selftest(void)pr_cont (" |" );dotest (rwsem_ABBA3,FAILURE ,LOCKTYPE_RWSEM ); +print_testname ("chain cached mixed R-L/L-W ABBA" ); +pr_cont (" |" ); +dotest (rlock_chaincache_ABBA1,FAILURE ,LOCKTYPE_RWLOCK ); +printk (" --------------------------------------------------------------------------\n" );/*
Signature · 1 line
2.15.1
RFC v4 11/17 lockdep: Take read/write status in consideration when generate chainkey
In reply to Boqun FengCurrently, the chainkey of a lock chain is a hash sum of the class_idx of all the held locks, the read/write status are not taken in to consideration while generating the chainkey. This could result into a problem, if we have:
P1()
{
read_lock(B);
lock(A);
}
P2()
{
lock(A);
read_lock(B);
}
P3()
{
lock(A);
write_lock(B);
}, and P1(), P2(), P3() run one by one. And when running P2(), lockdep detects such a lock chain A -> B is not a deadlock, then it's added in the chain cache, and then when running P3(), even if it's a deadlock, we could miss it because of the hit of chain cache. This could be confirmed by self testcase "chain cached mixed R-L/L-W ".
To resolve this, we use concept"hlock_id" to generate the chainkey, the hlock_id is a tuple (hlock->class_idx, hlock->read), which fits in a u16 type. With this, the chainkeys are different is the lock sequences have the same locks but different read/write status.
Besides, since we use "hlock_id" to generate chainkeys, the chain_hlocks array now store the "hlock_id"s rather than lock_class indexes.
--- kernel/locking/lockdep.c | 56 +++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 20 deletions(-)
kernel/locking/lockdep.c +36 -20
@@ -306,6 +306,21 @@ static struct hlist_head classhash_table[CLASSHASH_SIZE];static struct hlist_head chainhash_table[CHAINHASH_SIZE ]; +/* +* the id chain_hlocks +*/ +static inline u16 hlock_id (struct held_lock *hlock) +{ +BUILD_BUG_ON (MAX_LOCKDEP_KEYS_BITS +2 >16 ); + +return (hlock->class_idx | (hlock->read <<MAX_LOCKDEP_KEYS_BITS )); +} + +static inline unsigned int chain_hlock_class_idx (u16 hlock_id) +{ +return hlock_id &MAX_LOCKDEP_KEYS ; +} +/* * The hash key of the lock dependency chains is a hash itself too: * it's a hash of all locks taken up to that lock, including that lock.
@@ -2186,7 +2201,10 @@ static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];struct lock_class *lock_chain_get_class (struct lock_chain *chain,int i) { -return lock_classes + chain_hlocks[chain->base + i]; +u16 chain_hlock = chain_hlocks[chain->base + i]; +unsigned int class_idx =chain_hlock_class_idx (chain_hlock); + +return lock_classes + class_idx -1 ; }/*
@@ -2212,12 +2230,12 @@ static inline int get_first_held_lock(struct task_struct *curr,/* * Returns the next chain_key iteration */ -static u64 print_chain_key_iteration (int class_idx,u64 chain_key) +static u64 print_chain_key_iteration (u16 hlock_id,u64 chain_key) { -u64 new_chain_key =iterate_chain_key (chain_key, class_idx); +u64 new_chain_key =iterate_chain_key (chain_key, hlock_id); -printk (" class_idx:%d -> chain_key:%016Lx" , - class_idx, +printk (" hlock_id:%d -> chain_key:%016Lx" , + (unsigned int )hlock_id, (unsigned long long )new_chain_key);return new_chain_key; }
@@ -2233,12 +2251,12 @@ print_chain_keys_held_locks(struct task_struct *curr, struct held_lock *hlock_neprintk ("depth: %u\n" , depth +1 );for (i =get_first_held_lock (curr, hlock_next); i < depth; i++) { hlock = curr->held_locks + i; - chain_key =print_chain_key_iteration (hlock->class_idx, chain_key); + chain_key =print_chain_key_iteration (hlock_id (hlock), chain_key);print_lock (hlock); } -print_chain_key_iteration (hlock_next->class_idx, chain_key); +print_chain_key_iteration (hlock_id (hlock_next), chain_key);print_lock (hlock_next); }
@@ -2246,14 +2264,14 @@ static void print_chain_keys_chain(struct lock_chain *chain) {int i;u64 chain_key =0 ; -int class_id; +u16 hlock_id;printk ("depth: %u\n" , chain->depth);for (i =0 ; i < chain->depth; i++) { - class_id = chain_hlocks[chain->base + i]; - chain_key =print_chain_key_iteration (class_id +1 , chain_key); + hlock_id = chain_hlocks[chain->base + i]; + chain_key =print_chain_key_iteration (hlock_id, chain_key); -print_lock_name (lock_classes + class_id); +print_lock_name (lock_classes +chain_hlock_class_idx (hlock_id) -1 );printk ("\n" ); } }
@@ -2302,7 +2320,7 @@ static int check_no_collision(struct task_struct *curr, }for (j =0 ; j < chain->depth -1 ; j++, i++) { - id = curr->held_locks[i].class_idx -1 ; + id =hlock_id (&curr->held_locks[i]);if (DEBUG_LOCKS_WARN_ON (chain_hlocks[chain->base + j] != id)) {print_collision (curr, hlock, chain);
@@ -2359,8 +2377,8 @@ static inline int add_chain_cache_classes(unsigned int prev,if (likely (nr_chain_hlocks + chain->depth <=MAX_LOCKDEP_CHAIN_HLOCKS )) { chain->base = nr_chain_hlocks; nr_chain_hlocks += chain->depth; - chain_hlocks[chain->base] = prev -1 ; - chain_hlocks[chain->base +1 ] = next -1 ; + chain_hlocks[chain->base] = prev; + chain_hlocks[chain->base +1 ] = next; }#ifdef CONFIG_DEBUG_LOCKDEP /*
@@ -2394,7 +2412,6 @@ static inline int add_chain_cache(struct task_struct *curr, struct held_lock *hlock, u64 chain_key) { -struct lock_class *class =hlock_class (hlock);struct hlist_head *hash_head =chainhashentry (chain_key);struct lock_chain *chain;int i, j;
@@ -2433,10 +2450,9 @@ static inline int add_chain_cache(struct task_struct *curr,if (likely (nr_chain_hlocks + chain->depth <=MAX_LOCKDEP_CHAIN_HLOCKS )) { chain->base = nr_chain_hlocks;for (j =0 ; j < chain->depth -1 ; j++, i++) { -int lock_id = curr->held_locks[i].class_idx -1 ; - chain_hlocks[chain->base + j] = lock_id; + chain_hlocks[chain->base + j] =hlock_id (&curr->held_locks[i]); } - chain_hlocks[chain->base + j] = class - lock_classes; + chain_hlocks[chain->base + j] =hlock_id (hlock); }if (nr_chain_hlocks <MAX_LOCKDEP_CHAIN_HLOCKS )
@@ -2634,7 +2650,7 @@ static void check_chain_key(struct task_struct *curr)if (prev_hlock && (prev_hlock->irq_context != hlock->irq_context)) chain_key =0 ; - chain_key =iterate_chain_key (chain_key, hlock->class_idx); + chain_key =iterate_chain_key (chain_key,hlock_id (hlock)); prev_hlock = hlock; }if (chain_key != curr->curr_chain_key) {
@@ -3585,7 +3601,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, chain_key =0 ; chain_head =1 ; } - chain_key =iterate_chain_key (chain_key, class_idx); + chain_key =iterate_chain_key (chain_key,hlock_id (hlock));if (nest_lock && !__lock_is_held (nest_lock,-1 ))return print_lock_nested_lock_not_held (curr, hlock, ip);
Signature · 1 line
2.15.1
RFC v4 12/17 lockdep/selftest: Unleash irq_read_recursion2 and add more
In reply to Boqun FengNow since we can handle recursive read related irq inversion deadlocks correctly, uncomment the irq_read_recursion2 and add more testcases.
--- lib/locking-selftest.c | 59 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 12 deletions(-)
lib/locking-selftest.c +47 -12
@@ -1052,20 +1052,28 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_inversion_soft_wlock)#define E3 () \ \ IRQ_ENTER(); \ - RL(A); \ + LOCK(A); \ L(B); \ U(B); \ - RU(A); \ + UNLOCK(A); \ IRQ_EXIT();/* -* Generate 12 testcases: +* Generate 24 testcases: */ #include "locking-selftest-hardirq.h" -GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_hard) +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_hard_rlock) + +#include "locking-selftest-wlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_hard_wlock)#include "locking-selftest-softirq.h" -GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft) +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft_rlock) + +#include "locking-selftest-wlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft_wlock)#undef E1#undef E2
@@ -1079,8 +1087,8 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft) \IRQ_DISABLE (); \L (B ); \ -WL (A ); \ -WU (A ); \ +LOCK (A ); \ +UNLOCK (A ); \U (B ); \IRQ_ENABLE ();
@@ -1097,13 +1105,21 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft)IRQ_EXIT ();/* -* Generate 12 testcases: +* Generate 24 testcases: */ #include "locking-selftest-hardirq.h" -// GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_hard) +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_hard_rlock) + +#include "locking-selftest-wlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_hard_wlock)#include "locking-selftest-softirq.h" -// GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_soft) +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_soft_rlock) + +#include "locking-selftest-wlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion2_soft_wlock)#ifdef CONFIG_DEBUG_LOCK_ALLOC # define I_SPINLOCK (x) lockdep_reset_lock(&lock_##x.dep_map)
@@ -1256,6 +1272,25 @@ static inline void print_testname(const char *testname) dotest(name##_rlock_##nr, SUCCESS, LOCKTYPE_RWLOCK); \ pr_cont("\n"); +#define DO_TESTCASE_2RW (desc, name, nr) \ + print_testname(desc"/"#nr); \ + pr_cont(" |"); \ + dotest(name##_wlock_##nr, FAILURE, LOCKTYPE_RWLOCK); \ + dotest(name##_rlock_##nr, SUCCESS, LOCKTYPE_RWLOCK); \ + pr_cont("\n"); + +#define DO_TESTCASE_2x2RW (desc, name, nr) \ + DO_TESTCASE_2RW("hard-"desc, name##_hard, nr) \ + DO_TESTCASE_2RW("soft-"desc, name##_soft, nr) \ + +#define DO_TESTCASE_6x2x2RW (desc, name) \ + DO_TESTCASE_2x2RW(desc, name, 123); \ + DO_TESTCASE_2x2RW(desc, name, 132); \ + DO_TESTCASE_2x2RW(desc, name, 213); \ + DO_TESTCASE_2x2RW(desc, name, 231); \ + DO_TESTCASE_2x2RW(desc, name, 312); \ + DO_TESTCASE_2x2RW(desc, name, 321); +#define DO_TESTCASE_6 (desc, name) \ print_testname(desc); \ dotest(name##_spin, FAILURE, LOCKTYPE_SPIN); \
@@ -2114,8 +2149,8 @@ void locking_selftest(void)DO_TESTCASE_6x6 ("safe-A + unsafe-B #2" , irqsafe4);DO_TESTCASE_6x6RW ("irq lock-inversion" , irq_inversion); -DO_TESTCASE_6x2 ("irq read-recursion" , irq_read_recursion); -// DO_TESTCASE_6x2B("irq read-recursion #2", irq_read_recursion2); +DO_TESTCASE_6x2x2RW ("irq read-recursion" , irq_read_recursion); +DO_TESTCASE_6x2x2RW ("irq read-recursion #2" , irq_read_recursion2);ww_tests ();
Signature · 1 line
2.15.1
RFC v4 13/17 lockdep/selftest: Add more recursive read related test cases
In reply to Boqun FengAdd those four test cases:
1. X --(NR)--> Y --(NR)--> Z --(NR)--> X is deadlock.
2. X --(NN)--> Y --(RR)--> Z --(NR)--> X is deadlock.
3. X --(NN)--> Y --(RR)--> Z --(RN)--> X is not deadlock.
4. X --(NR)--> Y --(RR)--> Z --(NN)--> X is not deadlock.
Those self testcases are valuable for the development of supporting recursive read related deadlock detection.
--- lib/locking-selftest.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
lib/locking-selftest.c +161 -0
@@ -1033,6 +1033,133 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_inversion_soft_wlock)#undef E2#undef E3 +/* +* write-read / write-read / write-read deadlock even if read is recursive +*/ + +#define E1 () \ + \ + WL(X1); \ + RL(Y1); \ + RU(Y1); \ + WU(X1); + +#define E2 () \ + \ + WL(Y1); \ + RL(Z1); \ + RU(Z1); \ + WU(Y1); + +#define E3 () \ + \ + WL(Z1); \ + RL(X1); \ + RU(X1); \ + WU(Z1); + +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(W1R2_W2R3_W3R1) + +#undef E1 +#undef E2 +#undef E3 + +/* +* write-write / read-read / write-read deadlock even if read is recursive +*/ + +#define E1 () \ + \ + WL(X1); \ + WL(Y1); \ + WU(Y1); \ + WU(X1); + +#define E2 () \ + \ + RL(Y1); \ + RL(Z1); \ + RU(Z1); \ + RU(Y1); + +#define E3 () \ + \ + WL(Z1); \ + RL(X1); \ + RU(X1); \ + WU(Z1); + +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(W1W2_R2R3_W3R1) + +#undef E1 +#undef E2 +#undef E3 + +/* +* write-write / read-read / read-write is not deadlock when read is recursive +*/ + +#define E1 () \ + \ + WL(X1); \ + WL(Y1); \ + WU(Y1); \ + WU(X1); + +#define E2 () \ + \ + RL(Y1); \ + RL(Z1); \ + RU(Z1); \ + RU(Y1); + +#define E3 () \ + \ + RL(Z1); \ + WL(X1); \ + WU(X1); \ + RU(Z1); + +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(W1R2_R2R3_W3W1) + +#undef E1 +#undef E2 +#undef E3 + +/* +* write-read / read-read / write-write is not deadlock when read is recursive +*/ + +#define E1 () \ + \ + WL(X1); \ + RL(Y1); \ + RU(Y1); \ + WU(X1); + +#define E2 () \ + \ + RL(Y1); \ + RL(Z1); \ + RU(Z1); \ + RU(Y1); + +#define E3 () \ + \ + WL(Z1); \ + WL(X1); \ + WU(X1); \ + WU(Z1); + +#include "locking-selftest-rlock.h" +GENERATE_PERMUTATIONS_3_EVENTS(W1W2_R2R3_R3W1) + +#undef E1 +#undef E2 +#undef E3/* * read-lock / write-lock recursion that is actually safe. */
@@ -1258,6 +1385,19 @@ static inline void print_testname(const char *testname) dotest(name##_##nr, FAILURE, LOCKTYPE_RWLOCK); \ pr_cont("\n"); +#define DO_TESTCASE_1RR (desc, name, nr) \ + print_testname(desc"/"#nr); \ + pr_cont(" |"); \ + dotest(name##_##nr, SUCCESS, LOCKTYPE_RWLOCK); \ + pr_cont("\n"); + +#define DO_TESTCASE_1RRB (desc, name, nr) \ + print_testname(desc"/"#nr); \ + pr_cont(" |"); \ + dotest(name##_##nr, FAILURE, LOCKTYPE_RWLOCK); \ + pr_cont("\n"); + +#define DO_TESTCASE_3 (desc, name, nr) \ print_testname(desc"/"#nr); \ dotest(name##_spin_##nr, FAILURE, LOCKTYPE_SPIN); \
@@ -1367,6 +1507,22 @@ static inline void print_testname(const char *testname)DO_TESTCASE_2IB (desc, name,312 ); \DO_TESTCASE_2IB (desc, name,321 ); +#define DO_TESTCASE_6x1RR (desc, name) \ + DO_TESTCASE_1RR(desc, name, 123); \ + DO_TESTCASE_1RR(desc, name, 132); \ + DO_TESTCASE_1RR(desc, name, 213); \ + DO_TESTCASE_1RR(desc, name, 231); \ + DO_TESTCASE_1RR(desc, name, 312); \ + DO_TESTCASE_1RR(desc, name, 321); + +#define DO_TESTCASE_6x1RRB (desc, name) \ + DO_TESTCASE_1RRB(desc, name, 123); \ + DO_TESTCASE_1RRB(desc, name, 132); \ + DO_TESTCASE_1RRB(desc, name, 213); \ + DO_TESTCASE_1RRB(desc, name, 231); \ + DO_TESTCASE_1RRB(desc, name, 312); \ + DO_TESTCASE_1RRB(desc, name, 321); +#define DO_TESTCASE_6x6 (desc, name) \ DO_TESTCASE_6I(desc, name, 123); \ DO_TESTCASE_6I(desc, name, 132); \
@@ -2137,6 +2293,11 @@ void locking_selftest(void)pr_cont (" |" );dotest (rlock_chaincache_ABBA1,FAILURE ,LOCKTYPE_RWLOCK ); +DO_TESTCASE_6x1RRB ("rlock W1R2/W2R3/W3R1" ,W1R2_W2R3_W3R1 ); +DO_TESTCASE_6x1RRB ("rlock W1W2/R2R3/W3R1" ,W1W2_R2R3_W3R1 ); +DO_TESTCASE_6x1RR ("rlock W1W2/R2R3/R3W1" ,W1W2_R2R3_R3W1 ); +DO_TESTCASE_6x1RR ("rlock W1R2/R2R3/W3W1" ,W1R2_R2R3_W3W1 ); +printk (" --------------------------------------------------------------------------\n" );/*
Signature · 1 line
2.15.1
RFC v4 14/17 Revert "locking/lockdep/selftests: Fix mixed read-write ABBA tests"
In reply to Boqun FengThis reverts commit d82fed75294229abc9d757f08a4817febae6c4f4.
Since we now could handle mixed read-write deadlock detection well, the self tests could be detected as expected, no need to use this work-around.
--- lib/locking-selftest.c | 8 -------- 1 file changed, 8 deletions(-)
lib/locking-selftest.c +0 -8
@@ -2266,14 +2266,6 @@ void locking_selftest(void)print_testname ("mixed read-lock/lock-write ABBA" );pr_cont (" |" );dotest (rlock_ABBA1,FAILURE ,LOCKTYPE_RWLOCK ); -#ifdef CONFIG_PROVE_LOCKING -/* -* Lockdep does indeed fail here, but there's nothing we can do about -* that now. Don't kill lockdep for it. -*/ - unexpected_testcase_failures--; -#endif -pr_cont (" |" );dotest (rwsem_ABBA1,FAILURE ,LOCKTYPE_RWSEM );
Signature · 1 line
2.15.1
RFC v4 15/17 lockdep: Reduce the size of lock_list
In reply to Boqun FengWe actually only need 4 bits for lock_list::dep and 1 bit for lock_list::is_rr, besides lock_list::distance should always be no greater than MAX_LOCKDEP_DEPTH(which is 48 right now), so a u16 will fit, this patch then reduces the sizes of those fields to save space for lock_list structure, as a result we can reduce the size increment introduced by recursive read lock detection and keep the lock_list the same size as before.
--- include/linux/lockdep.h | 6 +++--- kernel/locking/lockdep.c | 11 ++++++----- 2 files changed, 9 insertions(+), 8 deletions(-)
include/linux/lockdep.h +3 -3
@@ -186,11 +186,11 @@ struct lock_list {struct list_head entry;struct lock_class *class;struct stack_trace trace; -int distance; +u16 distance;/* bitmap of different dependencies from head to this */ -u16 dep; +u8 dep;/* used by BFS to record whether this is picked as a recursive read */ -u16 is_rr; +bool is_rr;/* * The parent field is used to implement breadth-first search, and the
kernel/locking/lockdep.c +6 -5
@@ -869,7 +869,7 @@ static struct lock_list *alloc_list_entry(void)* Add a new dependency to the head of the list: */ static int add_lock_to_list (struct lock_class *this,struct list_head *head, -unsigned long ip,int distance,unsigned int dep, +unsigned long ip,u16 distance,unsigned int dep,struct stack_trace *trace) {struct lock_list *entry;
@@ -1058,7 +1058,7 @@ static inline unsigned int calc_dep(int prev, int next)* N: non-recursive lock * R: recursive read lock */ -static inline int pick_dep (u16 is_rr,u16 cap_dep) +static inline int pick_dep (bool is_rr,u8 cap_dep) {if (is_rr) {/* could only pick -(N*)-> */ if (cap_dep &DEP_NN_MASK )
@@ -1143,7 +1143,8 @@ static enum bfs_result __bfs(struct lock_list *source_entry,struct list_head *head;struct circular_queue *cq = &lock_cq;enum bfs_result ret =BFS_RNOMATCH ; -int is_rr, next_is_rr; +bool is_rr; +int next_is_rr;if (match (source_entry, data)) { *target_entry = source_entry;
@@ -1199,7 +1200,7 @@ static enum bfs_result __bfs(struct lock_list *source_entry, next_is_rr =pick_dep (is_rr, entry->dep);if (next_is_rr <0 )continue ; - entry->is_rr = next_is_rr; + entry->is_rr = !!next_is_rr;visit_lock_entry (entry, lock);if (match (entry, data)) {
@@ -2148,7 +2149,7 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next) goto out_bug;for (;;) { -int distance = curr->lockdep_depth - depth +1 ; +u16 distance = curr->lockdep_depth - depth +1 ; hlock = curr->held_locks + depth -1 ;if (hlock->check) {
Signature · 1 line
2.15.1
RFC v4 16/17 lockdep: Documention for recursive read lock detection reasoning
In reply to Boqun FengAs now we support recursive read lock deadlock detection, add related explanation in the Documentation/lockdep/lockdep-desgin.txt:
* Definition of recursive read locks, non-recursive locks, strong
dependency path and notions of -(**)->.
* Lockdep's assumption.
* Informal proof of recursive read lock deadlock detection.
--- Documentation/locking/lockdep-design.txt | 170 +++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+)
Documentation/locking/lockdep-design.txt +170 -0
@@ -284,3 +284,173 @@ Run the command and save the output, then compare against the output from a later run of this command to identify the leakers. This same output can also help you find situations where runtime lock initialization has been omitted. + +Recursive Read Deadlock Detection: +---------------------------------- +Lockdep now is equipped with deadlock detection for recursive read locks. + +Recursive read locks, as their name indicates, are the locks able to be +acquired recursively, unlike non-recursive read locks, recursive read locks +only get blocked by current write lock *holders* other than write lock +*waiters*, for example: + + TASK A: TASK B: + + read_lock(X); + + write_lock(X); + + read_lock(X); + +is not a deadlock for recursive read locks, as while the task B is waiting for +the lock X, the second read_lock() doesn't need to wait because it's a recursive +read lock. + +Note that a lock can be a write lock(exclusive lock), a non-recursive read lock +(non-recursive shared lock) or a recursive read lock(recursive shared lock), +depending on the API used to acquire it(more detailedly, the value of the +'read' parameter for lock_acquire(...)). In other words, a single lock instance +have three types of acquisition depending on the acquisition functions: +exclusive, non-recursive read, and recursive read. + +That said, recursive read locks could introduce deadlocks too, considering the +following: + + TASK A: TASK B: + + read_lock(X); + read_lock(Y); + write_lock(Y); + write_lock(X); + +, neither task could get the write locks because the corresponding read locks +are held by each other. + +Lockdep could detect recursive read lock related deadlocks. The dependencies(edges) +in the lockdep graph are classified into four categories: + +1) -(NN)->: non-recursive to non-recursive dependency, non-recursive locks include + non-recursive read locks, write locks and exclusive locks(e.g. spinlock_t), + they are treated equally in deadlock detection. "X -(NN)-> Y" means + X -> Y and both X and Y are non-recursive locks. + +2) -(RN)->: recursive to non-recursive dependency, recursive locks means recursive read + locks. "X -(RN)-> Y" means X -> Y and X is recursive read lock and + Y is non-recursive lock. + +3) -(NR)->: non-recursive to recursive dependency, "X -(NR)-> Y" means X -> Y and X is + non-recursive lock and Y is recursive lock. + +4) -(RR)->: recursive to recursive dependency, "X -(RR)-> Y" means X -> Y and both X + and Y are recursive locks. + +Note that given two locks, they may have multiple dependencies between them, for example: + + TASK A: + + read_lock(X); + write_lock(Y); + ... + + TASK B: + + write_lock(X); + write_lock(Y); + +, we have both X -(RN)-> Y and X -(NN)-> Y in the dependency graph. + +And obviously a non-recursive lock can block the corresponding recursive lock, +and vice versa. Besides a non-recursive lock may block the other non-recursive +lock of the same instance(e.g. a write lock may block a corresponding +non-recursive read lock and vice versa). + +We use -(*N)-> for edges that is either -(RN)-> or -(NN)->, the similar for -(N*)->, +-(*R)-> and -(R*)-> + +A "path" is a series of conjunct dependency edges in the graph. And we define a +"strong" path, which indicates the strong dependency throughout each dependency +in the path, as the path that doesn't have two conjunct edges(dependencies) as +-(*R)-> and -(R*)->. IOW, a "strong" path is a path from a lock walking to another +through the lock dependencies, and if X -> Y -> Z in the path(where X, Y, Z are +locks), if the walk from X to Y is through a -(NR)-> or -(RR)-> dependency, the +walk from Y to Z must not be through a -(RN)-> or -(RR)-> dependency, otherwise +it's not a strong path. + +We now prove that if a strong path forms a circle, then we have a potential deadlock. +By "forms a circle", it means for a set of locks A0,A1...An, there is a path from +A0 to An: + + A0 -> A1 -> ... -> An + +and there is also a dependency An->A0. And as the circle is formed by a strong path, +so there is no two conjunct dependency edges as -(*R)-> and -(R*)->. + + +To understand the actual proof, let's look into lockdep's assumption: + +For each lockdep dependency A -> B, there may exist a case where someone is +trying to acquire B with A held, and the existence of such a case is +independent to the existences of cases for other lockdep dependencies. + +For example if we have two functions func1 and func2: + + void func1(...) { + lock(A); + lock(B); + unlock(A); + unlock(B); + + lock(C); + lock(A); + unlock(A); + unlock(C); + } + + void func2(...) { + lock(B); + lock(C); + unlock(C); + unlock(B); + } + +lockdep will generate dependencies: A->B, B->C and C->A, and assume that: + + there may exist a case where someone is trying to acquire B with A held, + there may exist a case where someone is trying to acquire C with B held, + and there may exist a case where someone is trying to acquire A with C held, + +and those three cases exists *independently*, meaning they can happen at the +same time(which requires func1() being called simultaneously by two CPUs or +tasks, which may be impossible due to other constraints in the real life) + + +With this assumption, we can prove that if a strong dependency path forms a circle, +then it indicate a deadlock as far as lockdep is concerned. + +For a strong dependency circle like: + + A0 -> A1 -> ... -> An + ^ | + | | + +------------------+ +, lockdep assumes that + + there may exist a case where someone is trying to acquire A1 with A0 held + there may exist a case where someone is trying to acquire A2 with A1 held + ... + there may exist a case where someone is trying to acquire An with An-1 held + there may exist a case where someone is trying to acquire A0 with An held + +, and because it's a *strong* dependency circle for every Ai (0<=i<=n), Ai is either +held as a non-recursive lock or someone is trying to acquire Ai as a non-recursive lock, +which gives: + +* If Ai is held as a non-recursive lock, then trying to acquire it, + whether as a recursive lock or not, will get blocked. + +* If Ai is held as a recursive lock, then there must be someone is trying to acquire + it as a non-recursive lock, and because recursive locks blocks non-recursive locks, + then it(the "someone") will get blocked. + +So all the holders of A0, A1...An are blocked with A0, A1...An held by each other, +no one can progress, therefore deadlock.
Signature · 1 line
2.15.1
Randy Dunlap
reviewerRe: [RFC tip/locking/lockdep v4 16/17] lockdep: Documention for recursive read lock detection reasoning
Quoted 20 lines · Boqun Feng
As now we support recursive read lock deadlock detection, add related explanation in the
Documentation/lockdep/lockdep-desgin.txt:* Definition of recursive read locks, non-recursive locks, strong
dependency path and notions of -(**)->.* Lockdep's assumption.
* Informal proof of recursive read lock deadlock detection.
Signed-off-by Boqun Feng <[email protected]>--- Documentation/locking/lockdep-design.txt | 170 +++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+)Documentation/locking/lockdep-design.txt +0
-0
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 293
Quoted 10 lines
Documentation/locking/lockdep-design.txt +7
-0@@ -284,3 +284,173 @@ Run the command and save the output, then compare against the output from a later run of this command to identify the leakers. This same output can also help you find situations where runtime lock initialization has been omitted. + +Recursive Read Deadlock Detection: +---------------------------------- +Lockdep now is equipped with deadlock detection for recursive read locks. + +Recursive read locks, as their name indicates, are the locks able to be +acquired recursively, unlike non-recursive read locks, recursive read locks
recursively. Unlike
Quoted 8 lines
Documentation/locking/lockdep-design.txt +8
-0+only get blocked by current write lock *holders* other than write lock +*waiters*, for example: + + TASK A: TASK B: + + read_lock(X); + + write_lock(X);
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 311
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ + read_lock(X); + +is not a deadlock for recursive read locks, as while the task B is waiting for +the lock X, the second read_lock() doesn't need to wait because it's a recursive +read lock. + +Note that a lock can be a write lock(exclusive lock), a non-recursive read lock +(non-recursive shared lock) or a recursive read lock(recursive shared lock), +depending on the API used to acquire it(more detailedly, the value of the
more specifically,
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 313
Quoted 2 lines
Documentation/locking/lockdep-design.txt +2
-0+'read' parameter for lock_acquire(...)). In other words, a single lock instance +have three types of acquisition depending on the acquisition functions:
has three types
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+exclusive, non-recursive read, and recursive read. + +That said, recursive read locks could introduce deadlocks too, considering the +following: + + TASK A: TASK B: + + read_lock(X); + read_lock(Y); + write_lock(Y);
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 333
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ write_lock(X); + +, neither task could get the write locks because the corresponding read locks +are held by each other. + +Lockdep could detect recursive read lock related deadlocks. The dependencies(edges) +in the lockdep graph are classified into four categories: + +1) -(NN)->: non-recursive to non-recursive dependency, non-recursive locks include + non-recursive read locks, write locks and exclusive locks(e.g. spinlock_t),
spinlock_t).
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 334
Quoted 1 line
Documentation/locking/lockdep-design.txt +1
-0+ they are treated equally in deadlock detection. "X -(NN)-> Y" means
They
Quoted 42 lines
Documentation/locking/lockdep-design.txt +42
-0+ X -> Y and both X and Y are non-recursive locks. + +2) -(RN)->: recursive to non-recursive dependency, recursive locks means recursive read + locks. "X -(RN)-> Y" means X -> Y and X is recursive read lock and + Y is non-recursive lock. + +3) -(NR)->: non-recursive to recursive dependency, "X -(NR)-> Y" means X -> Y and X is + non-recursive lock and Y is recursive lock. + +4) -(RR)->: recursive to recursive dependency, "X -(RR)-> Y" means X -> Y and both X + and Y are recursive locks. + +Note that given two locks, they may have multiple dependencies between them, for example: + + TASK A: + + read_lock(X); + write_lock(Y); + ... + + TASK B: + + write_lock(X); + write_lock(Y); + +, we have both X -(RN)-> Y and X -(NN)-> Y in the dependency graph. + +And obviously a non-recursive lock can block the corresponding recursive lock, +and vice versa. Besides a non-recursive lock may block the other non-recursive +lock of the same instance(e.g. a write lock may block a corresponding +non-recursive read lock and vice versa). + +We use -(*N)-> for edges that is either -(RN)-> or -(NN)->, the similar for -(N*)->, +-(*R)-> and -(R*)-> + +A "path" is a series of conjunct dependency edges in the graph. And we define a +"strong" path, which indicates the strong dependency throughout each dependency +in the path, as the path that doesn't have two conjunct edges(dependencies) as +-(*R)-> and -(R*)->. IOW, a "strong" path is a path from a lock walking to another +through the lock dependencies, and if X -> Y -> Z in the path(where X, Y, Z are +locks), if the walk from X to Y is through a -(NR)-> or -(RR)-> dependency, the +walk from Y to Z must not be through a -(RN)-> or -(RR)-> dependency, otherwise
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 386
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+it's not a strong path. + +We now prove that if a strong path forms a circle, then we have a potential deadlock. +By "forms a circle", it means for a set of locks A0,A1...An, there is a path from +A0 to An: + + A0 -> A1 -> ... -> An + +and there is also a dependency An->A0. And as the circle is formed by a strong path, +so there is no two conjunct dependency edges as -(*R)-> and -(R*)->.
I would say:
there are no two
Quoted 26 lines
Documentation/locking/lockdep-design.txt +26
-0+ + +To understand the actual proof, let's look into lockdep's assumption: + +For each lockdep dependency A -> B, there may exist a case where someone is +trying to acquire B with A held, and the existence of such a case is +independent to the existences of cases for other lockdep dependencies. + +For example if we have two functions func1 and func2: + + void func1(...) { + lock(A); + lock(B); + unlock(A); + unlock(B); + + lock(C); + lock(A); + unlock(A); + unlock(C); + } + + void func2(...) { + lock(B); + lock(C); + unlock(C);
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 422
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ unlock(B); + } + +lockdep will generate dependencies: A->B, B->C and C->A, and assume that: + + there may exist a case where someone is trying to acquire B with A held, + there may exist a case where someone is trying to acquire C with B held, + and there may exist a case where someone is trying to acquire A with C held, + +and those three cases exists *independently*, meaning they can happen at the
exist
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 424
Quoted 2 lines
Documentation/locking/lockdep-design.txt +2
-0+same time(which requires func1() being called simultaneously by two CPUs or +tasks, which may be impossible due to other constraints in the real life)
life).
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 428
Quoted 4 lines
Documentation/locking/lockdep-design.txt +4
-0+ + +With this assumption, we can prove that if a strong dependency path forms a circle, +then it indicate a deadlock as far as lockdep is concerned.
indicates
Quoted 8 lines
Documentation/locking/lockdep-design.txt +8
-0+ +For a strong dependency circle like: + + A0 -> A1 -> ... -> An + ^ | + | | + +------------------+ +, lockdep assumes that
...
Signature · 1 line
~Randy
Re: [RFC tip/locking/lockdep v4 16/17] lockdep: Documention for recursive read lock detection reasoning
Quoted 134 lines · Randy Dunlap
Quoted 20 lines · Boqun Feng
As now we support recursive read lock deadlock detection, add related explanation in the
Documentation/lockdep/lockdep-desgin.txt:* Definition of recursive read locks, non-recursive locks, strong
dependency path and notions of -(**)->.* Lockdep's assumption.
* Informal proof of recursive read lock deadlock detection.
Signed-off-by Boqun Feng <[email protected]>--- Documentation/locking/lockdep-design.txt | 170 +++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+)Documentation/locking/lockdep-design.txt +0
-0Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 293
Quoted 10 lines
Documentation/locking/lockdep-design.txt +7
-0@@ -284,3 +284,173 @@ Run the command and save the output, then compare against the output from a later run of this command to identify the leakers. This same output can also help you find situations where runtime lock initialization has been omitted. + +Recursive Read Deadlock Detection: +---------------------------------- +Lockdep now is equipped with deadlock detection for recursive read locks. + +Recursive read locks, as their name indicates, are the locks able to be +acquired recursively, unlike non-recursive read locks, recursive read locksrecursively. Unlike
Quoted 8 lines
Documentation/locking/lockdep-design.txt +8
-0+only get blocked by current write lock *holders* other than write lock +*waiters*, for example: + + TASK A: TASK B: + + read_lock(X); + + write_lock(X);Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 311
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ + read_lock(X); + +is not a deadlock for recursive read locks, as while the task B is waiting for +the lock X, the second read_lock() doesn't need to wait because it's a recursive +read lock. + +Note that a lock can be a write lock(exclusive lock), a non-recursive read lock +(non-recursive shared lock) or a recursive read lock(recursive shared lock), +depending on the API used to acquire it(more detailedly, the value of themore specifically,
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 313
Quoted 2 lines
Documentation/locking/lockdep-design.txt +2
-0+'read' parameter for lock_acquire(...)). In other words, a single lock instance +have three types of acquisition depending on the acquisition functions:has three types
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+exclusive, non-recursive read, and recursive read. + +That said, recursive read locks could introduce deadlocks too, considering the +following: + + TASK A: TASK B: + + read_lock(X); + read_lock(Y); + write_lock(Y);Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 333
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ write_lock(X); + +, neither task could get the write locks because the corresponding read locks +are held by each other. + +Lockdep could detect recursive read lock related deadlocks. The dependencies(edges) +in the lockdep graph are classified into four categories: + +1) -(NN)->: non-recursive to non-recursive dependency, non-recursive locks include + non-recursive read locks, write locks and exclusive locks(e.g. spinlock_t),
spinlock_t).Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 334
Quoted 1 line
Documentation/locking/lockdep-design.txt +1
-0+ they are treated equally in deadlock detection. "X -(NN)-> Y" meansThey
Quoted 47 lines
Documentation/locking/lockdep-design.txt +47
-0+ X -> Y and both X and Y are non-recursive locks. + +2) -(RN)->: recursive to non-recursive dependency, recursive locks means recursive read + locks. "X -(RN)-> Y" means X -> Y and X is recursive read lock and + Y is non-recursive lock. + +3) -(NR)->: non-recursive to recursive dependency, "X -(NR)-> Y" means X -> Y and X is + non-recursive lock and Y is recursive lock. + +4) -(RR)->: recursive to recursive dependency, "X -(RR)-> Y" means X -> Y and both X + and Y are recursive locks. + +Note that given two locks, they may have multiple dependencies between them, for example: + + TASK A: + + read_lock(X); + write_lock(Y); + ... + + TASK B: + + write_lock(X); + write_lock(Y); + +, we have both X -(RN)-> Y and X -(NN)-> Y in the dependency graph. + +And obviously a non-recursive lock can block the corresponding recursive lock, +and vice versa. Besides a non-recursive lock may block the other non-recursive +lock of the same instance(e.g. a write lock may block a corresponding +non-recursive read lock and vice versa). + +We use -(*N)-> for edges that is either -(RN)-> or -(NN)->, the similar for -(N*)->, +-(*R)-> and -(R*)-> + +A "path" is a series of conjunct dependency edges in the graph. And we define a +"strong" path, which indicates the strong dependency throughout each dependency +in the path, as the path that doesn't have two conjunct edges(dependencies) as +-(*R)-> and -(R*)->. IOW, a "strong" path is a path from a lock walking to another +through the lock dependencies, and if X -> Y -> Z in the path(where X, Y, Z are +locks), if the walk from X to Y is through a -(NR)-> or -(RR)-> dependency, the +walk from Y to Z must not be through a -(RN)-> or -(RR)-> dependency, otherwise +it's not a strong path. + +We now prove that if a strong path forms a circle, then we have a potential deadlock. +By "forms a circle", it means for a set of locks A0,A1...An, there is a path from +A0 to An:
Quoted 9 lines
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 386
Quoted 5 lines
Documentation/locking/lockdep-design.txt +5
-0+ + A0 -> A1 -> ... -> An + +and there is also a dependency An->A0. And as the circle is formed by a strong path, +so there is no two conjunct dependency edges as -(*R)-> and -(R*)->.I would say:
there are no two
Agreed. I've applied other fixes in your comments. Thank you!
Regards,
Boqun
Quoted 64 lines
Quoted 26 lines
Documentation/locking/lockdep-design.txt +26
-0+ + +To understand the actual proof, let's look into lockdep's assumption: + +For each lockdep dependency A -> B, there may exist a case where someone is +trying to acquire B with A held, and the existence of such a case is +independent to the existences of cases for other lockdep dependencies. + +For example if we have two functions func1 and func2: + + void func1(...) { + lock(A); + lock(B); + unlock(A); + unlock(B); + + lock(C); + lock(A); + unlock(A); + unlock(C); + } + + void func2(...) { + lock(B); + lock(C); + unlock(C);Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 422
Quoted 10 lines
Documentation/locking/lockdep-design.txt +10
-0+ unlock(B); + } + +lockdep will generate dependencies: A->B, B->C and C->A, and assume that: + + there may exist a case where someone is trying to acquire B with A held, + there may exist a case where someone is trying to acquire C with B held, + and there may exist a case where someone is trying to acquire A with C held, + +and those three cases exists *independently*, meaning they can happen at theexist
Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 424
Quoted 2 lines
Documentation/locking/lockdep-design.txt +2
-0+same time(which requires func1() being called simultaneously by two CPUs or +tasks, which may be impossible due to other constraints in the real life)life).Documentation/locking/lockdep-design.txt Run the command and save the output, then compare against the output from · line 428
Quoted 4 lines
Documentation/locking/lockdep-design.txt +4
-0+ + +With this assumption, we can prove that if a strong dependency path forms a circle, +then it indicate a deadlock as far as lockdep is concerned.indicates
Quoted 8 lines
Documentation/locking/lockdep-design.txt +8
-0+ +For a strong dependency circle like: + + A0 -> A1 -> ... -> An + ^ | + | | + +------------------+ +, lockdep assumes that...
Signature · 1 line
~Randy
RFC v4 17/17 MAINTAINERS: Add myself as a LOCKING PRIMITIVES reviewer
In reply to Boqun FengRecursive read lock detection work touches most core part of lockdep, so add myself as a dedicated reviewer to help people find me if any of my code introduces problems or misunderstandings, also if they need my help on parsing logs related to recursive read locks.
Besides, I'd like to provide any help for lock related code.
--- MAINTAINERS | 1 + 1 file changed, 1 insertion(+)
MAINTAINERS +1 -0
@@ -8177,6 +8177,7 @@ F: Documentation/admin-guide/LSM/LoadPin.rst LOCKING PRIMITIVES M: Peter Zijlstra <[email protected]> M: Ingo Molnar <[email protected]> +R: Boqun Feng <[email protected]> L: [email protected] T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core S: Maintained
Signature · 1 line
2.15.1