Extreme time jitter with suspend/resume cycles

From: Gabriel Beddingfield
Date: Wed Oct 04 2017 - 12:11:21 EST

TL;DR: the "delta_delta" hack[1 and 2] in kernel/time/timekeeping.c
and drivers/rtc/class.c undermines the NTP system. It's not
appropriate to use if sub-second precision is available. I've attached
a patch to resolve this... please let me know the ways you hate it.

Hello Kernel Timekeeping Maintainers,

We have been developing a device that has very a very aggressive power
policy, doing suspend/resume cycles a few times a minute ("echo mem >
/sys/power/state"). In doing so, we found that the system time
experiences a lot of jitter (compared to, say, an NTP server). It was
not uncommon for us to see time corrections of 1s to 4s on a regular
basis. This didn't happen when the device stayed awake, only when it
was allowed to do suspend/resume.

We found that the problem is an interaction between the NTP code and
what I call the "delta_delta hack." (see [1] and [2]) This code
allocates a static variable in a function that contains an offset from
the system time to the persistent/rtc clock. It uses that time to
fudge the suspend timestamp so that on resume the sleep time will be
compensated. It's kind of a statistical hack that assumes things will
average out. It seems to have two main assumptions:

1. The persistent/rtc clock has only single-second precision
2. The system does not frequently suspend/resume.
3. If delta_delta is less than 2 seconds, these assumptions are "true"

Because the delta_delta hack is trying to maintain an offset from the
system time to the persistent/rtc clock, any minor NTP corrections
that have occurred since the last suspend will be discarded. However,
the NTP subsystem isn't notified that this is happening -- and so it
causes some level of instability in its PLL logic.

This problem affects any device that does "frequent" suspend/resume
cycles. I.e. any battery-powered "linux" device (like Android phones).

Many ARM systems provide a "persistent clock." Most of them are backed
by a 32kHz clock that gives good precision and makes the delta_delta
hack unnecessary. However, devices that only have single-second
precision for the persistent clock and/or are forced to use the RTC
(whose API only allows for single-second precision) -- they still need
this hack.

I've attached a patch that we developed in-house. I have a feeling you
won't like it... since it pushes the responsibility on whoever
configures the kernel. It also ignores the RTC problem (which will
still affect a lot of battery-powered devices).

Please let me know what you think -- and what the right approach for
solving this would be.


[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/kernel/time/timekeeping.c?h=v4.13.4#n1717
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/class.c?h=v4.13.4#n76
From c03ceced9a210b48f2552e7dafa9099ef2449370 Mon Sep 17 00:00:00 2001
From: "Gabriel M. Beddingfield" <beddingfield@xxxxxxxxxxxx>
Date: Wed, 4 Oct 2017 08:38:57 -0700
suspend/resume hack

Signed-off-by: Gabriel M. Beddingfield <beddingfield@xxxxxxxxxxxx>
Signed-off-by: Guy <guy@xxxxxxxxxxxx>
kernel/time/Kconfig | 16 ++++++++++++++++
kernel/time/timekeeping.c | 4 ++++
2 files changed, 20 insertions(+)

diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index ac09bc29eb08..32d54086c96c 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -143,5 +143,21 @@ config HIGH_RES_TIMERS
hardware is not capable then this option only increases
the size of the kernel image.

+ bool "The persistent clock has only single-second precision"
+ default y
+ help
+ When enabled, then on suspend/resume the timekeeping code will
+ try to maintain a constant offset between the system time and
+ the persistent clock as a means of compensating for the coarse
+ (+/- 1 second) sleep time calculation. However, this will discard
+ any "small" NTP corrections that have happened since the last
+ resume. However, if the system's persistent clock has better
+ precision (e.g. because it's backed by a 32kHz clock), this is
+ not necessary and introduces unneeded time jitter.
+ If your persistent clock has only single-second precision, say Y.
+ If your persistent clock has sub-second precision, say N.
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2cafb49aa65e..b2c7b443ef37 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1694,8 +1694,10 @@ int timekeeping_suspend(void)
struct timekeeper *tk = &tk_core.timekeeper;
unsigned long flags;
struct timespec64 delta, delta_delta;
static struct timespec64 old_delta;


@@ -1712,6 +1714,7 @@ int timekeeping_suspend(void)
timekeeping_suspended = 1;

if (persistent_clock_exists) {
* To avoid drift caused by repeated suspend/resumes,
@@ -1733,6 +1736,7 @@ int timekeeping_suspend(void)
timespec64_add(timekeeping_suspend_time, delta_delta);

timekeeping_update(tk, TK_MIRROR);