[PATCH 0/5] timekeeping: Reduce magnitude of ntp_error

[PATCH 0/5] timekeeping: Reduce magnitude of ntp_error

[David Woodhouse]

The timekeeper's ntp_error variable tracks the delta between the ideal
time that the kernel *should* be reporting as CLOCK_REALTIME, and the
time that is *actually* being reported.

A previous patch series in v7.3, up to commit 794ddd6e15cf ("ntp:
Remove tick_length_base, use tick_length directly"), fixed *errors* in
the accounting of ntp_error — cases where the reported time would
diverge from the ideal time *without* the divergence being correctly
tracked in ntp_error for subsequent compensation.

This series is the next step: it reduces the magnitude of the
excursions which are (now) *correctly* tracked in ntp_error.

The ntp_error mechanism works fine when it remains close to zero. The
actual clock is steered back towards the ideal by a single ±1
dithering of the per-cycle 'mult' period, changing between high and
low each time ntp_error crosses zero.

This doesn't work as well when the value of ntp_error gets higher. In
some cases, 83µs of ntp_error was seen. Draining that through the mult
±1 dithering, at typically less than 1ns/s, could take days. And while
it does, the precision of the frequency that can be effected by the
kernel's timekeeping is limited by the integer quantisation, because
it's permanently biased to either mult or mult+1 according to the
direction of the persistent ntp_error, and frequencies between the two
values cannot be achieved by the normal dithering. On top of which,
chrony was attempting to discipline the *reported* clock, even as
the kernel was attempting to steer it as hard as it could (albeit
not very hard) back to the kernel's ideal.

There are two cases where during frequency/phase adjustment, the
change was effectively applied to the 'ideal' vs 'actual' rate at
different moments in time. This accounting for this in ntp_error was
self-consistent, but the better answer is for them to be conceptually
applied at the same moment, without contributing anything to ntp_error
at all.

While the timekeeping code is applying a phase correction, that
applies an additional bias to the per-cycle 'mult' values. If a
tickless kernel goes idle while this skew is being applied, it can
continue to apply that bias *long* after it was intended to finish,
overshooting the intended phase correction (by 500µs/s in the case of
adjtime). Fix this in timekeeping_max_deferment() by requesting a
wakeup at the top of the next second while phase skew is being
applied. (NB: Only for the core timekeeper, not aux clocks which
remain as they were).

Finally, if ntp_error ever does get excessive, we can deliberately
apply a phase skew to return it to zero faster than the normal
dithering would permit. An equivalent mechanism was removed in commit
dc491596f639 ("timekeeping: Rework frequency adjustments to work
better w/ nohz") but especially with the timekeeping_max_deferment()
change mentioned above, this now works effectively even with (and
in fact especially with) tickless idle. But again only for the core
timekeeper... unless we want to wake for aux clocks too?

Tests running at https://david.woodhou.se/ntptest-virt/ on baseline
vs. patched kernels, both tickless and periodic. The effect is seen
most clearly on the tickless kernel, where the baseline experiences
much higher jitter around corrections that should be smooth.

Phase 1 was *correcting* the accounting of ntp_error. Phase 2 is
*reducing* the amount of ntp_error. Tacked on at the end, marked
DO NOT MERGE for now, is phase 3: *correcting* the output of
ktime_get_snapshot_id() for ntp_error at the moment of the snapshot.

This warrants further discussion, which is why it's being held back
for now. The logic is that snapshot users want *accurate* time, and
they don't want to be given a time that we *know* is microseconds
wrong just because we happened to be in a tickless kernel, running
idle for a large period accumulating divergence in ntp_error right
before they asked for their snapshot.

I vacillated about putting the correction as a separate field in the
snapshot for callers to apply only if they want it, but I think it
makes more sense just to return the corrected time in *all* cases,
unconditionally. Nobody *wants* inaccurate time. Except userspace,
where considerations about smoothness and monotonicity override.

David Woodhouse (5):
  timekeeping: Allow tick_length changes to apply mid-tick
  ntp: Recalculate skew_delta when the phase offset changes
  timekeeping: Bound idle sleep while a phase slew is in flight
  timekeeping: Reinstate proportional correction of ntp_error
  timekeeping: Apply extrapolated ntp_error to clock snapshots

 include/linux/timekeeper_internal.h |  10 +-
 kernel/time/ntp.c                   | 144 +++++++++++++++----------
 kernel/time/timekeeping.c           | 162 ++++++++++++++++++++++++++--
 3 files changed, 249 insertions(+), 67 deletions(-)

-- 
2.43.0

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

[David Woodhouse]

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);
+			s64 mult_delta = (s64)mult - old_base_mult;
+
+			tk->ntp_error -= ((s64)offset * mult_delta)
+					 << tk->ntp_error_shift;
+		}
 	}
 
 	/*
-- 
2.43.0

[PATCH 2/5] ntp: Recalculate skew_delta when the phase offset changes

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The skew rate is recomputed only by second_overflow(), so when
time_offset or time_adjust changes mid-second (hardpps on each pulse,
adjtimex, adjtime()) the stale rate keeps delivering until the end of
the second, and the misdelivery lands in ntp_error. Pull the
computation out into ntp_update_skew_delta() and call it from the
writers. Settling of an opposing time_offset/time_adjust overlap stays
paced at one chunk per second, via the settle argument.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: LLM
---
 kernel/time/ntp.c | 144 +++++++++++++++++++++++++++-------------------
 1 file changed, 85 insertions(+), 59 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index d22b532ec536..b8b0bf1d94e7 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -593,6 +593,82 @@ ktime_t ntp_get_next_leap(unsigned int tkid)
 	return KTIME_MAX;
 }
 
+/*
+ * ntp_update_skew_delta - Recompute the per-tick skew rate for the
+ * current second from the pending time_offset / time_adjust phase.
+ *
+ * The rate is in the same units as time_offset: (ns << NTP_SCALE_SHIFT)
+ * / HZ. If the result is so low that the skew imparted would round to
+ * zero, pass the bare minimum +-1 to ensure that it *does* actually
+ * drain completely to zero. It won't overshoot because
+ * logarithmic_accumulation() only drains what it can from time_offset
+ * or time_adjust, and the rest ends up in ntp_error which drives the
+ * selection of 'mult' immediately each tick.
+ *
+ * Called from second_overflow() at each second boundary (@settle =
+ * true), and directly by the writers of time_offset / time_adjust so a
+ * change takes effect at the next timekeeping advance rather than
+ * persisting a stale rate until the end of the second. @settle gates
+ * ntp_transfer_offset_adjust(): settling the opposing overlap is paced
+ * at one chunk per second by design and is the only mutation here, so
+ * with @settle false the call is a pure recompute, safe at any rate.
+ *
+ * The timekeeper lock is held on all paths, serializing ntp_data.
+ */
+static void ntp_update_skew_delta(struct ntp_data *ntpdata, bool settle)
+{
+	if (ntpdata->time_offset || ntpdata->time_adjust ||
+	    ntpdata->time_adjust_frac) {
+		s64 off_chunk = ntp_offset_chunk(ntpdata, ntpdata->time_offset);
+		s64 adj_chunk = 0, net;
+
+		/*
+		 * Once the exponential chunk rounds to zero, deliver the last
+		 * remaining offset this second so it converges to zero instead
+		 * of stalling just above it.
+		 */
+		if (!off_chunk)
+			off_chunk = ntpdata->time_offset;
+
+		if (ntpdata->time_adjust || ntpdata->time_adjust_frac) {
+			s64 adj;
+
+			if (ntpdata->time_adjust >= MAX_TICKADJ)
+				adj = MAX_TICKADJ * ONE_US_NS;
+			else if (ntpdata->time_adjust <= -MAX_TICKADJ)
+				adj = -MAX_TICKADJ * ONE_US_NS;
+			else
+				adj = ntpdata->time_adjust * ONE_US_NS +
+					ntpdata->time_adjust_frac;
+
+			adj_chunk = div_s64(adj, NTP_INTERVAL_FREQ);
+			if (!adj_chunk)
+				adj_chunk = signof(ntpdata->time_adjust_frac);
+		}
+
+		/*
+		 * If the two slews oppose, only their net would drive the
+		 * per-tick drain, so the cancelling part would never drain from
+		 * either tracker and an exact cancellation would stall both.
+		 * Settle that overlap directly between them (no clock motion).
+		 */
+		if (settle && off_chunk && adj_chunk &&
+		    signof(off_chunk) != signof(adj_chunk)) {
+			s64 conflict = min(abs(off_chunk), abs(adj_chunk));
+
+			ntp_transfer_offset_adjust(ntpdata, signof(off_chunk) * conflict);
+		}
+
+		/* Net is what the clock delivers; reduce to per-tick, then floor. */
+		net = off_chunk + adj_chunk;
+		ntpdata->skew_delta = div_s64(net, NTP_INTERVAL_FREQ);
+		if (!ntpdata->skew_delta && net)
+			ntpdata->skew_delta = signof(net);
+	} else {
+		ntpdata->skew_delta = 0;
+	}
+}
+
 /*
  * This routine handles the overflow of the microsecond field
  *
@@ -669,65 +745,8 @@ int second_overflow(unsigned int tkid, time64_t secs)
 	/* Check PPS signal */
 	pps_dec_valid(ntpdata);
 
