Re: [PATCH 1/5] timekeeping: Allow tick_length changes to apply mid-tick

[David Woodhouse <dwmw2@infradead.org>]

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

On Wed, 2026-10-21 at 12:01 -0700, John Stultz wrote:
> On Thu, Oct 1, 2026 at 1:21 PM David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > When the NTP tick length changes, timekeeping_apply_adjustment() applies
> > the accounting for the new rate to ntp_error retrospectively for the
> > partial tick which is currently in progress. So the ideal line of the
> > clock conceptually changes from the moment of cycle_last (which can be
> > almost a full tick ago), and ntp_error grows to show the difference
> > between that and what userspace actually saw already.
> > 
> > This can lead to a significant value (10s of microseconds) landing in
> > ntp_error which can take *days* to drain through the natural mult ±1
> > dithering.
> > 
> > The slow-draining ntp_error then applies a consistent bias to that ±1
> > dithering, meaning that the precision of the effective frequency
> > requested by userspace is limited to integer 'mult' values, since the
> > dithering is no longer effective to achieve the fractional parts.
> > 
> > Fix this by allowing the rate change to be conceptually applied at
> > 'offset' into the partial tick, rather than the start of the tick.
> > This means that the amount of delta which gets accumulated into
> > ntp_error remains low.
> > 
> > Rather than factoring this into timekeeping_apply_adjustment() which
> > already makes my eyes bleed, pre-correct it when the rate changes, to
> > adjust the 'intended' clock position according to the delta between
> > old and new tick lengths (but leaving skew as it should be).
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Assisted-by: LLM
> > ---
> >  kernel/time/timekeeping.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index ea2e6e55f37b..e2f7e28dc16e 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -2444,6 +2444,8 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
> >                 /* Revert to the base mult rate. */
> >                 mult = tk->tkr_mono.mult - tk->ntp_err_mult;
> >         } else {
> > +               u64 old_ntp_tick = tk->ntp_tick;
> > +
> >                 tk->ntp_tick = ntp_tl;
> >                 tk->skew_delta = skew;
> >                 /*
> > @@ -2453,6 +2455,21 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
> >                 skew *= NTP_INTERVAL_FREQ;
> >                 mult = div64_u64((tk->ntp_tick + skew) >> tk->ntp_error_shift,
> >                                  tk->cycle_interval);
> > +
> > +               /*
> > +                * Rate adjustments are conceptually applied mid-tick in order
> > +                * to avoid accumulating large deltas in ntp_error which can
> > +                * take days to drain. Offset the ntp_error that will be
> > +                * introduced by timekeeping_apply_adjustment(), accordingly.
> > +                */
> > +               if (tk->ntp_tick != old_ntp_tick) {
> > +                       u64 old_dividend = (old_ntp_tick + skew) >> tk->ntp_error_shift;
> > +                       s64 old_base_mult = div64_u64(old_dividend, tk->cycle_interval);
> 
> Total nit, but splitting the math up this way makes it less clear to
> dim-old-me as to what you're calculating.
> Maybe instead:
> s64 old_mult_w_skew = div64_u64((old_ntp_tick + skew) >> tk->ntp_error_shift,
>                                 tk->cycle_interval);
> 
> Might more obviously visually match the logic above where mult is
> calculated (or alternatively you could alter the mult calcualtion to
> match what you have here)?
> 

In fact I already do the latter in patch 5 (as I need 'dividend' again
to calculate the ntp_err_frac divergence rate), so it ends up looking
like this, and does match:

→		dividend = (tk->ntp_tick + skew) >> tk->ntp_error_shift;
→		mult = div64_u64(dividend, tk->cycle_interval);
		/*
		 * Stash the fractional part of the per-cycle ideal mult that
		 * the integer @mult discards, scaled by 2^32, in clock-shifted
		 * ns per cycle.
		 */
		dividend -= (u64)mult * tk->cycle_interval;
		tk->ntp_err_frac = div64_u64(dividend << 32, tk->cycle_interval);

		/*
		 * Rate adjustments are conceptually applied mid-tick in order
		 * to avoid accumulating large deltas in ntp_error which can
		 * take days to drain. Offset the ntp_error that will be
		 * introduced by timekeeping_apply_adjustment(), accordingly.
		 */
		if (tk->ntp_tick != old_ntp_tick) {
→			u64 old_dividend = (old_ntp_tick + skew) >> tk->ntp_error_shift;
→			s64 old_base_mult = div64_u64(old_dividend, tk->cycle_interval);
			s64 mult_delta = (s64)mult - old_base_mult;

			tk->ntp_error -= ((s64)offset * mult_delta)
					 << tk->ntp_error_shift;
		}


I can happily pull the cosmetic part up to this patch, leaving those
first two highlighted lines exactly as they are for symmetry, before
patch 5 adds the extra bit in the middle.


> > +                       s64 mult_delta = (s64)mult - old_base_mult;
> 
> It might be useful to add a comment clarifying exactly what the
> mult_delta here represents vs the (mult - tk->tkr_mono.mult) value
> later passed to timekeeping_apply_adjustment?

Rather than "add a comment", I think that should actually be "rewrite
the existing four-line comment cited above that clearly isn't doing its
job".  How about this...

	/*
	 * In timekeeping_apply_adjustment(), the mult_adj value is
	 * retrospectively applied as if it had come into effect at
	 * the *start* of the current tick.  Any discrepancy due to
	 * the fact that time has *actually* been counted at the old
	 * mult rate between the start of the tick and now ('offset'
	 * cycles into that tick) is dumped into ntp_error.
	 *
	 * When tick_length is changed by a large amount, that can
	 * lead to tens of microseconds of ntp_error being accrued.
	 *
	 * Fix this by conceptually applying the tick_length change
	 * now (at 'offset'), rather than the start of the tick.
	 *
	 * That means calculating what 'mult' would have been with
	 * the old ntp_tick length (but the *current* value of skew),
	 * and pre-adjusting ntp_error to compensate.
	 */

It's more than four lines now, but if we want it fully stated in the
code rather than just the commit message, I don't think it could be
much smaller.

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