From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B6FA823392B for ; Thu, 9 Jul 2026 12:56:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=210.118.77.12 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783601797; cv=none; b=gUUqNaQV7h74MFBEC7xSoFBDFV+alkdVoOHFUkj8kh+hbzR9XHwW5lYFMCCJBILv1GPQjqe8QJhv7uQ/GNPSELI6Q0f2xXZR8KEsltFKPoe2SlkhuUlAZxmkGOo7jc71iwhIWOPr/3TYhRWpSpoILihs2kUFO/6yxBFp3ttrYbc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783601797; c=relaxed/simple; bh=hZdN9hFcY5cbVrtGEiiiSbthUuJkDoUJynEZ0hPwjdo=; h=Message-ID:Date:MIME-Version:Subject:To:From:In-Reply-To: Content-Type:References; b=HepHuEitOwMecoggLELwKJSinNfezDZSDc4dc+r6ijHItGuQ2fGOb/hV5zWUjDkxEhanKDPno69eEjk7vM8hhg34jK+D/y5ywqwMXMHEW9CeWkjBBW9fKhO8zb8S7P24YKZOFYtGjPC+IDnlIcTZDBiCpywhPZIt+3uwU8bWNkg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com; spf=pass smtp.mailfrom=samsung.com; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b=Q7q2T8sF; arc=none smtp.client-ip=210.118.77.12 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=samsung.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b="Q7q2T8sF" Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20260709125631euoutp02eec9606e487d323623045e2a768bda1b~AoGE4_LM-2536125361euoutp02Q for ; Thu, 9 Jul 2026 12:56:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20260709125631euoutp02eec9606e487d323623045e2a768bda1b~AoGE4_LM-2536125361euoutp02Q DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1783601791; bh=5Fi4vI8zNwv9GqdGrCjnJeBEycjc3m3cnCP4eSjs/Vo=; h=Date:Subject:To:From:In-Reply-To:References:From; b=Q7q2T8sFZj9VQbvZ6jx5f9X3xUu85o5EF8rFIT5SS2Co4hGcNzVXA5VEEcfgi5Xsr tFNrBsEU97Rw0Z5OlR7DltzoR183G7VQ7fIDi4W3wf5ZBDOzAfQqHYSWh/xkEs0m24 Hz5wDX5yTNAsc9kTaANfRNVJ60ArXrhUhi0p2qso= Received: from eusmtip1.samsung.com (unknown [203.254.199.221]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20260709125630eucas1p2d1acb4893a1c72dfba5312453c0971f2~AoGD74jqK1249912499eucas1p2x; Thu, 9 Jul 2026 12:56:30 +0000 (GMT) Received: from [106.210.134.192] (unknown [106.210.134.192]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20260709125629eusmtip1c405e66566350e8ee987bd5483758a79~AoGCy3F0A1258012580eusmtip1y; Thu, 9 Jul 2026 12:56:29 +0000 (GMT) Message-ID: Date: Thu, 9 Jul 2026 14:56:28 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Betterbird (Windows) Subject: Re: [PATCH v7 3/7] timekeeping: Account for clocksource tick quantisation via NTP To: David Woodhouse , Richard Cochran , Wen Gu , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , John Stultz , Thomas Gleixner , Stephen Boyd , Anna-Maria Behnsen , Frederic Weisbecker , Shuah Khan , Peter Zijlstra , =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= , Arnd Bergmann , Miroslav Lichvar , Julien Ridoux , Ryan Luu , linux-kernel@vger.kernel.org Content-Language: en-US From: Marek Szyprowski In-Reply-To: <20260621220051.1030462-4-dwmw2@infradead.org> Content-Transfer-Encoding: 8bit X-CMS-MailID: 20260709125630eucas1p2d1acb4893a1c72dfba5312453c0971f2 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20260709125630eucas1p2d1acb4893a1c72dfba5312453c0971f2 X-EPHeader: CA X-CMS-RootMailID: 20260709125630eucas1p2d1acb4893a1c72dfba5312453c0971f2 References: <20260621220051.1030462-1-dwmw2@infradead.org> <20260621220051.1030462-4-dwmw2@infradead.org> On 21.06.2026 23:53, David Woodhouse wrote: > From: David Woodhouse > > 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 > 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