[2/3] kernel/workqueue: Surround work execution with shared lock annotations
diff mbox series

Message ID 20181025150540.259281-3-bvanassche@acm.org
State New, archived
Headers show
Series
  • Suppress false positives triggered by workqueue lockdep annotations
Related show

Commit Message

Bart Van Assche Oct. 25, 2018, 3:05 p.m. UTC
Surround execution of work with a shared lockdep annotation because multiple
work items associated with a work queue may execute concurrently.

Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 kernel/workqueue.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Johannes Berg Oct. 25, 2018, 4:53 p.m. UTC | #1
On Thu, 2018-10-25 at 15:05 +0000, Bart Van Assche wrote:
> Surround execution of work with a shared lockdep annotation because multiple
> work items associated with a work queue may execute concurrently.

Hmm. So, I'm not really entirely sure of the semantics here, but I fail
to see how "may execute concurrently" means "can be taken recursively"?

After all, if they execute concurrently, that's in a different thread,
right? So each thread is really just doing something with this work. It
may not match mutex semantics in how mutexes would lock each other out
and prevent concurrency, but I don't think that matters to lockdep at
all.

In fact, I'm not sure this actually changes anything, since you can't
really execute a work struct while executing one already?

What's this intended to change? I currently don't see how lockdep's
behaviour would differ with read==1, unless you actually tried to do
recursive locking, which isn't really possible?

Or perhaps this is actually the right change for the issue described in
patch 1, where a work struct flushes another work on the same wq, and
that causes recursion of sorts? But that recursion should only happen if
the workqueues is actually marked as ordered, in which case it *is* in
fact wrong?

johannes
Bart Van Assche Oct. 25, 2018, 5:22 p.m. UTC | #2
On Thu, 2018-10-25 at 18:53 +0200, Johannes Berg wrote:
> On Thu, 2018-10-25 at 15:05 +0000, Bart Van Assche wrote:
> > Surround execution of work with a shared lockdep annotation because multiple
> > work items associated with a work queue may execute concurrently.
> 
> Hmm. So, I'm not really entirely sure of the semantics here, but I fail
> to see how "may execute concurrently" means "can be taken recursively"?
> 
> After all, if they execute concurrently, that's in a different thread,
> right? So each thread is really just doing something with this work. It
> may not match mutex semantics in how mutexes would lock each other out
> and prevent concurrency, but I don't think that matters to lockdep at
> all.
> 
> In fact, I'm not sure this actually changes anything, since you can't
> really execute a work struct while executing one already?
> 
> What's this intended to change? I currently don't see how lockdep's
> behaviour would differ with read==1, unless you actually tried to do
> recursive locking, which isn't really possible?
> 
> Or perhaps this is actually the right change for the issue described in
> patch 1, where a work struct flushes another work on the same wq, and
> that causes recursion of sorts? But that recursion should only happen if
> the workqueues is actually marked as ordered, in which case it *is* in
> fact wrong?

How about modifying the wq->lockdep_map annotations only and not touching the
work->lockdep_map annotations? My comment about concurrency in the patch
description refers to a multithreaded workqueue executing multiple different
work items concurrently. I am aware that great care has been taken in the
workqueue implementation to ensure that each work item is executed by exactly
one worker.

Bart.
Johannes Berg Oct. 25, 2018, 7:17 p.m. UTC | #3
On Thu, 2018-10-25 at 10:22 -0700, Bart Van Assche wrote:

> > What's this intended to change? I currently don't see how lockdep's
> > behaviour would differ with read==1, unless you actually tried to do
> > recursive locking, which isn't really possible?
> > 
> > Or perhaps this is actually the right change for the issue described in
> > patch 1, where a work struct flushes another work on the same wq, and
> > that causes recursion of sorts? But that recursion should only happen if
> > the workqueues is actually marked as ordered, in which case it *is* in
> > fact wrong?
> 
> How about modifying the wq->lockdep_map annotations only and not touching the
> work->lockdep_map annotations? My comment about concurrency in the patch
> description refers to a multithreaded workqueue executing multiple different
> work items concurrently. I am aware that great care has been taken in the
> workqueue implementation to ensure that each work item is executed by exactly
> one worker.

