Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

[Peter Zijlstra]

On Tue, Aug 29, 2017 at 05:52:05PM +0200, Oleg Nesterov wrote:

> The problem is that start_flush_work() does not do acquire/release
> unconditionally, it does this only if it is going to wait, and I am not
> sure this is right...

Right, I think you're right, we can move it earlier, once we have the
pwq.

> Plus process_one_work() does lock_map_acquire_read(), I don't really
> understand this too.

Right, so aside from recursive-reads being broken, I think that was a
mistake.

> > 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:
> 
> Yes, but the simple answer is that flush_work() doesn't really differ
> from flush_workqueue() in this respect?

Right, and I think that the new code (aside from maybe placing it
earlier) does that. If single-threaded we use wq->lockdep_map, otherwise
we (also) use work->lockdep_map.

> If nothing else, if some WORK is the last queued work on WQ, then
> flush_work(WORK) is the same thing as flush_workqueuw(WQ), more or less.
> Again, I am talking about single-threaded workqueues.

Right, so single-threaded workqueues are special and are what we need
this extra bit for, but only for single-threaded.

> > workqueue-thread:
> >
> > 	work-n:
> > 	  flush_work(work-n+1);
> >
> > 	work-n+1:
> >
> >
> > Will not be going anywhere fast..
> 
> Or another example,
> 
> 	lock(LOCK);
> 	flush_work(WORK);
> 	unlock(LOCK);
> 
> 	workqueue-thread:
> 		another_pending_work:
> 			LOCK(LOCK);
> 			UNLOCK(LOCK);
> 
> 		WORK:
> 
> In this case we do not care about WORK->lockdep_map, but
> taking the wq->lockdep_map from flush_work() (if single-threaded) allows
> to report the deadlock.

Right. And the new code does so.