-	/*
-	 * Set the per-tick skew rate for the next second. This is in
-	 * the same units as time_offset: (ns << NTP_SCALE_SHIFT) / HZ.
-	 * If the result is so low that the skew imparted would round
-	 * to zero, pass the bare minimum ±1 to ensure that it *does*
-	 * actually drain completely to zero. It won't overshoot because
-	 * logarithmic_accumulation() only drains what it can from
-	 * time_offset or time_adjust, and the rest ends up in ntp_error
-	 * which drives the selection of 'mult' immediately each tick.
-	 */
-	if (ntpdata->time_offset || ntpdata->time_adjust ||
-	    ntpdata->time_adjust_frac) {
-		s64 off_chunk = ntp_offset_chunk(ntpdata, ntpdata->time_offset);
-		s64 adj_chunk = 0, net;
-
-		/*
-		 * Once the exponential chunk rounds to zero, deliver the last
-		 * remaining offset this second so it converges to zero instead
-		 * of stalling just above it.
-		 */
-		if (!off_chunk)
-			off_chunk = ntpdata->time_offset;
-
-		if (ntpdata->time_adjust || ntpdata->time_adjust_frac) {
-			s64 adj;
-
-			if (ntpdata->time_adjust >= MAX_TICKADJ)
-				adj = MAX_TICKADJ * ONE_US_NS;
-			else if (ntpdata->time_adjust <= -MAX_TICKADJ)
-				adj = -MAX_TICKADJ * ONE_US_NS;
-			else
-				adj = ntpdata->time_adjust * ONE_US_NS +
-					ntpdata->time_adjust_frac;
-
-			adj_chunk = div_s64(adj, NTP_INTERVAL_FREQ);
-			if (!adj_chunk)
-				adj_chunk = signof(ntpdata->time_adjust_frac);
-		}
-
-		/*
-		 * If the two slews oppose, only their net would drive the
-		 * per-tick drain, so the cancelling part would never drain from
-		 * either tracker and an exact cancellation would stall both.
-		 * Settle that overlap directly between them (no clock motion).
-		 */
-		if (off_chunk && adj_chunk && signof(off_chunk) != signof(adj_chunk)) {
-			s64 conflict = min(abs(off_chunk), abs(adj_chunk));
-
-			ntp_transfer_offset_adjust(ntpdata, signof(off_chunk) * conflict);
-		}
-
-		/* Net is what the clock delivers; reduce to per-tick, then floor. */
-		net = off_chunk + adj_chunk;
-		ntpdata->skew_delta = div_s64(net, NTP_INTERVAL_FREQ);
-		if (!ntpdata->skew_delta && net)
-			ntpdata->skew_delta = signof(net);
-	} else {
-		ntpdata->skew_delta = 0;
-	}
+	/* Set the per-tick skew rate for the next second */
+	ntp_update_skew_delta(ntpdata, true);
 
 	return leap;
 }
