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?