Re: [PATCH v7 3/7] timekeeping: Account for clocksource tick quantisation via NTP

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 21.06.2026 23:53, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> 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.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Assisted-by: Kiro:claude-opus-4.8


This patch landed yesterday in linux-next as commit e8bf3955dc2f
("timekeeping: Account for clocksource tick quantisation via NTP").
In my tests I found that it breaks booting Raspberry Pi 3B and Pi 4B
boards in ARM 32bit mode with quite old (Debian 10) userspace. Booting
freezes just before getting a getty login prompt and no more kernel
or userspace messages are observed. If I start with init=/bin/bash in
kernel cmdline and then wait a while and do "exec /sbin/init" I get
the following additional kernel message:

[  186.867605] Adjusting arch_sys_counter more than 11% (12136 vs 344864995)

then booting freezes again before getting getty prompt.

I can do some more tests, just let me know what will help debugging this
issue. Other ARM 32bit machines I have work fine with this patch.

Reverting subject together with its dependencies on top of linux-next 
fixes the issue.


> ---
>  include/linux/timekeeper_internal.h |  8 ++--
>  kernel/time/ntp.c                   | 27 ++++++++++--
>  kernel/time/ntp_internal.h          |  2 +-
>  kernel/time/timekeeping.c           | 66 +++++++++++++++++++++--------
>  4 files changed, 78 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
> index e36d11e33e0c..ec81587a1400 100644
> --- a/include/linux/timekeeper_internal.h
> +++ b/include/linux/timekeeper_internal.h
> @@ -84,8 +84,6 @@ struct tk_read_base {
>   * @cycle_interval:		Number of clock cycles in one NTP interval
>   * @xtime_interval:		Number of clock shifted nano seconds in one NTP
>   *				interval.
> - * @xtime_remainder:		Shifted nano seconds left over when rounding
> - *				@cycle_interval
>   * @raw_interval:		Shifted raw nano seconds accumulated per NTP interval.
>   * @next_leap_ktime:		CLOCK_MONOTONIC time value of a pending leap-second
>   * @ntp_tick:			The ntp_tick_length() value currently being
> @@ -99,6 +97,10 @@ 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
> + * @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
> + *				clocksource's integer @cycle_interval (upscaled).
>   * @skip_second_overflow:	Flag used to avoid updating NTP twice with same second
>   * @tai_offset:			The current UTC to TAI offset in seconds
>   *
> @@ -178,7 +180,6 @@ struct timekeeper {
>  
>  	u64			cycle_interval;
>  	u64			xtime_interval;
> -	s64			xtime_remainder;
>  	u64			raw_interval;
>  
>  	ktime_t			next_leap_ktime;
> @@ -186,6 +187,7 @@ struct timekeeper {
>  	s64			ntp_error;
>  	u32			ntp_error_shift;
>  	u32			ntp_err_mult;
> +	s64			cs_tick_adj;
>  	u32			skip_second_overflow;
>  	s32			tai_offset;
>  };
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 97fa99b96dd0..3fad82c47c4c 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -39,6 +39,10 @@
>   * @time_reftime:	Time at last adjustment in seconds
>   * @time_adjust:	Adjustment value
>   * @ntp_tick_adj:	Constant boot-param configurable NTP tick adjustment (upscaled)
> + * @cs_tick_adj:	Fixed per-second adjustment compensating for the difference
> + *			between the nominal NTP interval and the real time taken
> + *			by the clocksource's integer @cycle_interval (upscaled).
> + *			Set by the timekeeping core via ntp_clear().
>   * @ntp_next_leap_sec:	Second value of the next pending leapsecond, or TIME64_MAX if no leap
>   *
>   * @pps_valid:		PPS signal watchdog counter
> @@ -70,6 +74,7 @@ struct ntp_data {
>  	time64_t		time_reftime;
>  	long			time_adjust;
>  	s64			ntp_tick_adj;
> +	s64			cs_tick_adj;
>  	time64_t		ntp_next_leap_sec;
>  #ifdef CONFIG_NTP_PPS
>  	int			pps_valid;
> @@ -255,6 +260,7 @@ static void ntp_update_frequency(struct ntp_data *ntpdata)
>  	second_length		 = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << NTP_SCALE_SHIFT;
>  
>  	second_length		+= ntpdata->ntp_tick_adj;
> +	second_length		+= ntpdata->cs_tick_adj;
>  	second_length		+= ntpdata->time_freq;
>  
>  	new_base		 = div_u64(second_length, NTP_INTERVAL_FREQ);
> @@ -350,11 +356,26 @@ static void __ntp_clear(struct ntp_data *ntpdata)
>  }
>  
>  /**
> - * ntp_clear - Clears the NTP state variables
> - * @tkid:	Timekeeper ID to be able to select proper ntp data array member
> + * ntp_clear - Clear NTP state and set the clocksource quantisation adjustment
> + * @tkid:		Timekeeper ID
> + * @cs_tick_adj:	Per-second adjustment in ns << NTP_SCALE_SHIFT
> + *
> + * The timekeeping core uses an integer number of cycles (@cycle_interval)
> + * per NTP interval, so the real time that interval represents differs from
> + * the nominal NTP_INTERVAL_LENGTH by up to half a counter period. Folding
> + * this fixed offset into @cs_tick_adj makes it an explicit part of the NTP
> + * tick_length computation in ntp.c, instead of being applied during
> + * timekeeping accumulation where the NTP code never saw it. Like
> + * @ntp_tick_adj it stays internal to the kernel; userspace still sees the
> + * nominal tick via adjtimex. NTP retains its full symmetric ±MAXFREQ range
> + * around the corrected base rate.
> + *
> + * Called whenever the clocksource is (re)configured, which is also when the
> + * rest of the NTP state must be cleared, so the two are done together.
>   */
> -void ntp_clear(unsigned int tkid)
> +void ntp_clear(unsigned int tkid, s64 cs_tick_adj)
>  {
> +	tk_ntp_data[tkid].cs_tick_adj = cs_tick_adj;
>  	__ntp_clear(&tk_ntp_data[tkid]);
>  }
>  
> diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
> index 7084d839c207..598e5dd2fc5b 100644
> --- a/kernel/time/ntp_internal.h
> +++ b/kernel/time/ntp_internal.h
> @@ -3,7 +3,7 @@
>  #define _LINUX_NTP_INTERNAL_H
>  
>  extern void ntp_init(void);
> -extern void ntp_clear(unsigned int tkid);
> +extern void ntp_clear(unsigned int tkid, s64 cs_tick_adj);
>  /* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
>  extern u64 ntp_tick_length(unsigned int tkid);
>  extern ktime_t ntp_get_next_leap(unsigned int tkid);
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d847bba0481b..53961a1fcf47 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -339,7 +339,6 @@ static inline void clocksource_enable_inline_read(void) { }
>  static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
>  {
>  	u64 interval;
> -	u64 tmp, ntpinterval;
>  	struct clocksource *old_clock;
>  
>  	++tk->cs_was_changed_seq;
> @@ -353,20 +352,16 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
>  	tk->tkr_raw.cycle_last = tk->tkr_mono.cycle_last;
>  
>  	/* Do the ns -> cycle conversion first, using original mult */
> -	tmp = NTP_INTERVAL_LENGTH;
> -	tmp <<= clock->shift;
> -	ntpinterval = tmp;
> -	tmp += clock->mult/2;
> -	do_div(tmp, clock->mult);
> -	if (tmp == 0)
> -		tmp = 1;
> -
> -	interval = (u64) tmp;
> +	interval = NTP_INTERVAL_LENGTH << clock->shift;
> +	interval += clock->mult/2;
> +	do_div(interval, clock->mult);
> +	if (interval == 0)
> +		interval = 1;
> +
>  	tk->cycle_interval = interval;
>  
>  	/* Go back from cycles -> shifted ns */
>  	tk->xtime_interval = interval * clock->mult;
> -	tk->xtime_remainder = ntpinterval - tk->xtime_interval;
>  	tk->raw_interval = interval * clock->mult;
>  
>  	 /* if changing clocks, convert xtime_nsec shift units */
> @@ -386,7 +381,38 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
>  
>  	tk->ntp_error = 0;
>  	tk->ntp_error_shift = NTP_SCALE_SHIFT - clock->shift;
> -	tk->ntp_tick = ntpinterval << tk->ntp_error_shift;
> +
> +	/*
> +	 * ntp_tick is the tick length that NTP disciplines (its ±500 PPM
> +	 * scales only this part), in NTP-shifted ns: the real interval of
> +	 * a whole number of counter cycles. Because cycle_interval is
> +	 * rounded to an integer number of cycles, this ntp_tick differs
> +	 * from the true intended 1/HZ tick length by up to half a cycle
> +	 * period.
> +	 */
> +	tk->ntp_tick = (u64)tk->xtime_interval << tk->ntp_error_shift;
> +
> +	/*
> +	 * cs_tick_adj is the constant difference between the disciplined
> +	 * ntp_tick above and the true 1/HZ tick, expressed per-second to
> +	 * match the ntp_update_frequency() addends and handed to NTP via
> +	 * ntp_clear() to be explicitly included in its tick_length.
> +	 *
> +	 * Worked example: HZ=1000, ACPI PM timer at 3.579545 MHz, which
> +	 * has 3579.545 cycles in 1ms, rounded to cycle_interval = 3580.
> +	 *
> +	 * So ntp_tick is actually 1.000127ms, as that is the amount of
> +	 * time that 3580 cycles will take at the nominal frequency. This
> +	 * is the part that NTP disciplines, causing each 3580 counts to
> +	 * advance the clock by up to NTP's ±500PPM of that amount.
> +	 *
> +	 * The "extra" 127ns/tick is what's stored in cs_tick_adj and
> +	 * applied as a constant correction by ntp_update_frequency() so
> +	 * that NTP *believes* it's disciplining a 1ms tick.
> +	 */
> +	tk->cs_tick_adj = (s64)tk->ntp_tick -
> +			  ((s64)NTP_INTERVAL_LENGTH << NTP_SCALE_SHIFT);
> +	tk->cs_tick_adj *= NTP_INTERVAL_FREQ;
>  
>  	/*
>  	 * 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
>  
>  	if (action & TK_CLEAR_NTP) {
>  		tk->ntp_error = 0;
> -		ntp_clear(tk->id);
> +		ntp_clear(tk->id, tk->cs_tick_adj);
>  	}
>  
>  	tk_update_leap_state(tk);
> @@ -2075,7 +2101,12 @@ void __init timekeeping_init(void)
>  
>  	tk_set_wall_to_mono(tks, wall_to_mono);
>  
> -	timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET);
> +	/*
> +	 * Use TK_UPDATE_ALL so the NTP layer picks up the clocksource's
> +	 * cs_tick_adj via ntp_clear(). Clearing NTP here is otherwise
> +	 * redundant as ntp_init() already initialised it above.
> +	 */
> +	timekeeping_update_from_shadow(&tk_core, TK_UPDATE_ALL);
>  }
>  
>  /* time in seconds when suspend began for persistent clock */
> @@ -2424,8 +2455,8 @@ static void timekeeping_adjust(struct timekeeper *tk, s64 offset)
>  		mult = tk->tkr_mono.mult - tk->ntp_err_mult;
>  	} else {
>  		tk->ntp_tick = ntp_tl;
> -		mult = div64_u64((tk->ntp_tick >> tk->ntp_error_shift) -
> -				 tk->xtime_remainder, tk->cycle_interval);
> +		mult = div64_u64(tk->ntp_tick >> tk->ntp_error_shift,
> +				 tk->cycle_interval);
>  	}
>  
>  	/*
> @@ -2550,8 +2581,7 @@ static u64 logarithmic_accumulation(struct timekeeper *tk, u64 offset,
>  
>  	/* Accumulate error between NTP and clock interval */
>  	tk->ntp_error += tk->ntp_tick << shift;
> -	tk->ntp_error -= (tk->xtime_interval + tk->xtime_remainder) <<
> -						(tk->ntp_error_shift + shift);
> +	tk->ntp_error -= tk->xtime_interval << (tk->ntp_error_shift + shift);
>  
>  	return offset;
>  }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland