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

[Yuan Wang <yuan1.wang@intel.com>]

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