From patchwork Wed Oct 4 16:11:13 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gabriel Beddingfield X-Patchwork-Id: 821383 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-rtc-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=nestlabs.com header.i=@nestlabs.com header.b="OUBCarjx"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3y6gr570BCz9s7C for ; Thu, 5 Oct 2017 03:11:17 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751170AbdJDQLR (ORCPT ); Wed, 4 Oct 2017 12:11:17 -0400 Received: from mail-qk0-f169.google.com ([209.85.220.169]:46478 "EHLO mail-qk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751120AbdJDQLQ (ORCPT ); Wed, 4 Oct 2017 12:11:16 -0400 Received: by mail-qk0-f169.google.com with SMTP id k123so8479824qke.3 for ; Wed, 04 Oct 2017 09:11:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nestlabs.com; s=google; h=mime-version:from:date:message-id:subject:to:cc; bh=38xH5Fz87uLu7orDKsIrrqTcpkDYqygxp82YrJAH28U=; b=OUBCarjxX1ufMAPP2jpHEsSWO3WEuHNHRNFXgWQKE3lkzFT4DriPY5JkXp0QhxFgH+ yVdBgYMwVY3GVdp52HaL1JWNMJ2j1Ve4tbtTW9yPJA8OPbBguvKf119Pq9yyoLwZki1H mnPR2vFvoUo060RC1bqNgPlgdNA13xfg+UB8s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to:cc; bh=38xH5Fz87uLu7orDKsIrrqTcpkDYqygxp82YrJAH28U=; b=YF9Coii/xTOgDQjRG9VSXij2KSHvbnDQbLD4TzT9uYb6ds0ts3l3qj0LSXeo6oQnXj g2VvIXMvxK4aw56Tw9PpLVE2Zvst64F2rww4rbIbIOrb2nuQjeXbq/MXpdXtge3EJ4Bp V46hDJtRBd1shdnue9dKqLCgN4MzFEVvKESXjOVtOiIWt8wReQiTXI/1LVnT5BE7cQFc 20qs9GNPyXdBu+gihMIDeNkaof4ZDgQZW8ltBZt7BLbcxrFqyITFJ+E22bEtAZGDd3uM INqkK7xfzT3kDAbG2Luak1Ks5wdzjS2Cv2jvWJyHVvjM12jAe/rN4yxFKcRJTGW/mZDo eKBQ== X-Gm-Message-State: AMCzsaX6Yh6YHn3MzSiF36A2YRFlfpfu99ngmyOXEUxUFmDZVgsFNNrG dCAj6DiHLw4hVzS3jXfRm5PpdbJis3D1NMyvKWBK4g== X-Google-Smtp-Source: AOwi7QAz4QSbJBPP1N8ACfneMpvt2SaC5bsSkAB3HjFnfdFJFSNmX88xgJorZYjDCWf2mD3qxt8zOz6XvAW0z3Jmzqs= X-Received: by 10.55.212.203 with SMTP id s72mr2908354qks.318.1507133474746; Wed, 04 Oct 2017 09:11:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.21.47 with HTTP; Wed, 4 Oct 2017 09:11:13 -0700 (PDT) From: Gabriel Beddingfield Date: Wed, 4 Oct 2017 09:11:13 -0700 Message-ID: Subject: Extreme time jitter with suspend/resume cycles To: LKML , Stephen Boyd , Thomas Gleixner , John Stultz , Alessandro Zummo , Alexandre Belloni , linux-rtc@vger.kernel.org Cc: Guy Erb , hharte@nestlabs.com Sender: linux-rtc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rtc@vger.kernel.org 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" 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 Signed-off-by: Guy --- 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