Re: [PATCH v4 2/7] timekeeping: Remove xtime_remainder from ntp_error accumulation

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]

On Mon, 2026-05-25 at 14:54 +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The ntp_error accumulator tracks the difference between the time actually
> reported to consumers in xtime, and the *intended* time. The former is
> subject to a sawtooth effect due to the quantisation of 'mult', which
> means that it actually advances by 'xtime_interval' each tick, while
> the intended clock advances by 'ntp_tick'.
> 
> By dithering between adjacent integer values of 'mult' which result in
> an 'xtime_interval' slightly higher/lower than the intended tick length,
> the advancement of xtime is kept on average to the intended rate.
> 
> The accounting should therefore adjust ntp_error by adding ntp_tick and
> subtracting xtime_interval on each tick.
> 
> Since commit a386b5af8edd ("time: Compensate for rounding on
> odd-frequency clocksources") the value subtracted has been
> (xtime_interval + xtime_remainder), which is wrong. The effect is a
> systematic drift whose magnitude depends on the value of xtime_remainder
> and the NTP frequency correction. NTP masks this by continuously
> adjusting the frequency to compensate, but with a fixed frequency (or an
> external reference clock like vmclock), the drift is exposed.
> 
> The value of xtime_remainder actually does represent the difference
> between the tick period and xtime_interval, so simply adding it instead
> of (+ tick length - xtime_remainder) might have made sense... except
> that it's only calculated once at boot time, so it's inaccurate anyway.
> So just kill it with fire.
> 
> Also remove it from the mult computation in timekeeping_adjust(), which
> used it to offset the division for the same (incorrect) reason.
> 
> Fixes: a386b5af8edd ("time: Compensate for rounding on odd-frequency clocksources")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Assisted-by: Kiro:claude-opus-4.6-1m
> ---

Sashiko asks, "Does this removal cause tk->ntp_error to diverge towards
negative infinity when NTP is inactive and cycle_interval rounds up?"

And Sashiko is right; it *does*. But it's also *correct* to do so.

Because tk_setup_internals() currently rounds 'mult' to the *nearest*
value, instead of rounding *down* like timekeeping_adjust() does.

So the dithering that timekeeping_adjust() does between mult and mult+1
can't actually achieve the desired result, and we *do* just drift
further and further away from where we should be, because 'mult' is one
too many than it should be. The values chosen for mult/mult+1 are
supposed to be either *side* of the desired average period.

Any adjtimex() call which sets the frequency will fix this, because
mult *is* correctly calculated in timekeeping_adjust().

I'll fix that pre-existing bug in a separate patch, in v5. Something
like this in tk_setup_internals() should do it:

-	/* Go back from cycles -> shifted ns */
-	tk->xtime_interval = interval * clock->mult;
-	tk->raw_interval = interval * clock->mult;

-	tk->tkr_mono.mult = clock->mult;
+	tk->tkr_mono.mult = div64_u64(ntpinterval, tk-
>cycle_interval);

+	/* Go back from cycles -> shifted ns */
+	tk->xtime_interval = interval * tk->tkr_mono.mult;
+	tk->raw_interval = interval * tk->tkr_raw.mult;



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]