arm: sched: stop sched_clock() during suspend

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 <[email protected]>

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

arch/arm/include/asm/sched_clock.h +0 -2

index 05b8e82..e3f7572 100644
@@ -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

arch/arm/kernel/sched_clock.c +4 -14

index e21bac2..fc6692e 100644
@@ -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 = {

Signature · 1 line

1.8.0.rc0

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.

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.

Quoted 5 lines · Kevin Hilman

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.

Quoted 9 lines · Linus Walleij

Quoted 5 lines · Kevin Hilman

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.

Quoted 6 lines

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.

Quoted 6 lines · Russell King - ARM Linux

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?

Quoted 8 lines · Kevin Hilman

Quoted 6 lines · Russell King - ARM Linux

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.

Quoted 2 lines

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.

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 <[email protected]>

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 <[email protected]>