lkml.org 
[lkml]   [2010]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] workqueue: relax lockdep annotation on flush_work()
Date
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);


\
 
 \ /
  Last update: 2010-12-29 15:23    [from the cache]
©2003-2011 Jasper Spaans