On Wed, Aug 23, 2017 at 06:39:03PM +0200, Oleg Nesterov wrote:
> Peter, I am all confused and I am still trying to understand your email.
> In particular, because I no longer understand the lockdep annotations in
> workqueue.c, it turns out I forgot everything...
Yeah, that happens :/
> On 08/22, Peter Zijlstra wrote:
> >
> > I am however slightly puzzled by the need of flush_work() to take Q,
> > what deadlock potential is there?
>
> Do you really mean flush_work()? Or start_flush_work() ?
Same thing, start_flush_work() has exactly one caller: flush_work().
> > It was added by Oleg in commit:
> >
> > a67da70dc095 ("workqueues: lockdep annotations for flush_work()")
>
> No, these annotations were moved later into start_flush, iiuc...
>
> This
>
> lock_map_acquire(&work->lockdep_map);
> lock_map_release(&work->lockdep_map);
>
> was added by another commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
> "workqueue: Catch more locking problems with flush_work()", and at
> first glance it is fine.
Those are fine and are indeed the flush_work() vs work inversion.
The two straight forward annotations are:
flush_work(work) process_one_work(wq, work)
A(work) A(work)
R(work) work->func(work);
R(work)
Which catches:
Task-1: work:
mutex_lock(&A); mutex_lock(&A);
flush_work(work);
And the analogous:
flush_workqueue(wq) process_one_work(wq, work)
A(wq) A(wq)
R(wq) work->func(work);
R(wq)
The thing I puzzled over was flush_work() (really start_flush_work())
doing:
if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
lock_map_acquire(&pwq->wq->lockdep_map);
else
lock_map_acquire_read(&pwq->wq->lockdep_map);
lock_map_release(&pwq->wq->lockdep_map);
Why does flush_work() care about the wq->lockdep_map?
The answer is because, for single-threaded workqueues, doing
flush_work() from a work is a potential deadlock:
workqueue-thread:
work-n:
flush_work(work-n+1);
work-n+1:
Will not be going anywhere fast..
And by taking the wq->lockdep_map from flush_work(), but _only_ when
single-threaded (or rescuer, see other emails), and by doing that, it
forces a recursive lock deadlock message like:
process_one_work(wq, work)
A(wq)
A(work)
work->func(work)
flush_work(work2)
A(work2)
R(work2)
A(wq) <-- recursive lock deadlock
Make sense?
