On Tue, Aug 22, 2017 at 12:08:40PM +0200, Peter Zijlstra wrote: > > > > I meant: > > > > > > > > mutex_lock(&A) > > > > <work> > > > > lockdep_map_acquire_read(&work) > > > > mutex_lock(&A) > > > > > > > > lockdep_map_acquire(&work) > > > > flush_work(&work) > > > > > > > > I mean it can still be detected with a read acquisition in work. > > > > Am I wrong? > > > > > > Think so, although there's something weird with read locks that I keep > > > forgetting. But I'm not sure it'll actually solve the problem. But I can > > > > I mean, read acquisitions are nothing but ones allowing read ones to be > > re-acquired legally, IOW, we want to check entrance of flush_work() and > > works, not between works. That's why I suggested to use read ones in work > > in that case. > > Does seem to work.
So I think we'll end up hitting a lockdep deficiency and not trigger the splat on flush_work(), see also: https://lwn.net/Articles/332801/ lock_map_acquire_read() is a read-recursive and will not in fact create any dependencies because of this issue. In specific, check_prev_add() has: if (next->read == 2 || prev->read == 2) return 1; This means that for: lock_map_acquire_read(W)(2) down_write(A) (0) down_write(A) (0) wait_for_completion(C) (0) lock_map_acquire_read(W)(2) complete(C) (0) All the (2) effectively go away and 'solve' our current issue, but: lock_map_acquire_read(W)(2) mutex_lock(A) (0) mutex_lock(A) (0) lock_map_acquire(W) (0) as per flush_work() will not in fact trigger anymore either. See also the below locking-selftest changes. Now, this means I also have to consider the existing lock_map_acquire_read() users and if they really wanted to be recursive or not. When I change lock_map_acquire_read() to use lock_acquire_shared() this annotation no longer suffices and the splat comes back. Also, the acquire_read() annotation will (obviously) no longer work to cure this problem when we switch to normal read (1), because then the generated chain: W(1) -> A(0) -> C(0) -> W(1) spells deadlock, since W isn't allowed to recurse. /me goes dig through commit: e159489baa71 ("workqueue: relax lockdep annotation on flush_work()") to figure out wth the existing users really want. [ 0.000000] ---------------------------------------------------------------------------- [ 0.000000] | spin |wlock |rlock |mutex | wsem | rsem | [ 0.000000] -------------------------------------------------------------------------- [ 0.000000] -------------------------------------------------------------------------- [ 0.000000] recursive read-lock: | ok | | ok | [ 0.000000] recursive read-lock #2: | ok | | ok | [ 0.000000] mixed read-write-lock: | ok | | ok | [ 0.000000] mixed write-read-lock: | ok | | ok | [ 0.000000] mixed read-lock/lock-write ABBA: |FAILED| | ok | [ 0.000000] mixed read-lock/lock-read ABBA: | ok | | ok | [ 0.000000] mixed write-lock/lock-write ABBA: | ok | | ok | [ 0.000000] -------------------------------------------------------------------------- --- lib/locking-selftest.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 116 insertions(+), 1 deletion(-) diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 6f2b135dc5e8..b99d365cf399 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -363,6 +363,103 @@ static void rsem_AA3(void) } /* + * read_lock(A) + * spin_lock(B) + * spin_lock(B) + * write_lock(A) + */ +static void rlock_ABBA1(void) +{ + RL(X1); + L(Y1); + U(Y1); + RU(X1); + + L(Y1); + WL(X1); + WU(X1); + U(Y1); // should fail +} + +static void rwsem_ABBA1(void) +{ + RSL(X1); + ML(Y1); + MU(Y1); + RSU(X1); + + ML(Y1); + WSL(X1); + WSU(X1); + MU(Y1); // should fail +} + +/* + * read_lock(A) + * spin_lock(B) + * spin_lock(B) + * read_lock(A) + */ +static void rlock_ABBA2(void) +{ + RL(X1); + L(Y1); + U(Y1); + RU(X1); + + L(Y1); + RL(X1); + RU(X1); + U(Y1); // should NOT fail +} + +static void rwsem_ABBA2(void) +{ + RSL(X1); + ML(Y1); + MU(Y1); + RSU(X1); + + ML(Y1); + RSL(X1); + RSU(X1); + MU(Y1); // should fail +} + + +/* + * write_lock(A) + * spin_lock(B) + * spin_lock(B) + * write_lock(A) + */ +static void rlock_ABBA3(void) +{ + WL(X1); + L(Y1); + U(Y1); + WU(X1); + + L(Y1); + WL(X1); + WU(X1); + U(Y1); // should fail +} + +static void rwsem_ABBA3(void) +{ + WSL(X1); + ML(Y1); + MU(Y1); + WSU(X1); + + ML(Y1); + WSL(X1); + WSU(X1); + MU(Y1); // should fail +} + +/* * ABBA deadlock: */ @@ -1057,7 +1154,7 @@ static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask) unexpected_testcase_failures++; pr_cont("FAILED|"); - dump_stack(); +// dump_stack(); } else { testcase_successes++; pr_cont(" ok |"); @@ -1933,6 +2030,24 @@ void locking_selftest(void) dotest(rsem_AA3, FAILURE, LOCKTYPE_RWSEM); pr_cont("\n"); + print_testname("mixed read-lock/lock-write ABBA"); + pr_cont(" |"); + dotest(rlock_ABBA1, FAILURE, LOCKTYPE_RWLOCK); + pr_cont(" |"); + dotest(rwsem_ABBA1, FAILURE, LOCKTYPE_RWSEM); + + print_testname("mixed read-lock/lock-read ABBA"); + pr_cont(" |"); + dotest(rlock_ABBA2, SUCCESS, LOCKTYPE_RWLOCK); + pr_cont(" |"); + dotest(rwsem_ABBA2, FAILURE, LOCKTYPE_RWSEM); + + print_testname("mixed write-lock/lock-write ABBA"); + pr_cont(" |"); + dotest(rlock_ABBA3, FAILURE, LOCKTYPE_RWLOCK); + pr_cont(" |"); + dotest(rwsem_ABBA3, FAILURE, LOCKTYPE_RWSEM); + printk(" --------------------------------------------------------------------------\n"); /*
