[PATCH] workqueue: relax lockdep annotation on flush_work()

[PATCH] workqueue: relax lockdep annotation on flush_work()

[Tejun Heo]

Currently, the lockdep annotation in flush_work() requires exclusive
access on the workqueue the target work is queued on and triggers
warning if a work is trying to flush another work on the same
workqueue; however, this is no longer true as workqueues can now
execute multiple works concurrently.

This patch adds lock_map_acquire_read() and make process_one_work()
hold read access to the workqueue while executing a work and
start_flush_work() check for write access if concurrnecy level is one
and read access if higher.

This better represents what's going on and removes spurious lockdep
warnings which are triggered by fake dependency chain created through
flush_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: "Rafael J. Wysocki" <rjw@sisk.pl>
---
How should this one be routed?  The lockdep part can be split, merged
back into workqueue tree and so on but that seems a bit too much.  If
it's okay, I'll route this through the workqueue tree.  Going through
the lockdep tree is fine too.

Thanks.

 include/linux/lockdep.h |    3 +++
 kernel/workqueue.c      |    8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 71c09b2..9f19430 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -522,12 +522,15 @@ static inline void print_irqtrace_events(struct task_struct *curr)
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # ifdef CONFIG_PROVE_LOCKING
 #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 2, NULL, _THIS_IP_)
+#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 2, NULL, _THIS_IP_)
 # else
 #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
+#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 1, NULL, _THIS_IP_)
 # endif
 # define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
 #else
 # define lock_map_acquire(l)			do { } while (0)
+# define lock_map_acquire_read(l)		do { } while (0)
 # define lock_map_release(l)			do { } while (0)
 #endif
 
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8ee6ec8..85f8f7b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1840,7 +1840,7 @@ __acquires(&gcwq->lock)
 	spin_unlock_irq(&gcwq->lock);
 
 	work_clear_pending(work);
