cycle_interval is an integer number of counter cycles per NTP interval, so the real time it represents differs from the nominal NTP_INTERVAL_LENGTH by up to half a counter period. For coarse clocksources this is significant: the 3.579545 MHz ACPI PM timer at HZ=1000 rounds 3579.545 cycles up to 3580, making each tick 1.000127 ms (+127 PPM). Commit a386b5af8edd ("time: Compensate for rounding on odd-frequency clocksources") introduced xtime_remainder to compensate for exactly this, citing the same 127 PPM ACPI PM example. The compensation is correct and necessary, but it was applied inside the timekeeping accumulation in timekeeping.c: subtracted in the mult computation in timekeeping_adjust() and folded into the ntp_error update in logarithmic_accumulation(). That keeps the base rate correct and leaves NTP its full symmetric +/-MAXFREQ range rather than +373/-627 PPM, but the NTP code in ntp.c never sees it: tick_length is computed without the correction, so ntp.c's notion of how long a tick is disagrees with the rate timekeeping actually produces. Make the offset an explicit part of the NTP tick_length instead. Add ntp_data::cs_tick_adj, a fixed per-second addend that ntp_update_frequency() includes alongside ntp_tick_adj and time_freq. tk_setup_internals() computes it from the difference between the real cycle_interval duration and the nominal interval, stores it in the timekeeper, and hands it to NTP through a new argument to ntp_clear() -- which already recomputes the frequency and is invoked after every clocksource (re)configuration. timekeeping_init() now uses TK_UPDATE_ALL for this; clearing NTP there is otherwise redundant since ntp_init() has just initialised it. ntp.c now computes the true tick rate, giving a single source of truth. Like ntp_tick_adj, cs_tick_adj stays internal to the kernel: userspace still sees the nominal 1.000000 ms tick via adjtimex and is unaware of the addends. timekeeping_adjust() and logarithmic_accumulation() use ntp_tick / xtime_interval directly, and xtime_remainder is removed. The base-rate arithmetic is unchanged: ntp_tick becomes xtime_interval << ntp_error_shift, so the mult division yields the same base mult and the ntp_error accumulation still nets to zero per tick. Beyond the cleanup of treating all the tick_length contributions (nominal interval, ntp_tick_adj, cs_tick_adj, time_freq) consistently as addends in one place, it also prepares for feed-forward discipline: a future timekeeping_set_reference() will set tick_length to track an absolute external reference such as a vmclock, and that path needs ntp.c to own a tick_length that already reflects the clocksource quantisation, with no hidden correction applied elsewhere.
Showing the changes to kernel (3 of 4 files). Show all
Unified · Split
| @@ -39,6 +39,10 @@ · 38 unmodified lines | ||
| 39 | 39 | * @time_reftime: Time at last adjustment in seconds |
| 40 | 40 | * @time_adjust: Adjustment value |
| 41 | 41 | * @ntp_tick_adj: Constant boot-param configurable NTP tick adjustment (upscaled) |
| 42 | + * @cs_tick_adj: Fixed per-second adjustment compensating for the difference | |
| 43 | + * between the nominal NTP interval and the real time taken | |
| 44 | + * by the clocksource's integer @cycle_interval (upscaled). | |
| 45 | + * Set by the timekeeping core via ntp_clear(). | |
| 42 | 46 | * @ntp_next_leap_sec: Second value of the next pending leapsecond, or TIME64_MAX if no leap |
| 43 | 47 | * |
| 44 | 48 | * @pps_valid: PPS signal watchdog counter |
| @@ -70,6 +74,7 @@ struct ntp_data { · 25 unmodified lines | ||
| 70 | 74 | time64_t time_reftime; |
| 71 | 75 | long time_adjust; |
| 72 | 76 | s64 ntp_tick_adj; |
| 77 | + s64 cs_tick_adj; | |
| 73 | 78 | time64_t ntp_next_leap_sec; |
| 74 | 79 | #ifdef CONFIG_NTP_PPS |
| 75 | 80 | int pps_valid; |
| @@ -255,6 +260,7 @@ static void ntp_update_frequency(struct ntp_data *ntpdata) · 179 unmodified lines | ||
| 255 | 260 | second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << NTP_SCALE_SHIFT; |
| 256 | 261 | |
| 257 | 262 | second_length += ntpdata->ntp_tick_adj; |
| 263 | + second_length += ntpdata->cs_tick_adj; | |
| 258 | 264 | second_length += ntpdata->time_freq; |
| 259 | 265 | |
| 260 | 266 | new_base = div_u64(second_length, NTP_INTERVAL_FREQ); |
| @@ -350,11 +356,26 @@ static void __ntp_clear(struct ntp_data *ntpdata) · 89 unmodified lines | ||
| 350 | 356 | } |
| 351 | 357 | |
| 352 | 358 | /** |
| 353 | − * ntp_clear - Clears the NTP state variables | |
| 354 | − * @tkid: Timekeeper ID to be able to select proper ntp data array member | |
| 359 | + * ntp_clear - Clear NTP state and set the clocksource quantisation adjustment | |
| 360 | + * @tkid: Timekeeper ID | |
| 361 | + * @cs_tick_adj: Per-second adjustment in ns << NTP_SCALE_SHIFT | |
| 362 | + * | |
| 363 | + * The timekeeping core uses an integer number of cycles (@cycle_interval) | |
| 364 | + * per NTP interval, so the real time that interval represents differs from | |
| 365 | + * the nominal NTP_INTERVAL_LENGTH by up to half a counter period. Folding | |
| 366 | + * this fixed offset into @cs_tick_adj makes it an explicit part of the NTP | |
| 367 | + * tick_length computation in ntp.c, instead of being applied during | |
| 368 | + * timekeeping accumulation where the NTP code never saw it. Like | |
| 369 | + * @ntp_tick_adj it stays internal to the kernel; userspace still sees the | |
| 370 | + * nominal tick via adjtimex. NTP retains its full symmetric ±MAXFREQ range | |
| 371 | + * around the corrected base rate. | |
| 372 | + * | |
| 373 | + * Called whenever the clocksource is (re)configured, which is also when the | |
| 374 | + * rest of the NTP state must be cleared, so the two are done together. | |
| 355 | 375 | */ |
| 356 | −void ntp_clear(unsigned int tkid) | |
| 376 | +void ntp_clear(unsigned int tkid, s64 cs_tick_adj) | |
| 357 | 377 | { |
| 378 | + tk_ntp_data[tkid].cs_tick_adj = cs_tick_adj; | |
| 358 | 379 | __ntp_clear(&tk_ntp_data[tkid]); |
| 359 | 380 | } |
| 360 | 381 | |
| @@ -3,7 +3,7 @@ · 2 unmodified lines | ||
| 3 | 3 | #define _LINUX_NTP_INTERNAL_H |
| 4 | 4 | |
| 5 | 5 | extern void ntp_init(void); |
| 6 | −extern void ntp_clear(unsigned int tkid); | |
| 6 | +extern void ntp_clear(unsigned int tkid, s64 cs_tick_adj); | |
| 7 | 7 | /* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */ |
| 8 | 8 | extern u64 ntp_tick_length(unsigned int tkid); |
| 9 | 9 | extern ktime_t ntp_get_next_leap(unsigned int tkid); |
| @@ -339,7 +339,6 @@ static inline void clocksource_enable_inline_read(void) { } · 338 unmodified lines | ||
| 339 | 339 | static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock) |
| 340 | 340 | { |
| 341 | 341 | u64 interval; |
| 342 | − u64 tmp, ntpinterval; | |
| 343 | 342 | struct clocksource *old_clock; |
| 344 | 343 | |
| 345 | 344 | ++tk->cs_was_changed_seq; |
| @@ -353,20 +352,16 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock) · 7 unmodified lines | ||
| 353 | 352 | tk->tkr_raw.cycle_last = tk->tkr_mono.cycle_last; |
| 354 | 353 | |
| 355 | 354 | /* Do the ns -> cycle conversion first, using original mult */ |
| 356 | − tmp = NTP_INTERVAL_LENGTH; | |
| 357 | − tmp <<= clock->shift; | |
| 358 | − ntpinterval = tmp; | |
| 359 | − tmp += clock->mult/2; | |
| 360 | − do_div(tmp, clock->mult); | |
| 361 | − if (tmp == 0) | |
| 362 | − tmp = 1; | |
| 363 | − | |
| 364 | − interval = (u64) tmp; | |
| 355 | + interval = (u64)NTP_INTERVAL_LENGTH << clock->shift; | |
| 356 | + interval += clock->mult / 2; | |
| 357 | + do_div(interval, clock->mult); | |
| 358 | + if (interval == 0) | |
| 359 | + interval = 1; | |
| 360 | + | |
| 365 | 361 | tk->cycle_interval = interval; |
| 366 | 362 | |
| 367 | 363 | /* Go back from cycles -> shifted ns */ |
| 368 | 364 | tk->xtime_interval = interval * clock->mult; |
| 369 | − tk->xtime_remainder = ntpinterval - tk->xtime_interval; | |
| 370 | 365 | tk->raw_interval = interval * clock->mult; |
| 371 | 366 | |
| 372 | 367 | /* if changing clocks, convert xtime_nsec shift units */ |
| @@ -386,7 +381,38 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock) · 13 unmodified lines | ||
| 386 | 381 | |
| 387 | 382 | tk->ntp_error = 0; |
| 388 | 383 | tk->ntp_error_shift = NTP_SCALE_SHIFT - clock->shift; |
| 389 | − tk->ntp_tick = ntpinterval << tk->ntp_error_shift; | |
| 384 | + | |
| 385 | + /* | |
| 386 | + * ntp_tick is the tick length that NTP disciplines (its ±500 PPM | |
| 387 | + * scales only this part), in NTP-shifted ns: the real interval of | |
| 388 | + * a whole number of counter cycles. Because cycle_interval is | |
| 389 | + * rounded to an integer number of cycles, this ntp_tick differs | |
| 390 | + * from the true intended 1/HZ tick length by up to half a cycle | |
| 391 | + * period. | |
| 392 | + */ | |
| 393 | + tk->ntp_tick = (u64)tk->xtime_interval << tk->ntp_error_shift; | |
| 394 | + | |
| 395 | + /* | |
| 396 | + * cs_tick_adj is the constant difference between the disciplined | |
| 397 | + * ntp_tick above and the true 1/HZ tick, expressed per-second to | |
| 398 | + * match the ntp_update_frequency() addends and handed to NTP via | |
| 399 | + * ntp_clear() to be explicitly included in its tick_length. | |
| 400 | + * | |
| 401 | + * Worked example: HZ=1000, ACPI PM timer at 3.579545 MHz, which | |
| 402 | + * has 3579.545 cycles in 1ms, rounded to cycle_interval = 3580. | |
| 403 | + * | |
| 404 | + * So ntp_tick is actually 1.000127ms, as that is the amount of | |
| 405 | + * time that 3580 cycles will take at the nominal frequency. This | |
| 406 | + * is the part that NTP disciplines, causing each 3580 counts to | |
| 407 | + * advance the clock by up to NTP's ±500PPM of that amount. | |
| 408 | + * | |
| 409 | + * The "extra" 127ns/tick is what's stored in cs_tick_adj and | |
| 410 | + * applied as a constant correction by ntp_update_frequency() so | |
| 411 | + * that NTP *believes* it's disciplining a 1ms tick. | |
| 412 | + */ | |
| 413 | + tk->cs_tick_adj = (s64)tk->ntp_tick - | |
| 414 | + ((s64)NTP_INTERVAL_LENGTH << NTP_SCALE_SHIFT); | |
| 415 | + tk->cs_tick_adj *= NTP_INTERVAL_FREQ; | |
| 390 | 416 | |
| 391 | 417 | /* |
| 392 | 418 | * The timekeeper keeps its own mult values for the currently |
| @@ -803,7 +829,7 @@ static void timekeeping_update_from_shadow(struct tk_data *tkd, unsigned int act · 410 unmodified lines | ||
| 803 | 829 | |
| 804 | 830 | if (action & TK_CLEAR_NTP) { |
| 805 | 831 | tk->ntp_error = 0; |
| 806 | − ntp_clear(tk->id); | |
| 832 | + ntp_clear(tk->id, tk->cs_tick_adj); | |
| 807 | 833 | } |
| 808 | 834 | |
| 809 | 835 | tk_update_leap_state(tk); |
| @@ -2090,7 +2116,12 @@ void __init timekeeping_init(void) · 1280 unmodified lines | ||
| 2090 | 2116 | |
| 2091 | 2117 | tk_set_wall_to_mono(tks, wall_to_mono); |
| 2092 | 2118 | |
| 2093 | − timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET); | |
| 2119 | + /* | |
| 2120 | + * Use TK_UPDATE_ALL so the NTP layer picks up the clocksource's | |
| 2121 | + * cs_tick_adj via ntp_clear(). Clearing NTP here is otherwise | |
| 2122 | + * redundant as ntp_init() already initialised it above. | |
| 2123 | + */ | |
| 2124 | + timekeeping_update_from_shadow(&tk_core, TK_UPDATE_ALL); | |
| 2094 | 2125 | } |
| 2095 | 2126 | |
| 2096 | 2127 | /* time in seconds when suspend began for persistent clock */ |
| @@ -2439,8 +2470,8 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset) · 342 unmodified lines | ||
| 2439 | 2470 | mult = tk->tkr_mono.mult - tk->ntp_err_mult; |
| 2440 | 2471 | } else { |
| 2441 | 2472 | tk->ntp_tick = ntp_tl; |
| 2442 | − mult = div64_u64((tk->ntp_tick >> tk->ntp_error_shift) - | |
| 2443 | − tk->xtime_remainder, tk->cycle_interval); | |
| 2473 | + mult = div64_u64(tk->ntp_tick >> tk->ntp_error_shift, | |
| 2474 | + tk->cycle_interval); | |
| 2444 | 2475 | } |
| 2445 | 2476 | |
| 2446 | 2477 | /* |
| @@ -2565,8 +2596,7 @@ static u64 logarithmic_accumulation(struct timekeeper *tk, u64 offset, · 118 unmodified lines | ||
| 2565 | 2596 | |
| 2566 | 2597 | /* Accumulate error between NTP and clock interval */ |
| 2567 | 2598 | tk->ntp_error += tk->ntp_tick << shift; |
| 2568 | − tk->ntp_error -= (tk->xtime_interval + tk->xtime_remainder) << | |
| 2569 | − (tk->ntp_error_shift + shift); | |
| 2599 | + tk->ntp_error -= tk->xtime_interval << (tk->ntp_error_shift + shift); | |
| 2570 | 2600 | |
| 2571 | 2601 | return offset; |
| 2572 | 2602 | } |