spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Rafael J. Wysocki]

On Tuesday 08 December 2009, Alan Stern wrote:
> On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> 
> > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > here?
> 
> And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> these routines were not intended to be called with interrupts disabled, 
> but that requirement doesn't seem to be documented.  And it isn't a 
> natural requirement anyway.

OK, let's ask Ingo about that.

Ingo, is there any particular reason why completion_done() and
try_wait_for_completion() don't use spin_lock_irqsave() and
spin_unlock_irqrestore()?

Rafael

Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33)

[Linus Torvalds]

On Tue, 8 Dec 2009, Alan Stern wrote:
>
> And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> these routines were not intended to be called with interrupts disabled, 
> but that requirement doesn't seem to be documented.  And it isn't a 
> natural requirement anyway.

'complete()' is supposed to be callable from interrupts, but the waiting 
ones aren't. But 'complete()' is all you should need to call from 
interrupts, so that's fine.

So I think completions should work, if done right. That whole "make the 
parent wait for all the children to complete" is fine in that sense. And 
I'll happily take such an approach if my rwlock thing doesn't work.

		Linus

Re: spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Tuesday 08 December 2009, Alan Stern wrote:
> > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > 
> > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > here?
> > 
> > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > these routines were not intended to be called with interrupts disabled, 
> > but that requirement doesn't seem to be documented.  And it isn't a 
> > natural requirement anyway.
> 
> OK, let's ask Ingo about that.
> 
> Ingo, is there any particular reason why completion_done() and
> try_wait_for_completion() don't use spin_lock_irqsave() and
> spin_unlock_irqrestore()?

that's a bug that should be fixed - all the wakeup side (and atomic) 
variants of completetion API should be irq safe.

It appears that these new completion APIs were added via the XFS tree 
about a year ago:

  39d2f1a: [XFS] extend completions to provide XFS object flush requirements

Please Cc: scheduler folks to all scheduler patches.

	Ingo

Re: spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Rafael J. Wysocki]

On Wednesday 09 December 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Tuesday 08 December 2009, Alan Stern wrote:
> > > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > > 
> > > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > > here?
> > > 
> > > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > > these routines were not intended to be called with interrupts disabled, 
> > > but that requirement doesn't seem to be documented.  And it isn't a 
> > > natural requirement anyway.
> > 
> > OK, let's ask Ingo about that.
> > 
> > Ingo, is there any particular reason why completion_done() and
> > try_wait_for_completion() don't use spin_lock_irqsave() and
> > spin_unlock_irqrestore()?
> 
> that's a bug that should be fixed - all the wakeup side (and atomic) 
> variants of completetion API should be irq safe.
> 
> It appears that these new completion APIs were added via the XFS tree 
> about a year ago:
> 
>   39d2f1a: [XFS] extend completions to provide XFS object flush requirements
> 
> Please Cc: scheduler folks to all scheduler patches.

If you haven't fixed it locally yet, would you mind me posting a fix?

Rafael

Re: spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Wednesday 09 December 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Tuesday 08 December 2009, Alan Stern wrote:
> > > > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > > > 
> > > > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > > > here?
> > > > 
> > > > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > > > these routines were not intended to be called with interrupts disabled, 
> > > > but that requirement doesn't seem to be documented.  And it isn't a 
> > > > natural requirement anyway.
> > > 
> > > OK, let's ask Ingo about that.
> > > 
> > > Ingo, is there any particular reason why completion_done() and
> > > try_wait_for_completion() don't use spin_lock_irqsave() and
> > > spin_unlock_irqrestore()?
> > 
> > that's a bug that should be fixed - all the wakeup side (and atomic) 
> > variants of completetion API should be irq safe.
> > 
> > It appears that these new completion APIs were added via the XFS tree 
> > about a year ago:
> > 
> >   39d2f1a: [XFS] extend completions to provide XFS object flush requirements
> > 
> > Please Cc: scheduler folks to all scheduler patches.
> 
> If you haven't fixed it locally yet, would you mind me posting a fix?

I wouldnt mind it at all.

Thanks,

	Ingo

Re: spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Dave Chinner]

On Thu, Dec 10, 2009 at 08:59:47AM +0100, Ingo Molnar wrote:
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday 09 December 2009, Ingo Molnar wrote:
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Tuesday 08 December 2009, Alan Stern wrote:
> > > > > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > > > > 
> > > > > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > > > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > > > > here?
> > > > > 
> > > > > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > > > > these routines were not intended to be called with interrupts disabled, 
> > > > > but that requirement doesn't seem to be documented.  And it isn't a 
> > > > > natural requirement anyway.

When I implemented them they were not called from anywhere that
disabled interrupts.  IIRC the main reason I used spin_lock_irq()
was because that is what wait_for_completion() used at the time....

> > > that's a bug that should be fixed - all the wakeup side (and atomic) 
> > > variants of completetion API should be irq safe.

I see no problems with that ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: spinlock in completion_done() (was: Re: Async resume patch (was: Re: [GIT PULL] PM updates for 2.6.33))

[Ingo Molnar]

* Dave Chinner <david@fromorbit.com> wrote:

> On Thu, Dec 10, 2009 at 08:59:47AM +0100, Ingo Molnar wrote:
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday 09 December 2009, Ingo Molnar wrote:
> > > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > On Tuesday 08 December 2009, Alan Stern wrote:
> > > > > > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > > > > > 
> > > > > > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > > > > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > > > > > here?
> > > > > > 
> > > > > > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > > > > > these routines were not intended to be called with interrupts disabled, 
> > > > > > but that requirement doesn't seem to be documented.  And it isn't a 
> > > > > > natural requirement anyway.
> 
> When I implemented them they were not called from anywhere that
> disabled interrupts.  IIRC the main reason I used spin_lock_irq()
> was because that is what wait_for_completion() used at the time....

