lkml.org 
[lkml]   [2017]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING
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");

/*
\
 
 \ /
  Last update: 2017-08-22 15:53    [W:0.508 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site