@@ -1003,6 +1022,10 @@ static inline void process_adjtimex_modes(struct ntp_data *ntpdata, const struct
 
 	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
 		ntp_update_frequency(ntpdata);
+
+	/* A changed phase target invalidates the current skew rate */
+	if (txc->modes & (ADJ_OFFSET | ADJ_TIMECONST | ADJ_STATUS))
+		ntp_update_skew_delta(ntpdata, false);
 }
 
 /*
@@ -1023,6 +1046,7 @@ int ntp_adjtimex(unsigned int tkid, struct __kernel_timex *txc, const struct tim
 			ntpdata->time_adjust = txc->offset;
 			ntpdata->time_adjust_frac = 0;
 			ntp_update_frequency(ntpdata);
+			ntp_update_skew_delta(ntpdata, false);
 
 			audit_ntp_set_old(ad, AUDIT_NTP_ADJUST,	save_adjust);
 			audit_ntp_set_new(ad, AUDIT_NTP_ADJUST,	ntpdata->time_adjust);
@@ -1264,6 +1288,8 @@ static void hardpps_update_phase(struct ntp_data *ntpdata, long error)
 		/* Cancel running adjtime() */
 		ntpdata->time_adjust = 0;
 		ntpdata->time_adjust_frac = 0;
+		/* The previous correction's skew rate is stale; recompute */
+		ntp_update_skew_delta(ntpdata, false);
 	}
 	/* Update jitter */
 	ntpdata->pps_jitter += (jitter - ntpdata->pps_jitter) >> PPS_INTMIN;
-- 
2.43.0

[PATCH 3/5] timekeeping: Bound idle sleep while a phase slew is in flight

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Phase offset for adjtime() can be delivered at a rate of 500µs/s, and
for STA_PPSTIME potentially even more. The skew is recalculated in
second_overflow() each second, and if that's missed in a tickless
kernel, the skew continues to be applied for the whole of the idle
period. While skew is active, clamp timekeeping_max_deferment() to the
top of the current second to avoid the overshoot.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: LLM
---
 kernel/time/timekeeping.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e2f7e28dc16e..986649cae46d 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1975,6 +1975,21 @@ u64 timekeeping_max_deferment(void)
 
 		ret = tk->tkr_mono.clock->max_idle_ns;
 
+		/*
+		 * Skew introduced for phase offset is intended to be
+		 * recalculated each second. Ensure a tickless kernel
+		 * wakes up for that and does not overshoot, while skew
+		 * is being applied.
+		 */
+		if (abs(tk->skew_delta) > 1) {
+			u64 nsec_in_sec = tk->tkr_mono.xtime_nsec >>
+					  tk->tkr_mono.shift;
+			u64 remaining = NSEC_PER_SEC - min_t(u64, nsec_in_sec,
+							     NSEC_PER_SEC);
+
+			ret = min_t(u64, ret, max_t(u64, remaining, TICK_NSEC));
+		}
+
 	} while (read_seqcount_retry(&tk_core.seq, seq));
 
 	return ret;
@@ -3045,8 +3060,16 @@ static int __do_adjtimex(struct tk_data *tkd, struct __kernel_timex *txc,
 		tk_update_leap_state_all(tkd);
 	}
 
-	/* Update the multiplier immediately if frequency was set directly */
-	if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
+	/*
+	 * Update the multiplier immediately if the frequency was set
+	 * directly, or refresh the cached skew_delta if a phase
+	 * adjustment started a slew, so that it bounds idle sleep.
+	 * ADJ_OFFSET_READONLY excludes the unprivileged read-only
+	 * adjtime() form, which starts no slew.
+	 */
+	if ((txc->modes & (ADJ_FREQUENCY | ADJ_TICK)) ||
+	    ((txc->modes & ADJ_OFFSET) &&
+	     !(txc->modes & ADJ_OFFSET_READONLY)))
 		result->clock_set |= __timekeeping_advance(tkd, TK_ADV_FREQ);
 
 	return ret;
-- 
2.43.0

[PATCH 4/5] timekeeping: Reinstate proportional correction of ntp_error

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The ±1 mult dithering is sufficient to keep ntp_error at zero once
it's already there, but it doesn't do much to reduce it once any
significant ntp_error has accumulated (e.g. from frequency or phase
adjustments) — it can typically only drain single-digit nanoseconds
per second. Commit dc491596f639 ("timekeeping: Rework frequency
adjustments to work better w/ nohz") removed a larger skew because in
a tickless kernel, it would remain in effect for a full idle period
and overshoot. Now that timekeeping_max_deferment() ensures that the
system wakes at the top of the second when skew is active, the
correction can be reinstated. If ntp_error exceeds an amount that a
single ±1 change to mult can drain within a minute, add an
additional bias to mult to close the gap.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: LLM
---
 include/linux/timekeeper_internal.h |  5 ++-
 kernel/time/timekeeping.c           | 55 ++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
index fe077d97b5f8..85d123bcb270 100644
--- a/include/linux/timekeeper_internal.h
+++ b/include/linux/timekeeper_internal.h
@@ -97,6 +97,8 @@ struct tk_read_base {
  * @ntp_error_shift:		Shift conversion between clock shifted nano seconds and
  *				ntp shifted nano seconds.
  * @ntp_err_mult:		Multiplication factor for scaled math conversion
+ * @err_drain:			Extra mult bias repaying ntp_error beyond the
+ *				dither's reach; sized at second boundaries
  * @cs_tick_adj:		Per-second adjustment handed to NTP via ntp_clear()
  *				accounting for the difference between the nominal
  *				NTP interval and the real time taken by the
@@ -186,7 +188,8 @@ struct timekeeper {
 	u64			ntp_tick;
 	s64			ntp_error;
 	u32			ntp_error_shift;
-	u32			ntp_err_mult;
+	s32			ntp_err_mult;
+	s32			err_drain;
 	s64			cs_tick_adj;
 	u32			skip_second_overflow;
 	s64			skew_delta;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 986649cae46d..48d916a6c433 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -325,6 +325,15 @@ static inline void clocksource_disable_inline_read(void) { }
 static inline void clocksource_enable_inline_read(void) { }
 #endif
 
+/*
+ * The proportional ntp_error drain engages when the error exceeds
+ * TK_NTP_ERR_THRESH seconds' worth of the ±1 dither's delivery (so
+ * noise the dither handles alone never wakes the CPU), and then
+ * repays over ~TK_NTP_ERR_HORIZON seconds.
+ */
+#define TK_NTP_ERR_THRESH	60
+#define TK_NTP_ERR_HORIZON	10
+
 /**
  * tk_setup_internals - Set up internals to use clocksource clock.
  *
@@ -422,6 +431,7 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
 	tk->tkr_mono.mult = clock->mult;
 	tk->tkr_raw.mult = clock->mult;
 	tk->ntp_err_mult = 0;
+	tk->err_drain = 0;
 	tk->skip_second_overflow = 0;
 	tk->skew_delta = 0;
 
@@ -830,6 +840,7 @@ static void timekeeping_update_from_shadow(struct tk_data *tkd, unsigned int act
 
 	if (action & TK_CLEAR_NTP) {
 		tk->ntp_error = 0;
+		tk->err_drain = 0;
 		ntp_clear(tk->id, tk->cs_tick_adj);
 	}
 
@@ -1977,11 +1988,11 @@ u64 timekeeping_max_deferment(void)
 
 		/*
 		 * Skew introduced for phase offset is intended to be
-		 * recalculated each second. Ensure a tickless kernel
-		 * wakes up for that and does not overshoot, while skew
-		 * is being applied.
+		 * recalculated each second, as is the proportional
+		 * ntp_error drain. Ensure a tickless kernel wakes up
+		 * for that and does not overshoot.
 		 */
-		if (abs(tk->skew_delta) > 1) {
+		if (abs(tk->skew_delta) > 1 || (u32)tk->ntp_err_mult > 1) {
 			u64 nsec_in_sec = tk->tkr_mono.xtime_nsec >>
 					  tk->tkr_mono.shift;
 			u64 remaining = NSEC_PER_SEC - min_t(u64, nsec_in_sec,
@@ -2251,6 +2262,7 @@ void timekeeping_resume(void)
 	tks->tkr_raw.cycle_last  = cycle_now;
 
 	tks->ntp_error = 0;
+	tks->err_drain = 0;
 	timekeeping_suspended = 0;
 	timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET);
 	raw_spin_unlock_irqrestore(&tk_core.lock, flags);
@@ -2493,7 +2505,16 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
 	 * tick division, the clock will slow down. Otherwise it will stay
 	 * ahead until the tick length changes to a non-divisible value.
 	 */
-	tk->ntp_err_mult = tk->ntp_error > 0 ? 1 : 0;
+	/* Drop the ntp_error drain the moment it overshoots */
+	if (tk->err_drain &&
+	    (((s64)tk->err_drain ^ tk->ntp_error) < 0 || !tk->ntp_error))
+		tk->err_drain = 0;
+
+	if (tk->err_drain)
+		tk->ntp_err_mult = tk->err_drain;
+	else
+		tk->ntp_err_mult = tk->ntp_error > 0 ? 1 : 0;
+
 	mult += tk->ntp_err_mult;
 
 	timekeeping_apply_adjustment(tk, offset, mult - tk->tkr_mono.mult);
@@ -2536,6 +2557,7 @@ static inline unsigned int accumulate_nsecs_to_secs(struct timekeeper *tk)
 {
 	u64 nsecps = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;
 	unsigned int clock_set = 0;
+	bool crossed = false;
 
 	while (tk->tkr_mono.xtime_nsec >= nsecps) {
 		int leap;
@@ -2568,6 +2590,29 @@ static inline unsigned int accumulate_nsecs_to_secs(struct timekeeper *tk)
 
 			clock_set = TK_CLOCK_WAS_SET;
 		}
+
+		crossed = true;
+	}
+
+	/*
+	 * Core timekeeper only; aux timekeepers are not covered by the
+	 * timekeeping_max_deferment() wakeup which re-sizes the drain
+	 * each second before it can overshoot.
+	 */
+	if (crossed && tk->id == TIMEKEEPER_CORE) {
+		/* Re-size the proportional ntp_error drain for this second */
+		s64 lsb_sec = ((s64)tk->cycle_interval <<
+			       tk->ntp_error_shift) * NTP_INTERVAL_FREQ;
+
+		tk->err_drain = 0;
+		if (abs(tk->ntp_error) > lsb_sec * TK_NTP_ERR_THRESH) {
+			s32 headroom = tk->tkr_mono.clock->maxadj / 4;
+
+			tk->err_drain = clamp_t(s64,
+						div64_s64(tk->ntp_error,
+							  lsb_sec * TK_NTP_ERR_HORIZON),
+						-headroom, headroom);
+		}
 	}
 	return clock_set;
 }
-- 
2.43.0

[PATCH 5/5] [DO NOT MERGE] timekeeping: Apply extrapolated ntp_error to clock snapshots

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The time reported in ::systime of a system_time_snapshot is known to be
slightly inaccurate because of the way that the reported realtime clock
sawtooths around the *intended* time series, limited by the integer mult
value used to calculate the inter-tick times, and designed to ensure
smoothness and monotonicity for its consumers.

It is particularly inaccurate in a tickless kernel, where ntp_err_mult
is not adjusted on each tick, allowing the reported clock to diverge
from the intended time for a large number of ticks before re-converging.

This appears to be the reason why CONFIG_NTP_PPS is not enabled on
tickless kernels — because at that scale of precision, the realtime
snapshot at the time of the pulse bears little relation to the time the
kernel *actually* believes it to be, thus introducing random errors into
the PPS phase correction.

It would be better for callers of get_device_system_crosststamp() and
ktime_get_snapshot_id() to receive the *accurate* time, not the
sanitized version provided to gettimeofday().

Compute the deviation in snapshot_ntp_error() and add it to the returned
::systime so the snapshot lands on the ideal line. It sums four terms in
ns << NTP_SCALE_SHIFT before converting to signed ns:

  - tk->ntp_error, the deviation as of the last update;
  - (cycle_delta * ntp_err_frac), the fractional-mult drift accrued
    since then (cycle_delta is at most a tick on a tickful kernel, but
    many ticks' worth under NO_HZ);
  - (cycle_delta * ntp_err_mult), subtracting the applied +1 mult dither
    over the same span;
  - the sub-nanosecond fraction dropped when the read was truncated to
    whole ns (low shift bits, exact despite the multiply overflowing).

The helper uses the timekeeper selected for the requested clock id, so
all NTP-disciplined clocks are corrected, including the AUX clocks (each
has its own NTP instance); only CLOCK_MONOTONIC_RAW is undisciplined and
gets no correction. The residual is then a single clocksource cycle, the
same bound as a tickful kernel.

Note that this *unconditionally* changes the ::systime returned by all
snapshot and cross timestamp consumers (PTP SYS_OFFSET_PRECISE/EXTENDED,
etc.): it is now the ideal NTP-disciplined time rather than the raw
accumulated clock.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: LLM
---
 include/linux/timekeeper_internal.h |  5 ++
 kernel/time/timekeeping.c           | 71 +++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
index 85d123bcb270..1f69479dd846 100644
--- a/include/linux/timekeeper_internal.h
+++ b/include/linux/timekeeper_internal.h
@@ -99,6 +99,10 @@ struct tk_read_base {
  * @ntp_err_mult:		Multiplication factor for scaled math conversion
  * @err_drain:			Extra mult bias repaying ntp_error beyond the
  *				dither's reach; sized at second boundaries
+ * @ntp_err_frac:		Fractional part of the per-cycle NTP-ideal mult that the
+ *				integer @mult truncates, as a fraction of 2^32 in
+ *				clock-shifted nanoseconds per cycle. Describes the
+ *				working point of the +-1 mult dither.
  * @cs_tick_adj:		Per-second adjustment handed to NTP via ntp_clear()
  *				accounting for the difference between the nominal
  *				NTP interval and the real time taken by the
@@ -190,6 +194,7 @@ struct timekeeper {
 	u32			ntp_error_shift;
 	s32			ntp_err_mult;
 	s32			err_drain;
+	u64			ntp_err_frac;
 	s64			cs_tick_adj;
 	u32			skip_second_overflow;
 	s64			skew_delta;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 48d916a6c433..83c3e5ef9c5f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -431,6 +431,7 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
 	tk->tkr_mono.mult = clock->mult;
 	tk->tkr_raw.mult = clock->mult;
 	tk->ntp_err_mult = 0;
+	tk->ntp_err_frac = 0;
 	tk->err_drain = 0;
 	tk->skip_second_overflow = 0;
 	tk->skew_delta = 0;
@@ -1241,6 +1242,55 @@ static inline u64 tk_clock_read_snapshot(const struct tk_read_base *tkr,
 	return clock->read(clock);
 }
 
+/*
+ * snapshot_ntp_error - record how far a snapshot's ::systime is from the
+ * ideal NTP-disciplined time at @now, in signed nanoseconds, so a caller
+ * can land exactly on the ideal line by adding it to ::systime.
+ *
+ * The value is summed in ns << NTP_SCALE_SHIFT from four parts:
+ *
+ *  - tk->ntp_error, the deviation accumulated as of the last timekeeping
+ *    update (tkr_mono.cycle_last);
+ *  - (cycle_delta * ntp_err_frac), the fractional-mult drift accrued over
+ *    the cycles read since then -- at most a tick on a tickful kernel, but
+ *    potentially many ticks' worth under NO_HZ;
+ *  - (cycle_delta * ntp_err_mult), subtracting the applied +1 mult dither
+ *    over the same span;
+ *  - the sub-nanosecond fraction that ::systime dropped when the read was
+ *    truncated to whole ns (the low @shift bits, exact even though the
+ *    multiply overflows).
+ *
+ * CLOCK_MONOTONIC_RAW is not NTP-disciplined and carries no error. Every
+ * other clock id uses its own timekeeper @tk -- including the AUX clocks,
+ * which each have their own NTP instance.
+ */
+static s64 snapshot_ntp_error(const struct timekeeper *tk, clockid_t clock_id,
+			      u64 now)
+{
+	u64 cycle_delta;
+	u32 nes;
+	s64 tmp, err;
+
+	if (clock_id == CLOCK_MONOTONIC_RAW)
+		return 0;
+
+	cycle_delta = (now - tk->tkr_mono.cycle_last) & tk->tkr_mono.mask;
+	nes = tk->ntp_error_shift;
+
+	/* Insane deltas from marginal clocksources: skip the extrapolation */
+	if (unlikely(cycle_delta > tk->tkr_mono.clock->max_cycles))
+		return tk->ntp_error >> NTP_SCALE_SHIFT;
+
+	err = tk->ntp_error;
+	err += ((s64)mul_u64_u64_shr(cycle_delta, tk->ntp_err_frac, 32) -
+		(s64)cycle_delta * tk->ntp_err_mult) << nes;
+
+	tmp = (s64)(cycle_delta * tk->tkr_mono.mult + tk->tkr_mono.xtime_nsec);
+	tmp &= (1ULL << tk->tkr_mono.shift) - 1;
+	err += tmp << nes;
+
+	return (err + (1LL << (NTP_SCALE_SHIFT - 1))) >> NTP_SCALE_SHIFT;
+}
 
 /**
  * ktime_get_snapshot_id -  Simultaneously snapshot a given clock ID with
@@ -1264,6 +1314,7 @@ void ktime_get_snapshot_id(clockid_t clock_id, struct system_time_snapshot *syst
 {
 	ktime_t base_raw, base_sys, offs_sys, *offs, offs_zero = 0;
 	u64 nsec_raw, nsec_sys, now;
+	s64 ntp_error;
 	struct timekeeper *tk;
 	struct tk_data *tkd;
 	unsigned int seq;
@@ -1326,10 +1377,12 @@ void ktime_get_snapshot_id(clockid_t clock_id, struct system_time_snapshot *syst
 
 		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
 		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
+
+		ntp_error = snapshot_ntp_error(tk, clock_id, now);
 	} while (read_seqcount_retry(&tkd->seq, seq));
 
 	systime_snapshot->cycles = now;
-	systime_snapshot->systime = ktime_add_ns(base_sys, offs_sys + nsec_sys);
+	systime_snapshot->systime = ktime_add_ns(base_sys, offs_sys + nsec_sys) + ntp_error;
 	systime_snapshot->monoraw = ktime_add_ns(base_raw, nsec_raw);
 
 	/*
@@ -1581,6 +1634,7 @@ int get_device_system_crosststamp(int (*get_time_fn)
 	ktime_t base_sys, base_raw, *offs;
 	u32 clock_was_set_seq = 0;
 	u64 nsec_sys, nsec_raw;
+	s64 ntp_error;
 	u8 cs_was_changed_seq;
 	unsigned int seq;
 	bool do_interp;
@@ -1647,9 +1701,10 @@ int get_device_system_crosststamp(int (*get_time_fn)
 
 		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, cycles);
 		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, cycles);
+		ntp_error = snapshot_ntp_error(tk, xtstamp->clock_id, cycles);
 	} while (read_seqcount_retry(&tkd->seq, seq));
 
-	xtstamp->sys_systime = ktime_add_ns(base_sys, nsec_sys);
+	xtstamp->sys_systime = ktime_add_ns(base_sys, nsec_sys) + ntp_error;
 	xtstamp->sys_monoraw = ktime_add_ns(base_raw, nsec_raw);
 
 	/*
@@ -2460,6 +2515,7 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
 {
 	u64 ntp_tl = ntp_tick_length(tk->id);
 	s64 skew = ntp_get_skew_delta(tk->id);
+	u64 dividend;
 	u32 mult;
 
 	/*
@@ -2480,8 +2536,15 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
 		 * scale it back up to the full per-tick rate for the mult bias.
 		 */
 		skew *= NTP_INTERVAL_FREQ;
-		mult = div64_u64((tk->ntp_tick + skew) >> tk->ntp_error_shift,
-				 tk->cycle_interval);
+		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
-- 
2.43.0

Re: [PATCH 5/5] [DO NOT MERGE] timekeeping: Apply extrapolated ntp_error to clock snapshots

[Rodolfo Giometti]

On 01/10/2026 22:21, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> The time reported in ::systime of a system_time_snapshot is known to be
> slightly inaccurate because of the way that the reported realtime clock
> sawtooths around the *intended* time series, limited by the integer mult
> value used to calculate the inter-tick times, and designed to ensure
> smoothness and monotonicity for its consumers.
>
> It is particularly inaccurate in a tickless kernel, where ntp_err_mult
> is not adjusted on each tick, allowing the reported clock to diverge
> from the intended time for a large number of ticks before re-converging.
>
> This appears to be the reason why CONFIG_NTP_PPS is not enabled on
> tickless kernels — because at that scale of precision, the realtime
> snapshot at the time of the pulse bears little relation to the time the
> kernel *actually* believes it to be, thus introducing random errors into
> the PPS phase correction.

Since enabling NTP_PPS on tickless kernels no longer depends on this
patch, I think this paragraph should go.

>
> It would be better for callers of get_device_system_crosststamp() and
> ktime_get_snapshot_id() to receive the *accurate* time, not the
> sanitized version provided to gettimeofday().

With 1-4 applied the correction at a PPS edge should be in the tens of
ns you measured: is it worth having ts_real differ from clock_gettime()
for that?

>
> Compute the deviation in snapshot_ntp_error() and add it to the returned
> ::systime so the snapshot lands on the ideal line. It sums four terms in
> ns << NTP_SCALE_SHIFT before converting to signed ns:
>
>   - tk->ntp_error, the deviation as of the last update;
>   - (cycle_delta * ntp_err_frac), the fractional-mult drift accrued
>     since then (cycle_delta is at most a tick on a tickful kernel, but
>     many ticks' worth under NO_HZ);
>   - (cycle_delta * ntp_err_mult), subtracting the applied +1 mult dither
>     over the same span;
>   - the sub-nanosecond fraction dropped when the read was truncated to
>     whole ns (low shift bits, exact despite the multiply overflowing).
>
> The helper uses the timekeeper selected for the requested clock id, so
> all NTP-disciplined clocks are corrected, including the AUX clocks (each
> has its own NTP instance); only CLOCK_MONOTONIC_RAW is undisciplined and
> gets no correction. The residual is then a single clocksource cycle, the
> same bound as a tickful kernel.
>
> Note that this *unconditionally* changes the ::systime returned by all
> snapshot and cross timestamp consumers (PTP SYS_OFFSET_PRECISE/EXTENDED,
> etc.): it is now the ideal NTP-disciplined time rather than the raw
> accumulated clock.

With CONFIG_NTP_PPS this also changes the timestamps PPS_FETCH returns
to userspace: I think PPS should be named here.

>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Assisted-by: LLM
> ---
>  include/linux/timekeeper_internal.h |  5 ++
>  kernel/time/timekeeping.c           | 71 +++++++++++++++++++++++++++--
>  2 files changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
> index 85d123bcb270..1f69479dd846 100644
> --- a/include/linux/timekeeper_internal.h
> +++ b/include/linux/timekeeper_internal.h
> @@ -99,6 +99,10 @@ struct tk_read_base {
>   * @ntp_err_mult:		Multiplication factor for scaled math conversion
>   * @err_drain:			Extra mult bias repaying ntp_error beyond the
>   *				dither's reach; sized at second boundaries
> + * @ntp_err_frac:		Fractional part of the per-cycle NTP-ideal mult that the
> + *				integer @mult truncates, as a fraction of 2^32 in
> + *				clock-shifted nanoseconds per cycle. Describes the
> + *				working point of the +-1 mult dither.
>   * @cs_tick_adj:		Per-second adjustment handed to NTP via ntp_clear()
>   *				accounting for the difference between the nominal
>   *				NTP interval and the real time taken by the
> @@ -190,6 +194,7 @@ struct timekeeper {
>  	u32			ntp_error_shift;
>  	s32			ntp_err_mult;
>  	s32			err_drain;
> +	u64			ntp_err_frac;
>  	s64			cs_tick_adj;
>  	u32			skip_second_overflow;
>  	s64			skew_delta;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 48d916a6c433..83c3e5ef9c5f 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -431,6 +431,7 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
>  	tk->tkr_mono.mult = clock->mult;
>  	tk->tkr_raw.mult = clock->mult;
>  	tk->ntp_err_mult = 0;
> +	tk->ntp_err_frac = 0;
>  	tk->err_drain = 0;
>  	tk->skip_second_overflow = 0;
>  	tk->skew_delta = 0;
> @@ -1241,6 +1242,55 @@ static inline u64 tk_clock_read_snapshot(const struct tk_read_base *tkr,
>  	return clock->read(clock);
>  }
>
> +/*
> + * snapshot_ntp_error - record how far a snapshot's ::systime is from the
> + * ideal NTP-disciplined time at @now, in signed nanoseconds, so a caller
> + * can land exactly on the ideal line by adding it to ::systime.
> + *
> + * The value is summed in ns << NTP_SCALE_SHIFT from four parts:
> + *
> + *  - tk->ntp_error, the deviation accumulated as of the last timekeeping
> + *    update (tkr_mono.cycle_last);
> + *  - (cycle_delta * ntp_err_frac), the fractional-mult drift accrued over
> + *    the cycles read since then -- at most a tick on a tickful kernel, but
> + *    potentially many ticks' worth under NO_HZ;
> + *  - (cycle_delta * ntp_err_mult), subtracting the applied +1 mult dither
> + *    over the same span;
> + *  - the sub-nanosecond fraction that ::systime dropped when the read was
> + *    truncated to whole ns (the low @shift bits, exact even though the
> + *    multiply overflows).
> + *
> + * CLOCK_MONOTONIC_RAW is not NTP-disciplined and carries no error. Every
> + * other clock id uses its own timekeeper @tk -- including the AUX clocks,
> + * which each have their own NTP instance.
> + */
> +static s64 snapshot_ntp_error(const struct timekeeper *tk, clockid_t clock_id,
> +			      u64 now)
> +{
> +	u64 cycle_delta;
> +	u32 nes;
> +	s64 tmp, err;
> +
> +	if (clock_id == CLOCK_MONOTONIC_RAW)
> +		return 0;
> +
> +	cycle_delta = (now - tk->tkr_mono.cycle_last) & tk->tkr_mono.mask;
> +	nes = tk->ntp_error_shift;
> +
> +	/* Insane deltas from marginal clocksources: skip the extrapolation */
> +	if (unlikely(cycle_delta > tk->tkr_mono.clock->max_cycles))
> +		return tk->ntp_error >> NTP_SCALE_SHIFT;
> +
> +	err = tk->ntp_error;
> +	err += ((s64)mul_u64_u64_shr(cycle_delta, tk->ntp_err_frac, 32) -
> +		(s64)cycle_delta * tk->ntp_err_mult) << nes;
> +
> +	tmp = (s64)(cycle_delta * tk->tkr_mono.mult + tk->tkr_mono.xtime_nsec);
> +	tmp &= (1ULL << tk->tkr_mono.shift) - 1;
> +	err += tmp << nes;
> +
> +	return (err + (1LL << (NTP_SCALE_SHIFT - 1))) >> NTP_SCALE_SHIFT;
> +}
>
>  /**
>   * ktime_get_snapshot_id -  Simultaneously snapshot a given clock ID with
> @@ -1264,6 +1314,7 @@ void ktime_get_snapshot_id(clockid_t clock_id, struct system_time_snapshot *syst
>  {
>  	ktime_t base_raw, base_sys, offs_sys, *offs, offs_zero = 0;
>  	u64 nsec_raw, nsec_sys, now;
> +	s64 ntp_error;
>  	struct timekeeper *tk;
>  	struct tk_data *tkd;
>  	unsigned int seq;
> @@ -1326,10 +1377,12 @@ void ktime_get_snapshot_id(clockid_t clock_id, struct system_time_snapshot *syst
>
>  		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
>  		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
> +
> +		ntp_error = snapshot_ntp_error(tk, clock_id, now);
>  	} while (read_seqcount_retry(&tkd->seq, seq));
>
>  	systime_snapshot->cycles = now;
> -	systime_snapshot->systime = ktime_add_ns(base_sys, offs_sys + nsec_sys);
> +	systime_snapshot->systime = ktime_add_ns(base_sys, offs_sys + nsec_sys) + ntp_error;
>  	systime_snapshot->monoraw = ktime_add_ns(base_raw, nsec_raw);
>
>  	/*
> @@ -1581,6 +1634,7 @@ int get_device_system_crosststamp(int (*get_time_fn)
>  	ktime_t base_sys, base_raw, *offs;
>  	u32 clock_was_set_seq = 0;
>  	u64 nsec_sys, nsec_raw;
> +	s64 ntp_error;
>  	u8 cs_was_changed_seq;
>  	unsigned int seq;
>  	bool do_interp;
> @@ -1647,9 +1701,10 @@ int get_device_system_crosststamp(int (*get_time_fn)
>
>  		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, cycles);
>  		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, cycles);
> +		ntp_error = snapshot_ntp_error(tk, xtstamp->clock_id, cycles);
>  	} while (read_seqcount_retry(&tkd->seq, seq));
>
> -	xtstamp->sys_systime = ktime_add_ns(base_sys, nsec_sys);
> +	xtstamp->sys_systime = ktime_add_ns(base_sys, nsec_sys) + ntp_error;
>  	xtstamp->sys_monoraw = ktime_add_ns(base_raw, nsec_raw);
>
>  	/*
> @@ -2460,6 +2515,7 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
>  {
>  	u64 ntp_tl = ntp_tick_length(tk->id);
>  	s64 skew = ntp_get_skew_delta(tk->id);
> +	u64 dividend;
>  	u32 mult;
>
>  	/*
> @@ -2480,8 +2536,15 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
>  		 * scale it back up to the full per-tick rate for the mult bias.
>  		 */
>  		skew *= NTP_INTERVAL_FREQ;
> -		mult = div64_u64((tk->ntp_tick + skew) >> tk->ntp_error_shift,
> -				 tk->cycle_interval);
> +		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

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [PATCH 5/5] [DO NOT MERGE] timekeeping: Apply extrapolated ntp_error to clock snapshots

[David Woodhouse]

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

On Fri, 2026-10-02 at 10:30 +0200, Rodolfo Giometti wrote:
> On 01/10/2026 22:21, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > The time reported in ::systime of a system_time_snapshot is known to be
> > slightly inaccurate because of the way that the reported realtime clock
> > sawtooths around the *intended* time series, limited by the integer mult
> > value used to calculate the inter-tick times, and designed to ensure
> > smoothness and monotonicity for its consumers.
> > 
> > It is particularly inaccurate in a tickless kernel, where ntp_err_mult
> > is not adjusted on each tick, allowing the reported clock to diverge
> > from the intended time for a large number of ticks before re-converging.
> > 
> > This appears to be the reason why CONFIG_NTP_PPS is not enabled on
> > tickless kernels — because at that scale of precision, the realtime
> > snapshot at the time of the pulse bears little relation to the time the
> > kernel *actually* believes it to be, thus introducing random errors into
> > the PPS phase correction.
> 
> Since enabling NTP_PPS on tickless kernels no longer depends on this
> patch, I think this paragraph should go.

Yep. Assuming the NTP_PPS tickless enablement lands under separate
cover, after the main part of this series but before *this* "DO NOT
MERGE" patch, I should just lump PPS in with the other users listed
later for consideration, as you said.

For that assumption to be true, we have to be happy that the ntp_error
reductions in patches 1-4 are sufficient, and that we don't need to
*also* switch pps_get_ts() to ktime_get_snapshot_id() and have this
patch which applies the correction to the snapshot.

Which leads us to your next question...

> > It would be better for callers of get_device_system_crosststamp() and
> > ktime_get_snapshot_id() to receive the *accurate* time, not the
> > sanitized version provided to gettimeofday().
> 
> With 1-4 applied the correction at a PPS edge should be in the tens of
> ns you measured: is it worth having ts_real differ from clock_gettime()
> for that?

Good question; I've been wondering about that. In a sense, I'm fixing
the same problem *three* times. First I eliminate the cases which
*introduce* significant ntp_error (patches 1-2), then I let the system
*eliminate* it when it does happen (patches 3-4) and now this patch
even *deducts* what remains from the snapshots.

I think all three *do* make sense, even together. Especially now my
last-minute Sashiko review pointed out that the 'eliminate' part is
only for the core timekeeper and not the aux clocks (we *could* change
that, at a cost of extra work on the timekeeping_max_deferment() path).

But also, even for the core timekeeper in a tickless kernel, that
ntp_error can still accumulate at *any* time. If it has exceeded the
elimination threshold while the system sleeps, it could still pollute a
snapshot which is taken at wake time, before the correction has a
chance to happen.

So I think we do need it, and my inclination is to hold off on enabling
CONFIG_NTP_PPS for tickless kernels until we do. But I'll defer to your
preference. If you want to merge it sooner on the basis that with a
1PPS signal the system doesn't get to sleep for long *anyway*, I can do
another test run with just patches 1-4.


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

Re: [PATCH 5/5] [DO NOT MERGE] timekeeping: Apply extrapolated ntp_error to clock snapshots

[Rodolfo Giometti]

On 02/10/2026 11:14, David Woodhouse wrote:
> On Fri, 2026-10-02 at 10:30 +0200, Rodolfo Giometti wrote:
>> On 01/10/2026 22:21, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> The time reported in ::systime of a system_time_snapshot is known to be
>>> slightly inaccurate because of the way that the reported realtime clock
>>> sawtooths around the *intended* time series, limited by the integer mult
>>> value used to calculate the inter-tick times, and designed to ensure
>>> smoothness and monotonicity for its consumers.
>>>
>>> It is particularly inaccurate in a tickless kernel, where ntp_err_mult
>>> is not adjusted on each tick, allowing the reported clock to diverge
>>> from the intended time for a large number of ticks before re-converging.
>>>
>>> This appears to be the reason why CONFIG_NTP_PPS is not enabled on
>>> tickless kernels — because at that scale of precision, the realtime
>>> snapshot at the time of the pulse bears little relation to the time the
>>> kernel *actually* believes it to be, thus introducing random errors into
>>> the PPS phase correction.
>>
>> Since enabling NTP_PPS on tickless kernels no longer depends on this
>> patch, I think this paragraph should go.
> 
> Yep. Assuming the NTP_PPS tickless enablement lands under separate
> cover, after the main part of this series but before *this* "DO NOT
> MERGE" patch, I should just lump PPS in with the other users listed
> later for consideration, as you said.
> 
> For that assumption to be true, we have to be happy that the ntp_error
> reductions in patches 1-4 are sufficient, and that we don't need to
> *also* switch pps_get_ts() to ktime_get_snapshot_id() and have this
> patch which applies the correction to the snapshot.
> 
> Which leads us to your next question...
> 
>>> It would be better for callers of get_device_system_crosststamp() and
>>> ktime_get_snapshot_id() to receive the *accurate* time, not the
>>> sanitized version provided to gettimeofday().
>>
>> With 1-4 applied the correction at a PPS edge should be in the tens of
>> ns you measured: is it worth having ts_real differ from clock_gettime()
>> for that?
> 
> Good question; I've been wondering about that. In a sense, I'm fixing
> the same problem *three* times. First I eliminate the cases which
> *introduce* significant ntp_error (patches 1-2), then I let the system
> *eliminate* it when it does happen (patches 3-4) and now this patch
> even *deducts* what remains from the snapshots.
> 
> I think all three *do* make sense, even together. Especially now my
> last-minute Sashiko review pointed out that the 'eliminate' part is
> only for the core timekeeper and not the aux clocks (we *could* change
> that, at a cost of extra work on the timekeeping_max_deferment() path).
> 
> But also, even for the core timekeeper in a tickless kernel, that
> ntp_error can still accumulate at *any* time. If it has exceeded the
> elimination threshold while the system sleeps, it could still pollute a
> snapshot which is taken at wake time, before the correction has a
> chance to happen.
> 
> So I think we do need it, and my inclination is to hold off on enabling
> CONFIG_NTP_PPS for tickless kernels until we do. But I'll defer to your
> preference. If you want to merge it sooner on the basis that with a
> 1PPS signal the system doesn't get to sleep for long *anyway*, I can do
> another test run with just patches 1-4.
>

Yes, please do that run. This patch changes what PPS_FETCH returns to
userspace, so it has to wait for the chrony and ntpd people anyway; if
1-4 alone are good enough at 1PPS I'd rather not tie the tickless
enablement to it.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [PATCH 5/5] [DO NOT MERGE] timekeeping: Apply extrapolated ntp_error to clock snapshots

[David Woodhouse]

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

On Fri, 2026-10-02 at 12:26 +0200, Rodolfo Giometti wrote:
>  
> > So I think we do need it, and my inclination is to hold off on enabling
> > CONFIG_NTP_PPS for tickless kernels until we do. But I'll defer to your
> > preference. If you want to merge it sooner on the basis that with a
> > 1PPS signal the system doesn't get to sleep for long *anyway*, I can do
> > another test run with just patches 1-4.
> > 
> 
> Yes, please do that run. This patch changes what PPS_FETCH returns to
> userspace, so it has to wait for the chrony and ntpd people anyway; if
> 1-4 alone are good enough at 1PPS I'd rather not tie the tickless
> enablement to it.

https://david.woodhou.se/ntptest-r64/rodolfo-14-tickless-1hz/

Fairly much identical to the full series running tickless, which was
https://david.woodhou.se/ntptest-r64/tickless-1hz/

And not really much worse than the tickful variant
https://david.woodhou.se/ntptest-r64/tickful-1hz/

The ntp_error isn't measured because it's the clean series and most of
the instrumentation is gone. I'll set the 5s pulse version running
overnight.

The series is also tested on four virtual machines on the same host,
tickful and tickless, patches vs. baseline. You can see the tickless
baseline wobble when the actual frequency changes, while the other
three remain stable. You can also see ntp_error on both baseline
kernels spiking during the initial sync.
https://david.woodhou.se/ntptest-virt/

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