[PATCH] arm: sched: stop sched_clock() during suspend

[PATCH] arm: sched: stop sched_clock() during suspend

[Felipe Balbi]

The scheduler imposes a requirement to sched_clock()
which is to stop the clock during suspend, if we don't
do that IRQ threads will be rescheduled in the future
which might cause transfers to timeout depending on
how driver is written.

This became an issue on OMAP when we converted omap-i2c.c
to use threaded IRQs, it turned out that depending on how
much time we spent on suspend, the I2C IRQ thread would
end up being rescheduled so far in the future that I2C
transfers would timeout and, because omap_hsmmc depends
on an I2C-connected device to detect if an MMC card is
inserted in the slot, our rootfs would just vanish.

arch/arm/kernel/sched_clock.c already had an optional
implementation (sched_clock_needs_suspend()) which would
handle scheduler's requirement properly, what this patch
does is simply to make that implementation non-optional.

This has been tested with beagleboard XM (OMAP3630) and
pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
after this patch.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 arch/arm/include/asm/sched_clock.h |  2 --
 arch/arm/kernel/sched_clock.c      | 18 ++++--------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
index 05b8e82..e3f7572 100644
--- a/arch/arm/include/asm/sched_clock.h
+++ b/arch/arm/include/asm/sched_clock.h
@@ -10,7 +10,5 @@
 
 extern void sched_clock_postinit(void);
 extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
-extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
-		unsigned long rate);
 
 #endif
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index e21bac2..fc6692e 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -107,13 +107,6 @@ static void sched_clock_poll(unsigned long wrap_ticks)
 	update_sched_clock();
 }
 
