Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1589730 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-02-28 20:50 +0100
    Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-01 08:30 +0100
      Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-03-01 11:50 +0100
        Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-01 13:30 +0100
    Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Matthew Wilcox <willy@infradead.org> - 2017-03-02 05:50 +0100
      RE: [PATCH v5 06/13] lockdep: Implement crossrelease feature "byungchul.park" <byungchul.park@lge.com> - 2017-03-02 07:20 +0100
        Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Matthew Wilcox <willy@infradead.org> - 2017-03-02 15:50 +0100
          Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-03 01:00 +0100
            Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-05 09:10 +0100

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

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.

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.

(And we should not be returning to userspace with locks held anyway --
lockdep already has a check for that).

[toc] | [next] | [standalone]

#1590059

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.

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

> 
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).

Yes right. We should not be returning to userspace without reporting it
in that case.

[toc] | [prev] | [next] | [standalone]

#1590227

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?

[toc] | [prev] | [next] | [standalone]

#1590297

On Wed, Mar 01, 2017 at 11:43:28AM +0100, Peter Zijlstra wrote:
> 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.

Right. Now I understood your words.

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

Right.

> I would say this needs to be done for all 'work-queue' like things, and

Of course.

> there are quite a few outside of the obvious ones, smpboot threads and
> many other kthreads fall into this category.

Where can I check those?

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

Right. I should do that.

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

Let me think more. It looks not a simple problem.

> > 
> > Sorry. Could you explain it more?
> 
> Does the above make things clear?

Perfect. Thank you very much.

[toc] | [prev] | [next] | [standalone]

#1590841

On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).

Don't we return to userspace with page locks held, eg during async directio?

[toc] | [prev] | [next] | [standalone]

#1590857

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy@infradead.org]
> Sent: Thursday, March 02, 2017 1:20 PM
> To: Peter Zijlstra
> Cc: Byungchul Park; mingo@kernel.org; tglx@linutronix.de;
> walken@google.com; boqun.feng@gmail.com; kirill@shutemov.name; linux-
> kernel@vger.kernel.org; linux-mm@kvack.org; iamjoonsoo.kim@lge.com;
> akpm@linux-foundation.org; npiggin@gmail.com
> Subject: Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
> 
> On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > (And we should not be returning to userspace with locks held anyway --
> > lockdep already has a check for that).
> 
> Don't we return to userspace with page locks held, eg during async
> directio?

Hello,

I think that the check when returning to user with crosslocks held
should be an exception. Don't you think so?

Thanks,
Byungchul

[toc] | [prev] | [next] | [standalone]

#1591141

On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote:
> From: Matthew Wilcox [mailto:willy@infradead.org]
> > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > > (And we should not be returning to userspace with locks held anyway --
> > > lockdep already has a check for that).
> > 
> > Don't we return to userspace with page locks held, eg during async
> > directio?
> 
> Hello,
> 
> I think that the check when returning to user with crosslocks held
> should be an exception. Don't you think so?

Oh yes.  We have to keep the pages locked during reads, and we have to
return to userspace before I/O is complete, therefore we have to return
to userspace with pages locked.  They'll be unlocked by the interrupt
handler in page_endio().

Speaking of which ... this feature is far too heavy for use in production
on pages.  You're almost trebling the size of struct page.  Can we
do something like make all struct pages share the same lockdep_map?
We'd have to not complain about holding one crossdep lock and acquiring
another one of the same type, but with millions of pages in the system,
it must surely be creating a gargantuan graph right now?

[toc] | [prev] | [next] | [standalone]

#1591563

On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote:
> On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote:
> > From: Matthew Wilcox [mailto:willy@infradead.org]
> > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > > > (And we should not be returning to userspace with locks held anyway --
> > > > lockdep already has a check for that).
> > > 
> > > Don't we return to userspace with page locks held, eg during async
> > > directio?
> > 
> > Hello,
> > 
> > I think that the check when returning to user with crosslocks held
> > should be an exception. Don't you think so?
> 
> Oh yes.  We have to keep the pages locked during reads, and we have to
> return to userspace before I/O is complete, therefore we have to return
> to userspace with pages locked.  They'll be unlocked by the interrupt
> handler in page_endio().

Agree.

> Speaking of which ... this feature is far too heavy for use in production
> on pages.  You're almost trebling the size of struct page.  Can we
> do something like make all struct pages share the same lockdep_map?
> We'd have to not complain about holding one crossdep lock and acquiring
> another one of the same type, but with millions of pages in the system,
> it must surely be creating a gargantuan graph right now?

Um.. I will try it for page locks to work with one lockmap. That is also
what Peterz pointed out and what I worried about when implementing..

Thanks for your opinion.

[toc] | [prev] | [next] | [standalone]

#1592705

On Fri, Mar 03, 2017 at 08:50:03AM +0900, Byungchul Park wrote:
> On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote:
> > On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote:
> > > From: Matthew Wilcox [mailto:willy@infradead.org]
> > > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > > > > (And we should not be returning to userspace with locks held anyway --
> > > > > lockdep already has a check for that).
> > > > 
> > > > Don't we return to userspace with page locks held, eg during async
> > > > directio?
> > > 
> > > Hello,
> > > 
> > > I think that the check when returning to user with crosslocks held
> > > should be an exception. Don't you think so?
> > 
> > Oh yes.  We have to keep the pages locked during reads, and we have to
> > return to userspace before I/O is complete, therefore we have to return
> > to userspace with pages locked.  They'll be unlocked by the interrupt
> > handler in page_endio().
> 
> Agree.
> 
> > Speaking of which ... this feature is far too heavy for use in production
> > on pages.  You're almost trebling the size of struct page.  Can we
> > do something like make all struct pages share the same lockdep_map?
> > We'd have to not complain about holding one crossdep lock and acquiring
> > another one of the same type, but with millions of pages in the system,
> > it must surely be creating a gargantuan graph right now?
> 
> Um.. I will try it for page locks to work with one lockmap. That is also
> what Peterz pointed out and what I worried about when implementing..

I've thought it more and it seems not to be good. We could not use
subclass feature if we make page locks work with only one lockmap
instance. And there are several things we have to give up, that are,
things using each field in struct lockdep_map. So now, I'm not sure I
should change the current implementation. What do you think about it?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web