User-Agent: Cyrus-JMAP/3.5.0-alpha0-4524-g5e5d2efdba-fm-20211214.001-g5e5d2efd
Mime-Version: 1.0
Message-Id: <83deaa79-39eb-4fd7-ad80-9d233fd6fdbb@www.fastmail.com>
In-Reply-To: <CALAqxLXf6TmOn_jCOv68oop=4On+CN-p_KkN-70BDt9OjQhzUw@mail.gmail.com>
References: <5af5d2a5-767c-d313-3be6-cb6f426f1980@sent.com>
 <b074f506-2568-4506-9557-4a9bc9cbea83@www.fastmail.com> <87wnkbuuuz.ffs@tglx>
 <4bb238e1-e8fa-44e6-9f5e-d047d1d4a892@www.fastmail.com> <8735mvthk6.ffs@tglx>
 <2ab24da8-e37d-426a-9500-b7541d21f8a3@www.fastmail.com>
 <CALAqxLXf6TmOn_jCOv68oop=4On+CN-p_KkN-70BDt9OjQhzUw@mail.gmail.com>
Date:   Wed, 15 Dec 2021 15:42:05 -0700
From:   "Joel Daniels" <jdaniels@sent.com>
To:     "John Stultz" <john.stultz@linaro.org>
Cc:     "Thomas Gleixner" <tglx@linutronix.de>,
        "Stephen Boyd" <sboyd@kernel.org>, linux-kernel@vger.kernel.org,
        "Alessandro Zummo" <a.zummo@towertech.it>,
        "Alexandre Belloni" <alexandre.belloni@bootlin.com>,
        linux-rtc@vger.kernel.org, x86@kernel.org
Subject: Re: Time keeping while suspended in the presence of persistent clock drift
Content-Type: text/plain
Precedence: bulk

Hi John,

Thanks for your feedback.

>>    [A] On machines with a persistent clock how is userspace supposed
>>        to be sure what drift to measure? Can it assume that the drift
>>        of the persistent clock used for sleep time injection is the
>>        same as the drift of /dev/rtc? This seems dangerous.
>
> Yea, there can be multiple RTCs as well.
>
>>    [B] Sleep time injection can come from the "persistent clock" or,
>>        if there is no persistent clock, from an RTC driver. I'd like
>>        to correct for drift from the perisistant clock but not touch
>>        the RTC driver sleep time injection mechanism. Is this
>>        acceptable or do people feel that any drift correction should
>>        work with both mechanisms in order to ensure a polished
>>        interface?
>
> This dual interface comes from the desire to support both the more
> atomic/earlier correction we can do w/ the persistent_clock interface
> while holding the timekeeping lock, while also supporting RTC devices
> that may sleep when being read, or may have dependencies that aren't
> ready that early in resume.
> 
> Admittedly having two separate abstractions here is a bit of a pain,
> and fixing just one side doesn't make it better.

Thanks; that makes sense to me. I suppose I ought to have a separate
sleep-time-injection drift correction parameter per RTC? That way the
kernel wouldn't do something silly if somebody hotplugs one RTC while
removing another. The persistent clock is almost always exposed as an
RTC as well, so either I could try to be very clever and make the
persistent clock share the drift correction parameter of its
corresponding RTC or I could just maintain a separate correction for
the persistent clock.

>>    [C] Some users may want to correct for drift during suspend-to-RAM
>>        but during suspend-to-disk they might boot into some other
>>        operating system which itself sets the CMOS RTC. Hopefully,
>>        this could be solved from userspace by changing the drift
>>        correction parameter to 0 just before a suspend-to-disk
>>        operation.
>
> Oof. This feels particularly complex and fragile to try to address.

Yes, I think we should ignore this issue and treat all suspend/resume
cycles identically. People who regularly dual-boot can just not use
the new feature.

> Personally, I'm not sure this warrants adding new userland interfaces
> for. I'd probably lean towards having the RTC framework internally
> measure and correct for drift, rather than adding an extra knob in
> userland.

Measuring RTC drift is hard. The standard PC RTC has only one second
resolution so you have to wait for the "edge" of a tick and measure
drift over an extended period of time. If you have some NTP daemon
slewing your system clock while you try to measure RTC drift then
you will get garbage. If your motherboard gets hot enough then your
RTC will run at a different rate while the machine is on than while
it is off.

I know of three programs that measure RTC drift today:

  # hwclock: you must use it to set the RTC twice, the second time
    with the "--update-drift" argument. The manual suggests waiting
    one day between calls. The drift and offset information is
    stored in /etc/adjtime. On boot "hwclock --hctosys" will use this
    to set the system clock correctly.

  # adjtimex (program not syscall) when run with the "--compare"
    option. It uses a least squares estimate from multiple samples
    which by default are each 10 seconds apart.

  # chrony with the "rtcfile" directive. It tracks the RTC over time
    to measure its offset and drift similarly to how it tracks the
    system clock drift. Tracking information is saved into
      /var/lib/chrony/rtc
    and can be used (via "chronyd -s") to set the system clock
    correctly on next boot.

Any method of measuring the drift is going to need to persist the
drift coefficient to disk so that it can set the system clock
correctly on boot. I think it would be best for the kernel to use this
same coefficient.

> Alternatively I'd go very simple and just put the correction factor in
> a boot argument.

This works for my use case though it won't be useful to a general
distro. Would you have one argument being used regardless of where the
sleep injection was coming from or would you try to tie it to the
persistent clock and/or a specific RTC?