Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature

[Peter Zijlstra]

On Wed, Mar 01, 2017 at 04:21:28PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > +	/*
> > > +	 * Each work of workqueue might run in a different context,
> > > +	 * thanks to concurrency support of workqueue. So we have to
> > > +	 * distinguish each work to avoid false positive.
> > > +	 *
> > > +	 * TODO: We can also add dependencies between two acquisitions
> > > +	 * of different work_id, if they don't cause a sleep so make
> > > +	 * the worker stalled.
> > > +	 */
> > > +	unsigned int		work_id;
> > 
> > > +/*
> > > + * Crossrelease needs to distinguish each work of workqueues.
> > > + * Caller is supposed to be a worker.
> > > + */
> > > +void crossrelease_work_start(void)
> > > +{
> > > +	if (current->xhlocks)
> > > +		current->work_id++;
> > > +}
> > 
> > So what you're trying to do with that 'work_id' thing is basically wipe
> > the entire history when we're at the bottom of a context.
> 
> Sorry, but I do not understand what you are trying to say.
> 
> What I was trying to do with the 'work_id' is to distinguish between
> different works, which will be used to check if history locks were held
> in the same context as a release one.

The effect of changing work_id is that history disappears, yes? That is,
by changing it, all our hist_locks don't match the context anymore and
therefore we have no history.

This is a useful operation.

You would want to do this at points where you know there will not be any
dependencies on prior action, and typically at the same points we want
to not be holding any locks.

Hence my term: 'bottom of a context', referring to an empty (held) lock
stack.

I would say this needs to be done for all 'work-queue' like things, and
there are quite a few outside of the obvious ones, smpboot threads and
many other kthreads fall into this category.

Similarly the return to userspace point that I already mentioned.

I would propose something like:

	lockdep_assert_empty();

Or something similar, which would verify the lock stack is indeed empty
and wipe our entire hist_lock buffer when cross-release is enabled.

> > Which is a useful operation, but should arguably also be done on the
> > return to userspace path. Any historical lock from before the current
> > syscall is irrelevant.
> 
> Sorry. Could you explain it more?

Does the above make things clear?