Again, I'm not sure what you'd want to achieve by that?

If you look at lockdep, read==1 really doesn't do all that much. Note
read is the 4th arg to lock_acquire() via lock_acquire_shared().

Hold time statistics are accounted differently, but that's meaningless
here.

check_deadlock() will behave differently - if read==2 (recursive read)
it will not complain about recursing into a previous read==1 acquire.
Not relevant here, because you can't recurse into the workqueue while
you're running on it.

Some in-IRQ things are different, but we can't be in-IRQ here.


Note that lockdep doesn't care about multiple threads "holding" the same
lock. Everything it does in terms of this tracking is per-thread, afaict
(uses "current" everywhere, and that's how the reports look like etc.).
So ultimately the only places where this read==1 would matter cannot be
hit (recursion within the same thread). Importantly, it doesn't try to
track across the whole system if you're holding the same lock twice.
That's actually impossible with most locks anyway (one would wait), but
when it does happen like it can here, it doesn't really do anything bad.


I don't think it actually *breaks* anything to make this a read/write
lock, because even with a read/write lock you still get a deadlock if
you do

read(A)
		lock(B)
		write(A)
lock(B)

or

write(A)
		lock(B)
		read(A)
lock(B)


since read/write are mutually exclusive, and you're not changing the
flush-side to also be a readlock. If you were, I'd argue that'd be wrong
since then you *don't* detect such deadlocks:

read(A)
		lock(B)
		read(A)
lock(B)

would not result in a complaint (I think), but in the workqueue case it
*should* because you can't flush the workqueue while holding a lock that
you also hold in a work that's executing on it.


So in the end, I just simply don't see a point in this, and I don't
think you've sufficiently explained why this change should be made.

johannes

Patch
diff mbox series

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 6755ef21a679..fc9129d5909e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2125,8 +2125,8 @@  __acquires(&pool->lock)
 
 	spin_unlock_irq(&pool->lock);
 
-	lock_map_acquire(&pwq->wq->lockdep_map);
-	lock_map_acquire(&lockdep_map);
+	lock_acquire_shared(&pwq->wq->lockdep_map, 0, 0, NULL, _THIS_IP_);
+	lock_acquire_shared(&lockdep_map, 0, 0, NULL, _THIS_IP_);
 	/*
 	 * Strictly speaking we should mark the invariant state without holding
 	 * any locks, that is, before these two lock_map_acquire()'s.
@@ -2156,8 +2156,8 @@  __acquires(&pool->lock)
 	 * point will only record its address.
 	 */
 	trace_workqueue_execute_end(work);
-	lock_map_release(&lockdep_map);
-	lock_map_release(&pwq->wq->lockdep_map);
+	lock_release(&lockdep_map, 0, _THIS_IP_);
+	lock_release(&pwq->wq->lockdep_map, 0, _THIS_IP_);
 
 	if (unlikely(in_atomic() || lockdep_depth(current) > 0)) {
 		pr_err("BUG: workqueue leaked lock or atomic: %s/0x%08x/%d\n"
@@ -2652,8 +2652,8 @@  void flush_workqueue(struct workqueue_struct *wq)
 	if (WARN_ON(!wq_online))
 		return;
 
-	lock_map_acquire(&wq->lockdep_map);
-	lock_map_release(&wq->lockdep_map);
+	lock_acquire_exclusive(&wq->lockdep_map, 0, 0, NULL, _THIS_IP_);
+	lock_release(&wq->lockdep_map, 0, _THIS_IP_);
 
 	mutex_lock(&wq->mutex);
 
@@ -2891,8 +2891,9 @@  static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
 	 */
 	if (!from_cancel &&
 	    (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)) {
-		lock_map_acquire(&pwq->wq->lockdep_map);
-		lock_map_release(&pwq->wq->lockdep_map);
+		lock_acquire_exclusive(&pwq->wq->lockdep_map, 0, 0, NULL,
+				       _THIS_IP_);
+		lock_release(&pwq->wq->lockdep_map, 0, _THIS_IP_);
 	}
 
 	return true;