Re: TSC not updating after resume: Bug or Feature?

[Thomas Gleixner <tglx@linutronix.de>]

Fabio,

On Mon, 22 Dec 2008, Fabio Comolli wrote:
> On Mon, Dec 22, 2008 at 10:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Can please you revert the last patch and apply the following ? Does
> > the WARN_ON trigger ?
> >

> Yes, it does:
 
> [  159.768005]  [<c01389a2>] getnstimeofday+0x21/0xcd
> [  159.768005]  [<c0135e26>] ktime_get_ts+0x1d/0x3f
> [  159.768005]  [<c0135e57>] ktime_get+0xf/0x2b
> [  159.768005]  [<c013736b>] sched_clock_tick+0x46/0x83
> [  159.768005]  [<c01373ad>] sched_clock_idle_wakeup_event+0x5/0xa
> [  159.768005]  [<c0108670>] set_cyc2ns_scale+0x3f/0x5e
> [  159.768005]  [<c01088af>] time_cpufreq_notifier+0xf9/0x103
> [  159.768005]  [<c0136782>] notifier_call_chain+0x2a/0x52
> [  159.768005]  [<c0136866>] __srcu_notifier_call_chain+0x35/0x4a
> [  159.768005]  [<c0136884>] srcu_notifier_call_chain+0x9/0xc
> [  159.768005]  [<c031cbca>] cpufreq_resume+0xf3/0x112
> [  159.768005]  [<c028c3f3>] __sysdev_resume+0x24/0x34
> [  159.768005]  [<c028c421>] sysdev_resume+0x1e/0x50

Thanks for testing. It's exaclty the code path I described :)

So my code analysis holds and your test confirms my suspicion that
Shaggy's patch just unearthed some other weirdness in the
suspend/resume code.

Can you please apply the following hack^Wpatch and retest ? It
restores Shaggys patch, but prevents the sched_clock_tick() call when
timekeeping is not resumed. The WARN_ON should not longer trigger
except there is some other code path which fiddles with that as well.

If I'm not completely nuts then this should solve your suspend/resume
problem really instead of papering over the root cause.

The patch should work on top of 2.6.27.10 as well, so whatever is
easier to verify for you is fine.

Thanks,

	tglx
---
diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index e8ab096..dd97801 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -124,7 +124,7 @@ static u64 __update_sched_clock(struct sched_clock_data *scd, u64 now)
 
 	clock = scd->tick_gtod + delta;
 	min_clock = wrap_max(scd->tick_gtod, scd->clock);
-	max_clock = scd->tick_gtod + TICK_NSEC;
+	max_clock = wrap_max(scd->clock, scd->tick_gtod + TICK_NSEC);
 
 	clock = wrap_max(clock, min_clock);
 	clock = wrap_min(clock, max_clock);
@@ -222,11 +222,16 @@ void sched_clock_idle_sleep_event(void)
 }
 EXPORT_SYMBOL_GPL(sched_clock_idle_sleep_event);
 
+extern int timekeeping_suspended;
+
 /*
  * We just idled delta nanoseconds (called with irqs disabled):
  */
 void sched_clock_idle_wakeup_event(u64 delta_ns)
 {
+	if (timekeeping_suspended)
+		return;
+
 	sched_clock_tick();
 	touch_softlockup_watchdog();
 }
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index fa05e88..50ba3d0 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -46,6 +46,9 @@ struct timespec xtime __attribute__ ((aligned (16)));
 struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
 static unsigned long total_sleep_time;		/* seconds */
 
+/* flag for if timekeeping is suspended */
+int timekeeping_suspended;
+
 static struct timespec xtime_cache __attribute__ ((aligned (16)));
 void update_xtime_cache(u64 nsec)
 {
@@ -92,6 +95,8 @@ void getnstimeofday(struct timespec *ts)
 	unsigned long seq;
 	s64 nsecs;
 
+	WARN_ON(timekeeping_suspended);
+
 	do {
 		seq = read_seqbegin(&xtime_lock);
 
@@ -299,8 +304,6 @@ void __init timekeeping_init(void)
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
 
-/* flag for if timekeeping is suspended */
-static int timekeeping_suspended;
 /* time in seconds when suspend began */
 static unsigned long timekeeping_suspend_time;