Messages in this thread | | | From | Gabriel Beddingfield <> | Date | Thu, 5 Oct 2017 09:47:42 -0700 | Subject | Re: Extreme time jitter with suspend/resume cycles |
| |
Hi Thomas,
On Thu, Oct 5, 2017 at 4:01 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > The SoC we use backs the monotonic clock (sched_clock_register()) with a > > Again. monotonic clock != sched clock. The clocksource which feeds the > monotonic timekeeper clock is registered via clocksource_register() & al.
Sorry. I was hedging since I wasn't sure *which* term to use.
> Groan. Engineering based on theories is doomed to begin with.
I knew you'd say that. :-p
> Your 36 hour time jump is probably exactly 36.4089 hours as that's > > ((1 << 32) / 32768) / 3600 > > i.e. the 32bit rollover of the clocksource. So, if the clocksource->read() > function returns a full 64bit counter value, then it must have protection > against observing the rollover independent of the clock which feeds that > counter. Of course the frequency changes the probablity of observing it, > but still the read function must be protected against observing the > rollover unconditionally.
Right, but isn't this what clocksource->mask is supposed to do? When we change the back-end frequency, we're still using the same front-end 32-bit register and we don't see the same jumps.
> Which SoC/clocksource driver are you talking about?
NXP i.MX 6SoloX drivers/clocksource/timer-imx-gpt.c drivers/rtc/rtc-snvs.c arch/arm/boot/dts/imx6sx.dtsi (included from imx6sx-sdb.dts) (node: soc/aips1/gpt)
We patched the RTC driver to call register_persistent_clock() (arch/arm/kernel/time.c)
The upstream driver doesn't support using the 32kHz clock, but the silicone does... so we modified the driver to accept it and set the "NONSTOP" flag.
> I can understand that. Though, using that value for injecting accurate > sleep time should just work with the existing code no matter how long the > actual sleep time was. The timekeeping core takes the nsec part of the > timespec value retrieved via read_persistent_clock64() into account. > > I still have a hard time to figure out what you are trying to achieve.
I'm trying to disable this block:
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);
//...
#ifdef CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION if (persistent_clock_exists) { /* * To avoid drift caused by repeated suspend/resumes, * which each can add ~1 second drift error, * try to compensate so the difference in system time * and persistent_clock time stays close to constant. */ delta = timespec64_sub(tk_xtime(tk), timekeeping_suspend_time); delta_delta = timespec64_sub(delta, old_delta); if (abs(delta_delta.tv_sec) >= 2) { /* * if delta_delta is too large, assume time correction * has occurred and set old_delta to the current delta. */ old_delta = delta; } else { /* Otherwise try to adjust old_system to compensate */ timekeeping_suspend_time =
timespec64_add(timekeeping_suspend_time, delta_delta); } } #endif
In the typical case, it is trying to maintain the assumption that (ideally) 'delta' is a constant. It tries to maintain this assumption by fiddling with the "suspend time" -- adding or subtracting a little to it.
However, if NTP has adjusted the system time but *not* adjusted the persistent clock's time... then the assumption is violated, 'delta' is *not* constant, and this block cancels out the NTP corrections.
-gabe
| |