Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

[Peter Zijlstra]

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");
 
 	/*