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

[Byungchul Park <byungchul.park@lge.com>]

On Wed, Aug 23, 2017 at 12:20:48PM +0200, Peter Zijlstra wrote:
> 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.

No, they can be relevant, 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.

No, wrong, it can be either one.

> > 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@twins.programming.kicks-ass.net
> > 
> > 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.

The purpose of the PROC thing is to annotate independent execution of
work _itself_.

'o' in my examplel can be relevant and should be relevant with each
execution, that is, 'i' in my example.

> 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.

No, as I said, we don't have to start(PROC) for normal kernel contexts
before taking any locks. They should be able to do it w/o start(PROC).
That is necessary only for spectial contexts such as work.

> Similar with history, independence means to not depend on prior state,

Independence means to not depend on each other context e.i. work. IOW,
we don't have to make dependencies between 'o's and 'i's indenpendent.
The reason is simple. It's becasue the dependencies can exist.

> 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);
>

No matter whether the lock_map_acquire()s is 'o' or 'i', it works
depending on what we want. If we want to make each lock_map_acquire()
here independent, then we should choose your way. But if we should not,
then we should keep my way unchaged.

> This way _all_ works are guaranteed the same context, and not only those

I don't understand what you intended here. This way all works become
independent each other.

> 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)

I got it. I want to work and make the read-recursive thing in lockdep
work soon.

> 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 think you get confused now about start(PROC)/end(PROC). Or am I
confused wrt what you intend?