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

[Peter Zijlstra]

On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote:

> > > We have to detect dependecies if it exists, even in the following case:
> > > 
> > > oooooooiiiiiiiiiiiiiiiiiii.........
> > >   |<- range for commit ->|
> > > 
> > >   where
> > >   o: acquisition outside of each work,
> > >   i: acquisition inside of each work,
> > > 
> > > With yours, we can never detect dependecies wrt 'o'.
> > 
> > There really shouldn't be any o's when you call
> 
> There can be any o's.

Yes, but they _must_ be irrelevant, see below.

> > crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a
> 
> No, I don't think so. It can be either the bottom or not.

Nope, wrong, _must_ be bottom.

> hist_start() and hist_end() is only for special contexts which need roll
> back on exit e.g. irq, work and so on. Normal kernel context should work
> well w/o hist_start() or hist_end().

The (soft) IRQ ones, yes, the PROC one is special though.

> > context, see:
> > 
> >   https://lkml.kernel.org/r/20170301104328.GD6515@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> Actually, I don't agree with that.
> 
> > And in that respect you placed the calls wrongly in process_one_work(),
> 
> Why is it wrong? It's intended. Could you tell me why?

The purpose of the PROC thing is to annotate _independent_ execution,
like work's.

Either they should _all_ depend on a held lock, or they should not
depend on it at all. Either way this means we should start(PROC) before
taking any locks.

Similar with history, independence means to not depend on prior state,
so per definition we should not have history at this point.

So by this reasoning the workqueue annotation should be:


	crossrelease_hist_start(XHLOCK_PROC);

	lock_map_acquire(wq->lockdep_map);
	lock_map_acquire(lockdep_map);

	work->func(work); /* go hard */

	lock_map_release(lockdep_map);
	lock_map_release(wq->lockdep_map);

	crossrelease_hist_end(XHLOCK_PROC);


This way _all_ works are guaranteed the same context, and not only those
that didn't overflow the history stack.

Now, it so happens we have an unfortunate interaction with the
flush_work{,queue}() annotations. The read-recursive thing would work if
lockdep were fixed, however I don't think there is a possible deadlock
between flush_work() and complete() (except for single-threaded
workqueues)

So until we fix lockdep for read-recursive things, I don't think its a
problem if we (ab)use your wrong placement to kill the interaction
between these two annotations.

I'll send some patches..