On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote:
> 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.
It should be triggered. Lockdep code should be fixed so that it does.
> 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)
Please explain what W/A/C stand for.
>
> 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");
>
> /*
