Re: [PATCH v4 15/15] lockdep: Crossrelease feature documentation

Byungchul Park
2017-01-11T01:33:57Z

On Tue, Jan 10, 2017 at 09:08:50PM +0100, Peter Zijlstra wrote:
> But I still feel this document is very hard to read and presents things
> backwards.

I admit it. I think I need to modify the document more.. I will try it.

>
> > +Let's take a look at more complicated example.
> > +
> > + TASK X TASK Y
> > + ------ ------
> > + acquire B
> > +
> > + release B
> > +
> > + acquire C
> > +
> > + release C
> > + (1)
> > + fork Y
> > + acquire AX
> > + acquire D
> > + /* A dependency 'AX -> D' exists */
> > + acquire F
> > + release D
> > + acquire G
> > + /* A dependency 'F -> G' exists */
> > + acquire E
> > + /* A dependency 'AX -> E' exists */
> > + acquire H
> > + /* A dependency 'G -> H' exists */
> > + release E
> > + release H
> > + release AX held by Y
> > + release G
> > +
> > + release F
> > +
> > + where AX, B, C,..., H are different lock classes, and a suffix 'X' is
> > + added on crosslocks.
> > +
> > +Does a dependency 'AX -> B' exist? Nope.
>
> I think the above without the "fork Y" line is a much more interesting
> example, because then the answer becomes: maybe.

Sure. The dependency 'AX -> B' might exist in that case. Then we can
add the dependency once we detect it, in other words, once we prove it's
a true dependency. But we cannot add it before we prove it, though it
might be a true one, because it might not be a true one.

> This all boils down to the asynchonous nature of the primitive. There is
> no well defined point other than what is observed (as I think you tried
> to point out in our earlier exchanges).

Exactly.

> The "acquire AX" point is entirely random wrt any action in other
> threads, _however_ the time between "acquire" and "release" of any
> 'lock' is the only time we can be certain of things.
>
> > +==============
> > +Implementation
> > +==============
> > +
> > +Data structures
> > +---------------
> > +
> > +Crossrelease feature introduces two main data structures.
> > +
> > +1. pend_lock
>
> I'm not sure 'pending' is the right name here, but I'll consider that
> more when I review the code patches.

Thank you.

> > +
> > + This is an array embedded in task_struct, for keeping locks queued so
> > + that real dependencies can be added using them at commit step. Since
> > + it's local data, it can be accessed locklessly in the owner context.
> > + The array is filled at acquire step and consumed at commit step. And
> > + it's managed in circular manner.
> > +
> > +2. cross_lock
> > +
> > + This is a global linked list, for keeping all crosslocks in progress.
> > + The list grows at acquire step and is shrunk at release step.
>
> FWIW, this is a perfect example of why I say the document is written
> backwards. At this point there is no demonstrated need or use for this
> list.

I will consider that more.

> OK, so commit adds multiple dependencies, that makes more sense.
> Previously I understood commit to only add a single dependency, which
> does not make sense (except in the special case where there is but one).
>
> I dislike how I have to reconstruct this from an example instead of
> first having had the rules stated though.

So do I.

>
> > + *
> > + * In pend_lock: D, E
> > + * In graph: 'B -> C', 'C -> D',
> > + * 'AX -> D', 'AX -> E'
> > + */