-void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
-		unsigned long rate)
-{
-	setup_sched_clock(read, bits, rate);
-	cd.needs_suspend = true;
-}
-
 void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 {
 	unsigned long r, w;
@@ -189,18 +182,15 @@ void __init sched_clock_postinit(void)
 static int sched_clock_suspend(void)
 {
 	sched_clock_poll(sched_clock_timer.data);
-	if (cd.needs_suspend)
-		cd.suspended = true;
+	cd.suspended = true;
 	return 0;
 }
 
 static void sched_clock_resume(void)
 {
-	if (cd.needs_suspend) {
-		cd.epoch_cyc = read_sched_clock();
-		cd.epoch_cyc_copy = cd.epoch_cyc;
-		cd.suspended = false;
-	}
+	cd.epoch_cyc = read_sched_clock();
+	cd.epoch_cyc_copy = cd.epoch_cyc;
+	cd.suspended = false;
 }
 
 static struct syscore_ops sched_clock_ops = {
-- 
1.8.0.rc0

[PATCH] arm: sched: stop sched_clock() during suspend

[Felipe Balbi]

Hi,

On Mon, Oct 22, 2012 at 02:54:37PM +0300, Felipe Balbi wrote:
> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.
> 
> This became an issue on OMAP when we converted omap-i2c.c
> to use threaded IRQs, it turned out that depending on how
> much time we spent on suspend, the I2C IRQ thread would
> end up being rescheduled so far in the future that I2C
> transfers would timeout and, because omap_hsmmc depends
> on an I2C-connected device to detect if an MMC card is
> inserted in the slot, our rootfs would just vanish.
> 
> arch/arm/kernel/sched_clock.c already had an optional
> implementation (sched_clock_needs_suspend()) which would
> handle scheduler's requirement properly, what this patch
> does is simply to make that implementation non-optional.
> 
> This has been tested with beagleboard XM (OMAP3630) and
> pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
> after this patch.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>

just adding more guys to Cc. Please add more if I have missed someone.

> ---
>  arch/arm/include/asm/sched_clock.h |  2 --
>  arch/arm/kernel/sched_clock.c      | 18 ++++--------------
>  2 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
> index 05b8e82..e3f7572 100644
> --- a/arch/arm/include/asm/sched_clock.h
> +++ b/arch/arm/include/asm/sched_clock.h
> @@ -10,7 +10,5 @@
>  
>  extern void sched_clock_postinit(void);
>  extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
> -extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
> -		unsigned long rate);
>  
>  #endif
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index e21bac2..fc6692e 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -107,13 +107,6 @@ static void sched_clock_poll(unsigned long wrap_ticks)
>  	update_sched_clock();
>  }
>  
> -void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
> -		unsigned long rate)
> -{
> -	setup_sched_clock(read, bits, rate);
> -	cd.needs_suspend = true;
> -}
> -
>  void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
>  {
>  	unsigned long r, w;
> @@ -189,18 +182,15 @@ void __init sched_clock_postinit(void)
>  static int sched_clock_suspend(void)
>  {
>  	sched_clock_poll(sched_clock_timer.data);
> -	if (cd.needs_suspend)
> -		cd.suspended = true;
> +	cd.suspended = true;
>  	return 0;
>  }
>  
>  static void sched_clock_resume(void)
>  {
> -	if (cd.needs_suspend) {
> -		cd.epoch_cyc = read_sched_clock();
> -		cd.epoch_cyc_copy = cd.epoch_cyc;
> -		cd.suspended = false;
> -	}
> +	cd.epoch_cyc = read_sched_clock();
> +	cd.epoch_cyc_copy = cd.epoch_cyc;
> +	cd.suspended = false;
>  }
>  
>  static struct syscore_ops sched_clock_ops = {
> -- 
> 1.8.0.rc0
> 

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121022/01b6c272/attachment.sig>

[PATCH] arm: sched: stop sched_clock() during suspend

[Kevin Hilman]

+Colin Cross, Barry Song also

Felipe Balbi <balbi@ti.com> writes:

> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.

It's not just about IRQ threads, it's about RT throttling.  IOW, not
just IRQ threads will be postponed, but all RT tasks will be throttled
temporarily as well.

The changelog should also mention that this has an inconvenient side
effect of stopping the printk times during suspend.  Based on the
original thread where this feature was discussed and introduced, some
platforms wanted to opt out of this behavior[1], so the optional API was
added.

However, in light of RT throttling, this a correctness issue for process
accounting, so I agree that this should be done for all platforms
instead of providing an optional 'needs suspend' version of the API,
even though it means printk times no longer reflect time spent
suspended.

After a discussion with peterz on this topic, it seems that x86
already ensures that sched_clock stops during suspend for similar
reasons[2].

The question then is whether this is a fix that belongs in v3.7.
Technically, it is not a regression, so I think this should probably be
v3.8 material.  If that's the decision, then the threaded IRQ support
for the OMAP I2C driver needs to be reverted for v3.7 until this fix is merged.

Kevin

[1] http://marc.info/?l=linux-arm-kernel&m=134307004508708&w=2
[2] http://marc.info/?l=linux-arm-kernel&m=135065529907297&w=2

[PATCH] arm: sched: stop sched_clock() during suspend

[Linus Walleij]

On Mon, Oct 22, 2012 at 7:05 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:

> However, in light of RT throttling, this a correctness issue for process
> accounting, so I agree that this should be done for all platforms
> instead of providing an optional 'needs suspend' version of the API,
> even though it means printk times no longer reflect time spent
> suspended.

Maybe we should get printk() to use the best clocksource
instead.

The reason AFAICT that printk() is using sched_clock() is that
it's supposed to be fast. But now it seems that it's not going
to return what printk() needs anymore.

(Dragging Stultz & Gleixner into this...)

Yours,
Linus Walleij

[PATCH] arm: sched: stop sched_clock() during suspend

[Russell King - ARM Linux]

On Tue, Oct 23, 2012 at 12:28:32AM +0200, Linus Walleij wrote:
> On Mon, Oct 22, 2012 at 7:05 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
> 
> > However, in light of RT throttling, this a correctness issue for process
> > accounting, so I agree that this should be done for all platforms
> > instead of providing an optional 'needs suspend' version of the API,
> > even though it means printk times no longer reflect time spent
> > suspended.
> 
> Maybe we should get printk() to use the best clocksource
> instead.
> 
> The reason AFAICT that printk() is using sched_clock() is that
> it's supposed to be fast. But now it seems that it's not going
> to return what printk() needs anymore.

No, printk() does not need this.  You think it does, but it doesn't.  What
we have is a difference between ARM and x86, and this difference is breaking
the scheduler.

The fact that the printk timestamp increments while suspended is a bug.  It
doesn't on x86.

There's three other functions in the kernel which do return updated time.
I'm sure one of those can be used to printk() the time passed in suspend
at resume time, which would give the information required.

[PATCH] arm: sched: stop sched_clock() during suspend

[Kevin Hilman]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Tue, Oct 23, 2012 at 12:28:32AM +0200, Linus Walleij wrote:
>> On Mon, Oct 22, 2012 at 7:05 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>> 
>> > However, in light of RT throttling, this a correctness issue for process
>> > accounting, so I agree that this should be done for all platforms
>> > instead of providing an optional 'needs suspend' version of the API,
>> > even though it means printk times no longer reflect time spent
>> > suspended.
>> 
>> Maybe we should get printk() to use the best clocksource
>> instead.
>> 
>> The reason AFAICT that printk() is using sched_clock() is that
>> it's supposed to be fast. But now it seems that it's not going
>> to return what printk() needs anymore.
>
> No, printk() does not need this.  You think it does, but it doesn't.  What
> we have is a difference between ARM and x86, and this difference is breaking
> the scheduler.
>
> The fact that the printk timestamp increments while suspended is a bug.  It
> doesn't on x86.

Russell, I agree that it's a bug, but does it qualify as a something
you're willing to take for v3.7-rc?

For OMAP, we need to know if this will go for v3.7 or not because
there's a regression in the OMAP I2C driver, and if this doesn't go in,
we'll need to revert something in the I2C driver until it does.

Thanks,

Kevin

[PATCH] arm: sched: stop sched_clock() during suspend

[Russell King - ARM Linux]

On Tue, Oct 23, 2012 at 07:17:33AM -0700, Kevin Hilman wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> > No, printk() does not need this.  You think it does, but it doesn't.  What
> > we have is a difference between ARM and x86, and this difference is breaking
> > the scheduler.
> >
> > The fact that the printk timestamp increments while suspended is a bug.  It
> > doesn't on x86.
> 
> Russell, I agree that it's a bug, but does it qualify as a something
> you're willing to take for v3.7-rc?

Definitely.  Our current behaviour across suspend for the scheduler is
wrong.  This is one of the questions I had when I created the sched_clock
stuff - but no one at the time could answer.  So, now that we have our
answer, let's get it fixed to conform.

[PATCH] arm: sched: stop sched_clock() during suspend

[Felipe Balbi]

Hi,

On Tue, Oct 23, 2012 at 05:03:31PM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2012 at 07:17:33AM -0700, Kevin Hilman wrote:
> > Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> > > No, printk() does not need this.  You think it does, but it doesn't.  What
> > > we have is a difference between ARM and x86, and this difference is breaking
> > > the scheduler.
> > >
> > > The fact that the printk timestamp increments while suspended is a bug.  It
> > > doesn't on x86.
> > 
> > Russell, I agree that it's a bug, but does it qualify as a something
> > you're willing to take for v3.7-rc?
> 
> Definitely.  Our current behaviour across suspend for the scheduler is
> wrong.  This is one of the questions I had when I created the sched_clock
> stuff - but no one at the time could answer.  So, now that we have our
> answer, let's get it fixed to conform.

I have just pushed the patch the your patch system, hope I did it all
right.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121023/827d4ccb/attachment-0001.sig>

[PATCH v2] arm: sched: stop sched_clock() during suspend

[Felipe Balbi]

The scheduler imposes a requirement to sched_clock()
which is to stop the clock during suspend, if we don't
do that any RT thread will be rescheduled in the future
which might cause any sort of problems.

This became an issue on OMAP when we converted omap-i2c.c
to use threaded IRQs, it turned out that depending on how
much time we spent on suspend, the I2C IRQ thread would
end up being rescheduled so far in the future that I2C
transfers would timeout and, because omap_hsmmc depends
on an I2C-connected device to detect if an MMC card is
inserted in the slot, our rootfs would just vanish.

arch/arm/kernel/sched_clock.c already had an optional
implementation (sched_clock_needs_suspend()) which would
handle scheduler's requirement properly, what this patch
does is simply to make that implementation non-optional.

Note that this has the side-effect that printk timings
won't reflect the actual time spent on suspend so other
methods to measure that will have to be used.

This has been tested with beagleboard XM (OMAP3630) and
pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
after this patch.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

Changes since v1:
	- changed commit log to mention that all RT threads will be
	  rescheduled to the future.
	- changed commit log to mention the side effect on printk
	  timings

 arch/arm/include/asm/sched_clock.h |  2 --
 arch/arm/kernel/sched_clock.c      | 18 ++++--------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
index 05b8e82..e3f7572 100644
--- a/arch/arm/include/asm/sched_clock.h
+++ b/arch/arm/include/asm/sched_clock.h
@@ -10,7 +10,5 @@
 
 extern void sched_clock_postinit(void);
 extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
-extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
-		unsigned long rate);
 
 #endif
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index e21bac2..fc6692e 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -107,13 +107,6 @@ static void sched_clock_poll(unsigned long wrap_ticks)
 	update_sched_clock();
 }
 
