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

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

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