Extreme time jitter with suspend/resume cycles

[Gabriel Beddingfield]

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.

Thanks,
Gabe

[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
Subject: [PATCH] time: add CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION to disable
 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.
 
+config PERSISTENT_CLOCK_IS_LOW_PRECISION
+	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.
+
 endmenu
 endif
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;
+#ifdef CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION
 	struct timespec64		delta, delta_delta;
 	static struct timespec64	old_delta;
+#endif
 
 	read_persistent_clock64(&timekeeping_suspend_time);
 
@@ -1712,6 +1714,7 @@ int timekeeping_suspend(void)
 	timekeeping_forward_now(tk);
 	timekeeping_suspended = 1;
 
+#ifdef CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION
 	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);
 		}
 	}
+#endif
 
 	timekeeping_update(tk, TK_MIRROR);
 	halt_fast_timekeeper(tk);
-- 
2.14.2.920.gcf0c67979c-goog