-void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
-		unsigned long rate)
-{
-	setup_sched_clock(read, bits, rate);
-	cd.needs_suspend = true;
-}
-
 void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 {
 	unsigned long r, w;
@@ -189,18 +182,15 @@ void __init sched_clock_postinit(void)
 static int sched_clock_suspend(void)
 {
 	sched_clock_poll(sched_clock_timer.data);
-	if (cd.needs_suspend)
-		cd.suspended = true;
+	cd.suspended = true;
 	return 0;
 }
 
 static void sched_clock_resume(void)
 {
-	if (cd.needs_suspend) {
-		cd.epoch_cyc = read_sched_clock();
-		cd.epoch_cyc_copy = cd.epoch_cyc;
-		cd.suspended = false;
-	}
+	cd.epoch_cyc = read_sched_clock();
+	cd.epoch_cyc_copy = cd.epoch_cyc;
+	cd.suspended = false;
 }
 
 static struct syscore_ops sched_clock_ops = {
-- 
1.8.0.rc0

[PATCH v2] arm: sched: stop sched_clock() during suspend

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that any RT thread will be rescheduled in the future
> which might cause any sort of problems.
>
> This became an issue on OMAP when we converted omap-i2c.c
> to use threaded IRQs, it turned out that depending on how
> much time we spent on suspend, the I2C IRQ thread would
> end up being rescheduled so far in the future that I2C
> transfers would timeout and, because omap_hsmmc depends
> on an I2C-connected device to detect if an MMC card is
> inserted in the slot, our rootfs would just vanish.
>
> arch/arm/kernel/sched_clock.c already had an optional
> implementation (sched_clock_needs_suspend()) which would
> handle scheduler's requirement properly, what this patch
> does is simply to make that implementation non-optional.
>
> Note that this has the side-effect that printk timings
> won't reflect the actual time spent on suspend so other
> methods to measure that will have to be used.
>
> This has been tested with beagleboard XM (OMAP3630) and
> pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
> after this patch.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>

Acked-by: Kevin Hilman <khilman@ti.com>

[PATCH] arm: sched: stop sched_clock() during suspend

[Linus Walleij]

On Mon, Oct 22, 2012 at 1:54 PM, Felipe Balbi <balbi@ti.com> wrote:

> The scheduler imposes a requirement to sched_clock()
> which is to stop the clock during suspend, if we don't
> do that IRQ threads will be rescheduled in the future
> which might cause transfers to timeout depending on
> how driver is written.
>
> This became an issue on OMAP when we converted omap-i2c.c
> to use threaded IRQs, it turned out that depending on how
> much time we spent on suspend, the I2C IRQ thread would
> end up being rescheduled so far in the future that I2C
> transfers would timeout and, because omap_hsmmc depends
> on an I2C-connected device to detect if an MMC card is
> inserted in the slot, our rootfs would just vanish.
>
> arch/arm/kernel/sched_clock.c already had an optional
> implementation (sched_clock_needs_suspend()) which would
> handle scheduler's requirement properly, what this patch
> does is simply to make that implementation non-optional.
>
> This has been tested with beagleboard XM (OMAP3630) and
> pandaboard rev A3 (OMAP4430). Suspend to RAM is now working
> after this patch.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>

After Russell explains so I get it:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij