Excessive delay or hang during resume from system suspend due to a hrtimer commit
Hi Linus,
Please revert:
commit 5baefd6d84163443215f4a99f6a20f054ef11236
Author: John Stultz <[email protected]> Date: Tue Jul 10 18:43:25 2012 -0400
hrtimer: Update hrtimer base offsets each hrtimer_interrupt
This breaks resume on the iBook G4 and Toshiba Portege R500 (at least), by adding an excessive delay to it (the Toshiba box sometimes hangs hard during resume from system suspend). According to Andreas
(https://lkml.org/lkml/2012/7/15/66):
"Apparently during or before noirq resume the system is hanging by the same amount of time as the system was sleeping."
which seems to agree with my observations.
Given that the two known-affected boxes are so different, it is quite probable that the total number of affected systems is actually quite high.
Thanks!
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Thanks,
Rafael
Quoted 3 lines · Rafael J. Wysocki
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Well, we spent an massive amount of time in testing, reviewing and discussion and it definitely did not break suspend/resume here.
This was not pushed without a lot of thoughts and in fact what you are seing is another long standing bug in the timekeeping resume code, which was just papered over by the incorrect handling of the clock was set cases in the other parts of the system.
Does the following patch fix the problem for you ?
@John: Should that clear ntp as well or is it enough to set ntp_error
to 0 ?
/me really goes on vacation now.
Thanks,
tglx
---------
kernel/time/timekeeping.c +1 -0
@@ -717,6 +717,7 @@ static void timekeeping_resume(void) timekeeper.clock->cycle_last = timekeeper.clock->read (timekeeper.clock); timekeeper.ntp_error =0 ; timekeeping_suspended =0 ; +timekeeping_update (false);write_sequnlock_irqrestore (&timekeeper.lock, flags);touch_softlockup_watchdog ();
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Rafael J. Wysocki
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Well, we spent an massive amount of time in testing, reviewing and discussion and it definitely did not break suspend/resume here.
I'm not saying that you didn't consider it thoroughly, but unfortunately you did overlook this particular issue, didn't you?
Quoted 6 lines
This was not pushed without a lot of thoughts and in fact what you are seing is another long standing bug in the timekeeping resume code, which was just papered over by the incorrect handling of the clock was set cases in the other parts of the system.
Does the following patch fix the problem for you ?
Yes, it does, thanks!
Quoted 4 lines
@John: Should that clear ntp as well or is it enough to set
ntp_errorto 0 ?/me really goes on vacation now.
So who's going to take care of the patch? :-)
Rafael
Quoted 16 lines
---------
kernel/time/timekeeping.c +1
-0@@ -717,6 +717,7 @@ static void timekeeping_resume(void) timekeeper.clock->cycle_last = timekeeper.clock->read (timekeeper.clock); timekeeper.ntp_error =0 ; timekeeping_suspended =0 ; +timekeeping_update (false);write_sequnlock_irqrestore (&timekeeper.lock, flags);touch_softlockup_watchdog ();
Quoted 19 lines · Rafael J. Wysocki
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Rafael J. Wysocki
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Well, we spent an massive amount of time in testing, reviewing and discussion and it definitely did not break suspend/resume here.
I'm not saying that you didn't consider it thoroughly, but unfortunately you did overlook this particular issue, didn't you?
Quoted 6 lines
This was not pushed without a lot of thoughts and in fact what you are seing is another long standing bug in the timekeeping resume code, which was just papered over by the incorrect handling of the clock was set cases in the other parts of the system.
Does the following patch fix the problem for you ?
Quoted 8 lines
Yes, it does, thanks!
Quoted 4 lines
@John: Should that clear ntp as well or is it enough to set
ntp_errorto 0 ?/me really goes on vacation now.
So who's going to take care of the patch? :-)
I'm still packing gear. So i'll push it into timers/urgent.
Quoted 23 lines · Thomas Gleixner
Quoted 21 lines · Rafael J. Wysocki
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Rafael J. Wysocki
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Well, we spent an massive amount of time in testing, reviewing and discussion and it definitely did not break suspend/resume here.
I'm not saying that you didn't consider it thoroughly, but unfortunately you did overlook this particular issue, didn't you?
Quoted 6 lines
This was not pushed without a lot of thoughts and in fact what you are seing is another long standing bug in the timekeeping resume code, which was just papered over by the incorrect handling of the clock was set cases in the other parts of the system.
Does the following patch fix the problem for you ?
Yes, it does, thanks!
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
@John: Should that clear ntp as well or is it enough to set
ntp_errorto 0 ?/me really goes on vacation now.
So who's going to take care of the patch? :-)
I'm still packing gear. So i'll push it into timers/urgent.
Actually that's a bad idea. John want's to double check vs. the ntp_clear question. So John can send it to linus directly.
@John: Should it be: timekeeping_update(true)
Now I'm gone for real.
Thanks,
tglx
-----
Subject: timekeeping: Add missing update call in timekeeping_resume()
From: Thomas Gleixner <[email protected]>
Date: Mon, 16 Jul 2012 11:47:31 +0200 (CEST)
The leap second rework unearthed another issue of inconsistent data.
On timekeeping_resume() the timekeeper data is updated, but nothing calls timekeeping_update(), so now the update code in the timer interrupt sees stale values.
This has been the case before those changes, but then the timer interrupt was using stale data as well so this went unnoticed for quite some time.
Add the missing update call, so all the data is consistent everywhere.
kernel/time/timekeeping.c +1 -0
@@ -717,6 +717,7 @@ static void timekeeping_resume(void) timekeeper.clock->cycle_last = timekeeper.clock->read (timekeeper.clock); timekeeper.ntp_error =0 ; timekeeping_suspended =0 ; +timekeeping_update (false);write_sequnlock_irqrestore (&timekeeper.lock, flags);touch_softlockup_watchdog ();
Quoted 25 lines · Thomas Gleixner
Quoted 23 lines · Thomas Gleixner
Quoted 21 lines · Rafael J. Wysocki
Quoted 6 lines · Thomas Gleixner
Quoted 3 lines · Rafael J. Wysocki
To everyone involved: the fact that this change, which was likely to introduce regressions from the look of it alone, has been pushed to Linus (an to -stable at the same time!) so late in the cycle, is seriuosly disappointing.
Well, we spent an massive amount of time in testing, reviewing and discussion and it definitely did not break suspend/resume here.
I'm not saying that you didn't consider it thoroughly, but unfortunately you did overlook this particular issue, didn't you?
Quoted 6 lines
This was not pushed without a lot of thoughts and in fact what you are seing is another long standing bug in the timekeeping resume code, which was just papered over by the incorrect handling of the clock was set cases in the other parts of the system.
Does the following patch fix the problem for you ?
Yes, it does, thanks!
Quoted 3 lines
@John: Should that clear ntp as well or is it enough to set
ntp_errorto 0 ?
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
/me really goes on vacation now.
So who's going to take care of the patch? :-)
I'm still packing gear. So i'll push it into timers/urgent.
Actually that's a bad idea. John want's to double check vs. the
ntp_clearquestion. So John can send it to linus directly.@John: Should it be:
timekeeping_update(true)
I think its better to leave it as false, so we don't reset the NTP state machine completely after suspend.
When we come back from suspend our error is usually off by the persistent_clock/rtc granularity, so it might make sense, but I'd want a lot more testing of using ntp over suspend before changing the existing behavior of not doing it.
Quoted 1 line
Now I'm gone for real.
Ok. Thanks for spinning this up so quickly. I'll go ahead and send it on to Linus.
thanks
-john
Quoted 4 lines · Rafael J. Wysocki
Quoted 1 line · Thomas Gleixner
Does the following patch fix the problem for you ?
Yes, it does, thanks!
Works for me as well.
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."