-	lock_map_acquire(&cwq->wq->lockdep_map);
+	lock_map_acquire_read(&cwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	trace_workqueue_execute_start(work);
 	f(work);
@@ -2384,8 +2384,12 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
 	insert_wq_barrier(cwq, barr, work, worker);
 	spin_unlock_irq(&gcwq->lock);
 
-	lock_map_acquire(&cwq->wq->lockdep_map);
+	if (cwq->wq->saved_max_active > 1)
+		lock_map_acquire_read(&cwq->wq->lockdep_map);
+	else
+		lock_map_acquire(&cwq->wq->lockdep_map);
 	lock_map_release(&cwq->wq->lockdep_map);
+
 	return true;
 already_gone:
 	spin_unlock_irq(&gcwq->lock);

Re: [PATCH] workqueue: relax lockdep annotation on flush_work()

[Rafael J. Wysocki]

On Wednesday, December 29, 2010, Tejun Heo wrote:
> Currently, the lockdep annotation in flush_work() requires exclusive
> access on the workqueue the target work is queued on and triggers
> warning if a work is trying to flush another work on the same
> workqueue; however, this is no longer true as workqueues can now
> execute multiple works concurrently.
> 
> This patch adds lock_map_acquire_read() and make process_one_work()
> hold read access to the workqueue while executing a work and
> start_flush_work() check for write access if concurrnecy level is one
> and read access if higher.
> 
> This better represents what's going on and removes spurious lockdep
> warnings which are triggered by fake dependency chain created through
> flush_work().

The spurious lockdep warning I've been observing is not printed any more with
the patch applied.

Thanks,
Rafael


> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
> How should this one be routed?  The lockdep part can be split, merged
> back into workqueue tree and so on but that seems a bit too much.  If
> it's okay, I'll route this through the workqueue tree.  Going through
> the lockdep tree is fine too.
> 
> Thanks.
> 
>  include/linux/lockdep.h |    3 +++
>  kernel/workqueue.c      |    8 ++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 71c09b2..9f19430 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -522,12 +522,15 @@ static inline void print_irqtrace_events(struct task_struct *curr)
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  # ifdef CONFIG_PROVE_LOCKING
>  #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 2, NULL, _THIS_IP_)
> +#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 2, NULL, _THIS_IP_)
>  # else
>  #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
> +#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 1, NULL, _THIS_IP_)
>  # endif
>  # define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
>  #else
>  # define lock_map_acquire(l)			do { } while (0)
> +# define lock_map_acquire_read(l)		do { } while (0)
>  # define lock_map_release(l)			do { } while (0)
>  #endif
>  
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 8ee6ec8..85f8f7b 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1840,7 +1840,7 @@ __acquires(&gcwq->lock)
>  	spin_unlock_irq(&gcwq->lock);
>  
>  	work_clear_pending(work);
> -	lock_map_acquire(&cwq->wq->lockdep_map);
> +	lock_map_acquire_read(&cwq->wq->lockdep_map);
>  	lock_map_acquire(&lockdep_map);
>  	trace_workqueue_execute_start(work);
>  	f(work);
> @@ -2384,8 +2384,12 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
>  	insert_wq_barrier(cwq, barr, work, worker);
>  	spin_unlock_irq(&gcwq->lock);
>  
> -	lock_map_acquire(&cwq->wq->lockdep_map);
> +	if (cwq->wq->saved_max_active > 1)
> +		lock_map_acquire_read(&cwq->wq->lockdep_map);
> +	else
> +		lock_map_acquire(&cwq->wq->lockdep_map);
>  	lock_map_release(&cwq->wq->lockdep_map);
> +
>  	return true;
>  already_gone:
>  	spin_unlock_irq(&gcwq->lock);

Re: [PATCH] workqueue: relax lockdep annotation on flush_work()

[Peter Zijlstra]

On Wed, 2010-12-29 at 13:57 +0100, Tejun Heo wrote:
> Currently, the lockdep annotation in flush_work() requires exclusive
> access on the workqueue the target work is queued on and triggers
> warning if a work is trying to flush another work on the same
> workqueue; however, this is no longer true as workqueues can now
> execute multiple works concurrently.
> 
> This patch adds lock_map_acquire_read() and make process_one_work()
> hold read access to the workqueue while executing a work and
> start_flush_work() check for write access if concurrnecy level is one
> and read access if higher.
> 
> This better represents what's going on and removes spurious lockdep
> warnings which are triggered by fake dependency chain created through
> flush_work().

Is this still true in the low memory situation where we're running the
emergency thread? I can imagine the emergency thread trying to flush
itself isn't really a good thing.

[PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Tejun Heo]

Currently, the lockdep annotation in flush_work() requires exclusive
access on the workqueue the target work is queued on and triggers
warning if a work is trying to flush another work on the same
workqueue; however, this is no longer true as workqueues can now
execute multiple works concurrently.

This patch adds lock_map_acquire_read() and make process_one_work()
hold read access to the workqueue while executing a work and
start_flush_work() check for write access if concurrnecy level is one
or the workqueue has a rescuer (as only one execution resource - the
rescuer - is guaranteed to be available under memory pressure), and
read access if higher.

This better represents what's going on and removes spurious lockdep
warnings which are triggered by fake dependency chain created through
flush_work().

* Peter pointed out that flushing another work from a WQ_MEM_RECLAIM
  wq breaks forward progress guarantee under memory pressure.
  Condition check accordingly updated.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
---
Ah... good point.  How does this one look then?

Thanks.

 include/linux/lockdep.h |    3 +++
 kernel/workqueue.c      |   14 ++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 71c09b2..9f19430 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -522,12 +522,15 @@ static inline void print_irqtrace_events(struct task_struct *curr)
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # ifdef CONFIG_PROVE_LOCKING
 #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 2, NULL, _THIS_IP_)
+#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 2, NULL, _THIS_IP_)
 # else
 #  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
+#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 1, NULL, _THIS_IP_)
 # endif
 # define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
 #else
 # define lock_map_acquire(l)			do { } while (0)
+# define lock_map_acquire_read(l)		do { } while (0)
 # define lock_map_release(l)			do { } while (0)
 #endif
 
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8ee6ec8..930c239 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1840,7 +1840,7 @@ __acquires(&gcwq->lock)
 	spin_unlock_irq(&gcwq->lock);
 
 	work_clear_pending(work);
-	lock_map_acquire(&cwq->wq->lockdep_map);
+	lock_map_acquire_read(&cwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	trace_workqueue_execute_start(work);
 	f(work);
@@ -2384,8 +2384,18 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
 	insert_wq_barrier(cwq, barr, work, worker);
 	spin_unlock_irq(&gcwq->lock);
 
-	lock_map_acquire(&cwq->wq->lockdep_map);
+	/*
+	 * 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 (cwq->wq->saved_max_active == 1 || cwq->wq->flags & WQ_RESCUER)
+		lock_map_acquire(&cwq->wq->lockdep_map);
+	else
+		lock_map_acquire_read(&cwq->wq->lockdep_map);
 	lock_map_release(&cwq->wq->lockdep_map);
+
 	return true;
 already_gone:
 	spin_unlock_irq(&gcwq->lock);

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Peter Zijlstra]

On Mon, 2011-01-03 at 15:17 +0100, Tejun Heo wrote:

> @@ -2384,8 +2384,18 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
>  	insert_wq_barrier(cwq, barr, work, worker);
>  	spin_unlock_irq(&gcwq->lock);
>  
> -	lock_map_acquire(&cwq->wq->lockdep_map);
> +	/*
> +	 * 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 (cwq->wq->saved_max_active == 1 || cwq->wq->flags & WQ_RESCUER)
> +		lock_map_acquire(&cwq->wq->lockdep_map);
> +	else
> +		lock_map_acquire_read(&cwq->wq->lockdep_map);
>  	lock_map_release(&cwq->wq->lockdep_map);
> +
>  	return true;
>  already_gone:
>  	spin_unlock_irq(&gcwq->lock);

Ah, but this violates the rule that you must always use the most strict
constraints. Code doesn't know if it will run in a rescue thread or not,
hence it must assume it does.

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Tejun Heo]

Hello,

On Mon, Jan 03, 2011 at 03:54:50PM +0100, Peter Zijlstra wrote:
> On Mon, 2011-01-03 at 15:17 +0100, Tejun Heo wrote:
> 
> > @@ -2384,8 +2384,18 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
> >  	insert_wq_barrier(cwq, barr, work, worker);
> >  	spin_unlock_irq(&gcwq->lock);
> >  
> > -	lock_map_acquire(&cwq->wq->lockdep_map);
> > +	/*
> > +	 * 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 (cwq->wq->saved_max_active == 1 || cwq->wq->flags & WQ_RESCUER)
> > +		lock_map_acquire(&cwq->wq->lockdep_map);
> > +	else
> > +		lock_map_acquire_read(&cwq->wq->lockdep_map);
> >  	lock_map_release(&cwq->wq->lockdep_map);
> > +
> >  	return true;
> >  already_gone:
> >  	spin_unlock_irq(&gcwq->lock);
> 
> Ah, but this violates the rule that you must always use the most strict
> constraints. Code doesn't know if it will run in a rescue thread or not,
> hence it must assume it does.

Hmmm?  The code applies the most strict contraints.  If the workqueue
has a rescuer, flushing another work from the workqueue will always
trigger lockdep warning.  The rule is relaxed only for workqueues
which aren't used for memory reclaiming && support parallel execution.

Thanks.

-- 
tejun

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Peter Zijlstra]

On Mon, 2011-01-03 at 16:00 +0100, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jan 03, 2011 at 03:54:50PM +0100, Peter Zijlstra wrote:
> > On Mon, 2011-01-03 at 15:17 +0100, Tejun Heo wrote:
> > 
> > > @@ -2384,8 +2384,18 @@ static bool start_flush_work(struct work_struct *work, struct wq_barrier *barr,
> > >  	insert_wq_barrier(cwq, barr, work, worker);
> > >  	spin_unlock_irq(&gcwq->lock);
> > >  
> > > -	lock_map_acquire(&cwq->wq->lockdep_map);
> > > +	/*
> > > +	 * 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 (cwq->wq->saved_max_active == 1 || cwq->wq->flags & WQ_RESCUER)
> > > +		lock_map_acquire(&cwq->wq->lockdep_map);
> > > +	else
> > > +		lock_map_acquire_read(&cwq->wq->lockdep_map);
> > >  	lock_map_release(&cwq->wq->lockdep_map);
> > > +
> > >  	return true;
> > >  already_gone:
> > >  	spin_unlock_irq(&gcwq->lock);
> > 
> > Ah, but this violates the rule that you must always use the most strict
> > constraints. Code doesn't know if it will run in a rescue thread or not,
> > hence it must assume it does.
> 
> Hmmm?  The code applies the most strict contraints.  If the workqueue
> has a rescuer, flushing another work from the workqueue will always
> trigger lockdep warning.  The rule is relaxed only for workqueues
> which aren't used for memory reclaiming && support parallel execution.

Ah, ok. I read it like: if the current thread is a rescue thread.

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Tejun Heo]

On Mon, Jan 03, 2011 at 04:14:45PM +0100, Peter Zijlstra wrote:
> > Hmmm?  The code applies the most strict contraints.  If the workqueue
> > has a rescuer, flushing another work from the workqueue will always
> > trigger lockdep warning.  The rule is relaxed only for workqueues
> > which aren't used for memory reclaiming && support parallel execution.
> 
> Ah, ok. I read it like: if the current thread is a rescue thread.

Ah, I see.  Rafael, can you please verify the updated version is okay
too (it should be but just in case)?  Peter, shall I route this
through workqueue tree with your ack?

Thank you.

-- 
tejun

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Peter Zijlstra]

On Mon, 2011-01-03 at 16:20 +0100, Tejun Heo wrote:
> On Mon, Jan 03, 2011 at 04:14:45PM +0100, Peter Zijlstra wrote:
> > > Hmmm?  The code applies the most strict contraints.  If the workqueue
> > > has a rescuer, flushing another work from the workqueue will always
> > > trigger lockdep warning.  The rule is relaxed only for workqueues
> > > which aren't used for memory reclaiming && support parallel execution.
> > 
> > Ah, ok. I read it like: if the current thread is a rescue thread.
> 
> Ah, I see.  Rafael, can you please verify the updated version is okay
> too (it should be but just in case)?  Peter, shall I route this
> through workqueue tree with your ack?

That's fine with me, thanks!

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Rafael J. Wysocki]

On Monday, January 03, 2011, Tejun Heo wrote:
> On Mon, Jan 03, 2011 at 04:14:45PM +0100, Peter Zijlstra wrote:
> > > Hmmm?  The code applies the most strict contraints.  If the workqueue
> > > has a rescuer, flushing another work from the workqueue will always
> > > trigger lockdep warning.  The rule is relaxed only for workqueues
> > > which aren't used for memory reclaiming && support parallel execution.
> > 
> > Ah, ok. I read it like: if the current thread is a rescue thread.
> 
> Ah, I see.  Rafael, can you please verify the updated version is okay
> too (it should be but just in case)?

It still appears to be fine.

Thanks,
Rafael

Re: [PATCH UPDATED] workqueue: relax lockdep annotation on flush_work()

[Tejun Heo]

On Mon, Jan 03, 2011 at 10:25:46PM +0100, Rafael J. Wysocki wrote:
> On Monday, January 03, 2011, Tejun Heo wrote:
> > On Mon, Jan 03, 2011 at 04:14:45PM +0100, Peter Zijlstra wrote:
> > > > Hmmm?  The code applies the most strict contraints.  If the workqueue
> > > > has a rescuer, flushing another work from the workqueue will always
> > > > trigger lockdep warning.  The rule is relaxed only for workqueues
> > > > which aren't used for memory reclaiming && support parallel execution.
> > > 
> > > Ah, ok. I read it like: if the current thread is a rescue thread.
> > 
> > Ah, I see.  Rafael, can you please verify the updated version is okay
> > too (it should be but just in case)?
> 
> It still appears to be fine.

Applied to wq#fixes-2.6.38 with Tested-by and stable cc added.

Thank you.

-- 
tejun