[1/3] kernel/workqueue: Remove lockdep annotation from __flush_work()
diff mbox series

Message ID 20181025150540.259281-2-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
As documented in a comment in start_flush_work(), calling flush_work()
from a multi-threaded workqueue is safe if that workqueue is not
equipped with a rescuer. Avoid that flush_work() calls from inside a
work item executing on such a queue trigger a lockdep complaint. Remove
the lockdep annotation from __flush_work() because start_flush_work()
already has such an annotation.

Fixes: 87915adc3f0a ("workqueue: re-add lockdep dependencies for flushing")
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 kernel/workqueue.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Johannes Berg Oct. 25, 2018, 3:31 p.m. UTC | #1
On Thu, 2018-10-25 at 15:05 +0000, Bart Van Assche wrote:
> As documented in a comment in start_flush_work(), calling flush_work()
> from a multi-threaded workqueue is safe if that workqueue is not
> equipped with a rescuer. Avoid that flush_work() calls from inside a
> work item executing on such a queue trigger a lockdep complaint.

So ... not sure I understand, do you happen to have an example (at least
conceptually) that shows the problem?

Something like

workqueue WQ, works W1, W2

W1 running on WQ -> flush_work(W2) also running on WQ?

I'm willing to believe that this is a corner case I missed with the
annotations since the rescuer things are tricky, but I don't think
removing them is the right thing to do.

> Remove
> the lockdep annotation from __flush_work() because start_flush_work()
> already has such an annotation.

This part at least isn't true, there's no annotation on the work
*struct*, only one on the work *queue*.

johannes
Johannes Berg Oct. 25, 2018, 3:57 p.m. UTC | #2
On Thu, 2018-10-25 at 17:31 +0200, Johannes Berg wrote:
> On Thu, 2018-10-25 at 15:05 +0000, Bart Van Assche wrote:
> > As documented in a comment in start_flush_work(), calling flush_work()
> > from a multi-threaded workqueue is safe if that workqueue is not
> > equipped with a rescuer. Avoid that flush_work() calls from inside a
> > work item executing on such a queue trigger a lockdep complaint.

So actually, come to think of it, certainly this will cause a false
negative in this case?

mutex_lock(A);
flush_work(W);

worker_W_function()
{
	mutex_lock(A);
}

right?

johannes
Bart Van Assche Oct. 25, 2018, 4:01 p.m. UTC | #3
On Thu, 2018-10-25 at 17:31 +0200, Johannes Berg wrote:
> > Remove
> > the lockdep annotation from __flush_work() because start_flush_work()
> > already has such an annotation.
> 
> This part at least isn't true, there's no annotation on the work
> *struct*, only one on the work *queue*.

You are right - I will drop this patch.

Bart.

Patch
diff mbox series

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0280deac392e..6755ef21a679 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2908,11 +2908,6 @@  static bool __flush_work(struct work_struct *work, bool from_cancel)
 	if (WARN_ON(!wq_online))
 		return false;
 
-	if (!from_cancel) {
-		lock_map_acquire(&work->lockdep_map);
-		lock_map_release(&work->lockdep_map);
-	}
-
 	if (start_flush_work(work, &barr, from_cancel)) {
 		wait_for_completion(&barr.done);
 		destroy_work_on_stack(&barr.work);