Obviously wait_for_competion() as a non-atomic API that can block will 
(and should) use _irq() - but atomic variants (complete, but also the 
try-wait thing) use irqsafe methods. A fair portion of completions 
happen in IRQ context.

	Ingo

[PATCH] sched: Make wakeup side variants of completion API irq safe (was: Re: spinlock in completion_done())

[Rafael J. Wysocki]

On Thursday 10 December 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Wednesday 09 December 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Tuesday 08 December 2009, Alan Stern wrote:
> > > > > On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:
> > > > > 
> > > > > > BTW, is there a good reason why completion_done() doesn't use spin_lock_irqsave
> > > > > > and spin_unlock_irqrestore?  complete() and complete_all() use them, so why not
> > > > > > here?
> > > > > 
> > > > > And likewise in try_wait_for_completion().  It looks like a bug.  Maybe 
> > > > > these routines were not intended to be called with interrupts disabled, 
> > > > > but that requirement doesn't seem to be documented.  And it isn't a 
> > > > > natural requirement anyway.
> > > > 
> > > > OK, let's ask Ingo about that.
> > > > 
> > > > Ingo, is there any particular reason why completion_done() and
> > > > try_wait_for_completion() don't use spin_lock_irqsave() and
> > > > spin_unlock_irqrestore()?
> > > 
> > > that's a bug that should be fixed - all the wakeup side (and atomic) 
> > > variants of completetion API should be irq safe.
> > > 
> > > It appears that these new completion APIs were added via the XFS tree 
> > > about a year ago:
> > > 
> > >   39d2f1a: [XFS] extend completions to provide XFS object flush requirements
> > > 
> > > Please Cc: scheduler folks to all scheduler patches.
> > 
> > If you haven't fixed it locally yet, would you mind me posting a fix?
> 
> I wouldnt mind it at all.

Is appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: sched: Make wakeup side variants of completion API irq safe

All the wakeup side variants of the completion API shoild be irq
safe, but completion_done() and try_wait_for_completion() aren't.
Fix the problem by making them use spin_lock_irqsave() and
spin_lock_irqrestore().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/sched.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -5931,14 +5931,15 @@ EXPORT_SYMBOL(wait_for_completion_killab
  */
 bool try_wait_for_completion(struct completion *x)
 {
+	unsigned long flags;
 	int ret = 1;
 
-	spin_lock_irq(&x->wait.lock);
+	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
 		ret = 0;
 	else
 		x->done--;
-	spin_unlock_irq(&x->wait.lock);
+	spin_unlock_irqrestore(&x->wait.lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(try_wait_for_completion);
@@ -5953,12 +5954,13 @@ EXPORT_SYMBOL(try_wait_for_completion);
  */
 bool completion_done(struct completion *x)
 {
+	unsigned long flags;
 	int ret = 1;
 
-	spin_lock_irq(&x->wait.lock);
+	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
 		ret = 0;
-	spin_unlock_irq(&x->wait.lock);
+	spin_unlock_irqrestore(&x->wait.lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(completion_done);

[tip:sched/urgent] sched: Make wakeup side and atomic variants of completion API irq safe

[tip-bot for Rafael J. Wysocki]

Commit-ID:  7539a3b3d1f892dd97eaf094134d7de55c13befe
Gitweb:     http://git.kernel.org/tip/7539a3b3d1f892dd97eaf094134d7de55c13befe
Author:     Rafael J. Wysocki <rjw@sisk.pl>
AuthorDate: Sun, 13 Dec 2009 00:07:30 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 13 Dec 2009 08:12:46 +0100

sched: Make wakeup side and atomic variants of completion API irq safe

Alan Stern noticed that all the wakeup side (and atomic) variants of the
completion APIs should be irq safe, but the newly introduced
completion_done() and try_wait_for_completion() aren't. The use of the
irq unsafe variants in IRQ contexts can cause crashes/hangs.

Fix the problem by making them use spin_lock_irqsave() and
spin_lock_irqrestore().

Reported-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: pm list <linux-pm@lists.linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: David Chinner <david@fromorbit.com>
Cc: Lachlan McIlroy <lachlan@sgi.com>
LKML-Reference: <200912130007.30541.rjw@sisk.pl>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index ff39cad..8b3532f 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5908,14 +5908,15 @@ EXPORT_SYMBOL(wait_for_completion_killable);
  */
 bool try_wait_for_completion(struct completion *x)
 {
+	unsigned long flags;
 	int ret = 1;
 
-	spin_lock_irq(&x->wait.lock);
+	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
 		ret = 0;
 	else
 		x->done--;
-	spin_unlock_irq(&x->wait.lock);
+	spin_unlock_irqrestore(&x->wait.lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(try_wait_for_completion);
@@ -5930,12 +5931,13 @@ EXPORT_SYMBOL(try_wait_for_completion);
  */
 bool completion_done(struct completion *x)
 {
+	unsigned long flags;
 	int ret = 1;
 
-	spin_lock_irq(&x->wait.lock);
+	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
 		ret = 0;
-	spin_unlock_irq(&x->wait.lock);
+	spin_unlock_irqrestore(&x->wait.lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(completion_done);