Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1717991
| From | Byungchul Park <byungchul.park@lge.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING |
| Date | 2017-08-23 04:50 +0200 |
| Message-ID | <uhtTX-34H-1@gated-at.bofh.it> (permalink) |
| References | (4 earlier) <uhdcu-8mO-5@gated-at.bofh.it> <uhdFw-ng-21@gated-at.bofh.it> <uhdPb-qO-1@gated-at.bofh.it> <uheie-T0-27@gated-at.bofh.it> <uhhSP-39j-21@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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");
>
> /*
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
[PATCH v3 2/3] lockdep: Reword title of LOCKDEP_CROSSRELEASE config Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
[tip:locking/core] locking/lockdep: Reword title of LOCKDEP_CROSSRELEASE config tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
[PATCH v3 3/3] lockdep: Rename LOCKDEP_COMPLETE config Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
[tip:locking/core] locking/lockdep: Rename CONFIG_LOCKDEP_COMPLETE to CONFIG_LOCKDEP_COMPLETIONS tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
[tip:locking/core] locking/lockdep: Make CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
Re: [tip:locking/core] locking/lockdep: Make CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING Ingo Molnar <mingo@kernel.org> - 2017-08-17 12:50 +0200
Re: [tip:locking/core] locking/lockdep: Make CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-18 07:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-21 17:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 07:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 10:00 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:00 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:30 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 12:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 16:00 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 16:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 17:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Oleg Nesterov <oleg@redhat.com> - 2017-08-22 18:00 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 18:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Oleg Nesterov <oleg@redhat.com> - 2017-08-23 18:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 19:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 08:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 09:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 10:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-25 03:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:30 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 07:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Dave Chinner <david@fromorbit.com> - 2017-08-22 07:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:30 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:30 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 04:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 09:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Dave Chinner <david@fromorbit.com> - 2017-08-22 23:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:40 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:20 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:50 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 07:10 +0200
Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:00 +0200
csiph-web