On Tue, Aug 22, 2017 at 04:46:02PM +0200, Peter Zijlstra wrote:
> I am however slightly puzzled by the need of flush_work() to take Q,
> what deadlock potential is there?
>
> Task: Work-W1: Work-W2:
>
> M(A) AR(Q) AR(Q)
> flush_work(W1) A(W1) A(W2)
> A(W1) M(A)
> R(W1)
> AR(Q)
> R(Q)
>
> Spells deadlock on AQ-QA, but why? Why is flush_work() linked to any lock
> taken inside random other works. If we can get rid of flush_work()'s
> usage of Q, we can drop the recursive nature.
>
> It was added by Oleg in commit:
>
> a67da70dc095 ("workqueues: lockdep annotations for flush_work()")
>
> Which has a distinct lack of Changelog. However, that is still very much
> the old workqueue code, where I think the annotation makes sense because
> that was a single thread running the works consecutively. But I don't
> see it making sense for the current workqueue that runs works
> concurrently.
>
> TJ, Oleg, can we agree flush_work() no longer needs the dependency on Q?
So we still need it in case of max_active==1 and that rescuer thing, and
then we still need to support calling it from a work, which then
recurses. How about the below, its a bit icky, but should work (boots
and builds a kernel).
---
kernel/workqueue.c | 41 ++++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index e86733a8b344..c37b761f60b1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2091,7 +2091,7 @@ __acquires(&pool->lock)
spin_unlock_irq(&pool->lock);
- lock_map_acquire_read(&pwq->wq->lockdep_map);
+ lock_map_acquire(&pwq->wq->lockdep_map);
lock_map_acquire(&lockdep_map);
crossrelease_hist_start(XHLOCK_PROC);
trace_workqueue_execute_start(work);
@@ -2783,6 +2783,32 @@ void drain_workqueue(struct workqueue_struct *wq)
}
EXPORT_SYMBOL_GPL(drain_workqueue);
+static bool need_wq_lock(struct pool_workqueue *pwq)
+{
+ struct worker *worker = current_wq_worker();
+
+ /*
+ * If current is running a work of the same pwq, we already hold
+ * pwq->wq->lockdep_map, no need to take it again.
+ */
+ if (worker && worker->current_pwq == pwq)
+ return false;
+
+ /*
+ * If @max_active is 1 or rescuer is in use, flushing another work
+ * item on the same workqueue may lead to deadlock. Make sure the
+ * flusher is not running on the same workqueue by verifying write
+ * access.
+ */
+ if (pwq->wq->saved_max_active == 1)
+ return true;
+
+ if (pwq->wq->rescuer)
+ return true;
+
+ return false;
+}
+
static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr)
{
struct worker *worker = NULL;
@@ -2816,17 +2842,10 @@ static bool start_flush_work(struct work_struct *work,
struct wq_barrier *barr)
insert_wq_barrier(pwq, barr, work, worker);
spin_unlock_irq(&pool->lock);
- /*
- * If @max_active is 1 or rescuer is in use, flushing another work
- * item on the same workqueue may lead to deadlock. Make sure the
- * flusher is not running on the same workqueue by verifying write
- * access.
- */
- if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
+ if (need_wq_lock(pwq)) {
lock_map_acquire(&pwq->wq->lockdep_map);
- else
- lock_map_acquire_read(&pwq->wq->lockdep_map);
- lock_map_release(&pwq->wq->lockdep_map);
+ lock_map_release(&pwq->wq->lockdep_map);
+ }
return true;
already_gone:
