[RFC PATCH 0/1] timekeeping: Don't charge NTP frequency changes to ntp_error

[RFC PATCH 0/1] timekeeping: Don't charge NTP frequency changes to ntp_error

[Yuan Wang]

From: Jie Wang <jie.wang@intel.com>

Hi,

Since b7befd6d9120 ("timekeeping: Account for monotonicity adjustment in
ntp_error") we see phc2sys take minutes, instead of seconds, to settle
CLOCK_REALTIME back to a few tens of ns after the PHC it follows is
stepped. The PHC itself (ptp4l) recovers as fast as before; only the
PHC -> CLOCK_REALTIME loop is affected.

Setup: two Intel i226 NICs back to back, gPTP (802.1AS profile, L2 P2P),
ptp4l + "phc2sys -O 0 -c CLOCK_REALTIME -s <if>" on both ends, PREEMPT_RT,
HZ=1000, TSC clocksource. The grandmaster's PHC is stepped by +1s with
"testptp -t 1". Time until |phc2sys offset| stays below 100ns:

                                 GM phc2sys      slave phc2sys  slave ptp4l
  v7.0 (before the series)         51-56s (test team, slave side)
  v7.2.7    (b7befd6d9120 only)    257-386s        79-333s        33-34s
  v7.2.7 + revert of b7befd6d9120  32s             40-42s         34-35s
  v7.3      (whole series)         207/>361/>413s  79-293s        34-35s
  v7.3 + revert of the series      32s             40-41s         34-35s
  v7.3 + this patch                32s             40-41s         34-35s

  (">" = not settled before the next step 400s later)

What happens: after the step phc2sys slews CLOCK_REALTIME at up to 10%
(ADJ_TICK + ADJ_FREQUENCY) and then backs off by several percent per
second. Since b7befd6d9120 every one of these base-rate changes adds
offset * mult_adj to ntp_error - up to a tick's worth of cycles times the
frequency change, i.e. tens of microseconds per update. ntp_error is only
repaid by the +1 mult dither (a few hundred ppb here), so CLOCK_REALTIME
keeps moving on its own for minutes after phc2sys has converged, and the
servo keeps chasing it.

That correction is the one c2cda2a5bda9 ("timekeeping/ntp: Don't align
NTP frequency adjustments to ticks") removed in 2018: it makes the clock
behave as if a new frequency had been applied at the start of the tick
rather than when adjtimex() was called. As far as I can tell the v7 series
discussion didn't revisit that trade-off.

I think both views are right for different callers:

 - A new base rate set from user space (ADJ_FREQUENCY/ADJ_TICK) belongs to
   an external servo (chrony, ptp4l, phc2sys) that measures the clock
   directly. Any phase effect is seen and corrected by that servo; booking
   it in ntp_error as well makes the kernel pay it back a second time,
   slowly.

 - The kernel's own adjustments - the mult/mult+1 dither and the
   skew_delta bias that delivers time_offset/time_adjust - have a phase
   target inside the kernel, and there the compensation from b7befd6d9120
   is what keeps the delivered phase exact.

So the patch keeps the compensation for the dither and skew changes, but
applies a new NTP base rate without charging ntp_error, as before
b7befd6d9120.

Open question, mainly for David: if timekeeping_set_reference() (ffclock
branch) changes tick_length, it would now also be applied "at the time of
the update". If that path needs the as-if-at-tick semantics, I'd suggest
it delivers the phase through time_offset (which the series now makes
exact) or passes an explicit flag down to timekeeping_adjust(); happy to
rework the patch either way.

The patch doesn't change the kernel-internal slews. An adjtime()
(ADJ_OFFSET_SINGLESHOT) of +5000us with no servo running, measured as
CLOCK_REALTIME - CLOCK_MONOTONIC_RAW, settles 10-12s later at:
  v7.3:              5009255ns (+9.3us)
  v7.3 + this patch: 5001024ns (+1.0us)
(one run each, so I'd read that as "not worse" rather than "better"; the
residual error in both cases looks worth a separate look). Steady-state
phc2sys lock before the step is the same on all three 7.3 kernels
(7-10ns rms on the GM).

phc2sys -S (step instead of slew) avoids the problem on an unpatched
kernel - a step clears ntp_error - but steps are usually not acceptable
on TSN systems once the schedule is running, so that is only a
workaround.

This fixes a 7.2 regression, so it probably wants
Cc stable list once the approach is agreed.

Jie Wang (1):
  timekeeping: Don't charge NTP frequency changes to ntp_error

 kernel/time/timekeeping.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

[RFC PATCH 1/1] timekeeping: Don't charge NTP frequency changes to ntp_error

[Yuan Wang]

From: Jie Wang <jie.wang@intel.com>

Commit b7befd6d9120 ("timekeeping: Account for monotonicity adjustment
in ntp_error") adds the xtime_nsec monotonicity fixup to ntp_error for
every mult change, including the change of the base rate when user space
sets a new frequency via adjtimex(). That makes the clock behave as if
the new frequency had been applied at the start of the tick, which is
exactly the correction removed by commit c2cda2a5bda9 ("timekeeping/ntp:
Don't align NTP frequency adjustments to ticks").

The amount is offset * mult_adj, i.e. up to one tick worth of cycles
times the frequency change. After a clock step, phc2sys and ptp4l slew
CLOCK_REALTIME at up to 10% and then back off by several percent per
second, which charges tens of microseconds to ntp_error each second.
ntp_error is only repaid by the +1 mult dithering (tens to hundreds of
ppb), so the clock keeps drifting on its own for minutes and fights the
external servo.

On two Intel i226 NICs running gPTP (ptp4l + phc2sys on each end,
PREEMPT_RT, HZ=1000, TSC), after a 1s step of the grandmaster's PHC,
the time until the phc2sys offset stays below 100ns is:

                         GM phc2sys     slave phc2sys  slave ptp4l
  v7.3                   207/>361/>413s 79-293s        34-35s
  v7.3, series reverted  32s            40-41s         34-35s
  v7.3 + this patch      32s            40-41s         34-35s

">" means it had not settled before the next step, 400s later.

Keep the ntp_error compensation for the ntp_err_mult dithering and the
skew bias, but apply a new base rate without charging ntp_error, so a
frequency change takes effect at the time of the update again.

Fixes: b7befd6d9120 ("timekeeping: Account for monotonicity adjustment in ntp_error")

Signed-off-by: Jie Wang <jie.wang@intel.com>
---
 kernel/time/timekeeping.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ea2e6e55f37b..8f6ab6c077f0 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2347,7 +2347,8 @@ device_initcall(timekeeping_init_ops);
  */
 static __always_inline void timekeeping_apply_adjustment(struct timekeeper *tk,
 							 s64 offset,
-							 s32 mult_adj)
+							 s32 mult_adj,
+							 bool track_error)
 {
 	s64 interval = tk->cycle_interval;
 
@@ -2412,6 +2413,10 @@ static __always_inline void timekeeping_apply_adjustment(struct timekeeper *tk,
 	 * (in appropriate units) has to be added to ntp_error, in
 	 * order to correctly track the delta between the time
 	 * reported in xtime_nsec, and the intended time.
+	 *
+	 * The exception is a new NTP frequency (@track_error == false):
+	 * it takes effect at the time of this update, not at the start
+	 * of the tick, so there is no delta to track.
 	 */
 	if ((mult_adj > 0) && (tk->tkr_mono.mult + mult_adj < mult_adj)) {
 		/* NTP adjustment caused clocksource mult overflow */
@@ -2422,7 +2427,8 @@ static __always_inline void timekeeping_apply_adjustment(struct timekeeper *tk,
 	tk->tkr_mono.mult += mult_adj;
 	tk->xtime_interval += interval;
 	tk->tkr_mono.xtime_nsec -= offset;
-	tk->ntp_error += offset << tk->ntp_error_shift;
+	if (track_error)
+		tk->ntp_error += offset << tk->ntp_error_shift;
 }
 
 /*
@@ -2433,6 +2439,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);
+	bool freq_changed = tk->ntp_tick != ntp_tl;
 	u32 mult;
 
 	/*
@@ -2453,6 +2460,19 @@ 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);
+
+		/*
+		 * Switch to the new base rate first. A new NTP frequency takes
+		 * effect at the time of this update rather than at the start of
+		 * the tick, so don't charge the realignment to ntp_error.
+		 * ntp_error is only repaid by the +1 mult dithering below, so
+		 * after a large frequency change (e.g. phc2sys/ptp4l slewing
+		 * after a clock step) it would otherwise drag the clock for
+		 * minutes against the external servo.
+		 */
+		timekeeping_apply_adjustment(tk, offset,
+					     mult + tk->ntp_err_mult - tk->tkr_mono.mult,
+					     !freq_changed);
 	}
 
 	/*
@@ -2464,7 +2484,7 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
 	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);
+	timekeeping_apply_adjustment(tk, offset, mult - tk->tkr_mono.mult, true);
 
 	if (unlikely(tk->tkr_mono.clock->maxadj &&
 		(abs(tk->tkr_mono.mult - tk->tkr_mono.clock->mult)
-- 
2.43.0