* Re: [PATCH v3] printk: fix zero-valued printk timestamps in early boot
2026-03-29 22:42 ` [PATCH v3] printk: fix zero-valued printk timestamps in early boot Thomas Gleixner
@ 2026-03-30 12:05 ` Peter Zijlstra
2026-03-30 13:38 ` David Laight
2026-04-07 20:34 ` Earlier tsc init patch (was RE: [PATCH v3] printk: fix zero-valued printk timestamps in early boot) Bird, Tim
2 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2026-03-30 12:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Bird, Tim, pmladek@suse.com, rostedt@goodmis.org,
senozhatsky@chromium.org, Shashank Balaji,
john.ogness@linutronix.de, francesco@valla.it,
geert@linux-m68k.org, linux-embedded@vger.kernel.org,
linux-kernel@vger.kernel.org, x86, Paolo Bonzini,
Sean Christopherson, KVM
On Mon, Mar 30, 2026 at 12:42:59AM +0200, Thomas Gleixner wrote:
> Quite the contrary, I'm going to get rid of technical debt:
>
> The get_cycles() related changes in tsc.h are going to end up in a
> obviously revised formal patch tomorrow as there is exactly _zero_
> requirement to provide this as a functional interface.
>
> 1) The default implementation in asm-generic returns 0
>
> Ergo any code depending on a functional implementation is broken
> by definition.
>
> 2) The ops/cycles metrics are as bogus as the infamous BogoMIPS metrics
>
> The "cycle" counter runs on almost all contemporary CPUs at a fixed
> frequency, which is completely unrelated to the actual CPU
> frequency and therefore to the actual CPU cycles.
>
> The only realistic metric is ops/sec and nothing else and that can
> be trivially achieved by using the generic time getter interfaces.
>
> Those might end up resulting in bogus benchmark results too if the
> underlying clocksource is jiffies, but that's again a matter of
> accepting reality.
>
> If people really mandate that ops/bogo_cycles is required for the
> very wrong reasons, then I'm happy to bring it back with a global
> name change from get_cycles() to get_bogo_cycles() which excludes
> it from any serious usage including printk.
>
> Thanks,
>
> tglx
> ---
> arch/x86/include/asm/tsc.h | 11 ++--
> arch/x86/kernel/platform-quirks.c | 2
> arch/x86/kernel/tsc.c | 96 ++++++++++++++++++++++++++++++--------
> 3 files changed, 85 insertions(+), 24 deletions(-)
>
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -76,10 +76,7 @@ extern void disable_TSC(void);
>
> static inline cycles_t get_cycles(void)
> {
> - if (!IS_ENABLED(CONFIG_X86_TSC) &&
> - !cpu_feature_enabled(X86_FEATURE_TSC))
> - return 0;
> - return rdtsc();
> + return 0;
> }
> #define get_cycles get_cycles
>
So this is probably going to break a bunch of generic stuff. There is a
fair amount of 'get_cycles' usage out in the tree.
Mostly it appears to be various test code.
But I do agree that get_cycles() is terrible and should go away.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] printk: fix zero-valued printk timestamps in early boot
2026-03-29 22:42 ` [PATCH v3] printk: fix zero-valued printk timestamps in early boot Thomas Gleixner
2026-03-30 12:05 ` Peter Zijlstra
@ 2026-03-30 13:38 ` David Laight
2026-04-07 20:34 ` Earlier tsc init patch (was RE: [PATCH v3] printk: fix zero-valued printk timestamps in early boot) Bird, Tim
2 siblings, 0 replies; 4+ messages in thread
From: David Laight @ 2026-03-30 13:38 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Bird, Tim, pmladek@suse.com, rostedt@goodmis.org,
senozhatsky@chromium.org, Shashank Balaji,
john.ogness@linutronix.de, francesco@valla.it,
geert@linux-m68k.org, linux-embedded@vger.kernel.org,
linux-kernel@vger.kernel.org, x86, Paolo Bonzini,
Sean Christopherson, KVM
On Mon, 30 Mar 2026 00:42:59 +0200
Thomas Gleixner <tglx@kernel.org> wrote:
...
> If you want to have early timestamps then you have to go through the low
> level code on every architecture and do the proper sanity checks,
> enumerations, etc. no matter what. There is no guarantee that you can
> use any clock unconditionally during very early boot.
>
> As you have to do that anyway, there is _zero_ justification for an
> extra facility side stepping sched clock, which would just create
> another pile of technical debt.
>
> Quite the contrary, I'm going to get rid of technical debt:
>
> The get_cycles() related changes in tsc.h are going to end up in a
> obviously revised formal patch tomorrow as there is exactly _zero_
> requirement to provide this as a functional interface.
>
> 1) The default implementation in asm-generic returns 0
>
> Ergo any code depending on a functional implementation is broken
> by definition.
>
> 2) The ops/cycles metrics are as bogus as the infamous BogoMIPS metrics
>
> The "cycle" counter runs on almost all contemporary CPUs at a fixed
> frequency, which is completely unrelated to the actual CPU
> frequency and therefore to the actual CPU cycles.
>
> The only realistic metric is ops/sec and nothing else and that can
> be trivially achieved by using the generic time getter interfaces.
ops/sec is also horrid because you get all all the interrupt time getting
in the way.
Additionally it is only any use for relative comparisons done on exactly
the same hardware, and is entirely useless is you are trying to show that
a short code loop is running at/near the theoretical rate.
The only thing I've found that works is the PERF_COUNT_HW_CPU_CYCLES
counter (low 32 bits).
In userspace you do have to directly read the register (rather than using
the library function - too much overhead), and ignore any silly values
generated by interrupts and process switches.
But it is accurate enough to time single function calls and see the
effect of things like mispredicted branches and the data-dependency of
divide instructions.
But I do agree that the TSC is entirely useless for measuring what it was
originally intended to measure.
David
>
> Those might end up resulting in bogus benchmark results too if the
> underlying clocksource is jiffies, but that's again a matter of
> accepting reality.
>
> If people really mandate that ops/bogo_cycles is required for the
> very wrong reasons, then I'm happy to bring it back with a global
> name change from get_cycles() to get_bogo_cycles() which excludes
> it from any serious usage including printk.
>
> Thanks,
>
> tglx
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: Earlier tsc init patch (was RE: [PATCH v3] printk: fix zero-valued printk timestamps in early boot)
2026-03-29 22:42 ` [PATCH v3] printk: fix zero-valued printk timestamps in early boot Thomas Gleixner
2026-03-30 12:05 ` Peter Zijlstra
2026-03-30 13:38 ` David Laight
@ 2026-04-07 20:34 ` Bird, Tim
2 siblings, 0 replies; 4+ messages in thread
From: Bird, Tim @ 2026-04-07 20:34 UTC (permalink / raw)
To: Thomas Gleixner, pmladek@suse.com, rostedt@goodmis.org,
senozhatsky@chromium.org, Shashank Balaji,
john.ogness@linutronix.de
Cc: francesco@valla.it, geert@linux-m68k.org,
linux-embedded@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, Paolo Bonzini, Sean Christopherson, KVM
Thomas,
Thanks for posting this PoC patch. I found the code interesting and informative.
Please see my comments below.
> -----Original Message-----
> From: Thomas Gleixner <tglx@kernel.org>
> Tim!
>
> On Sat, Mar 28 2026 at 22:59, Thomas Gleixner wrote:
> > The only clean way to solve this cleanly is moving the sched clock
> > initialization to the earliest point possible and accepting that due to
> > hardware, enumeration and virtualization constraints this point might be
> > suboptimal. Everything else is just an attempt to defy reality.
>
> As the wheather was lousy, I sat down for a couple of hours and
> implemented the obviously incomplete PoC below for illustration.
>
> As the FIXME comment in tsc_early_boot_setup() tells clearly, this is far
> from complete and needs way more thoughts and scrutiny, but it's good
> enough to prove the point:
>
> [ 0.000019] tsc: Boot sched clock enabled
>
> As early as possible, but with at least the minimal safe guards in place.
>
> [ 0.000041] Linux version 7.0.0-rc5+ (tglx@tglx-pladt) (gcc (Debian 14.2.0-19) 14.2.0, GNU ld (GNU Binutils for Debian) 2.44) #893 SMP
> PREEMPT_DYNAMIC Sun Mar 29 21:38:08 CEST 2026
> [ 0.000043] Command line: BOOT_IMAGE=/boot/vmlinuz-7.0.0-rc5+ root=UUID=fa44d501-1716-402a-8602-6315c3166f7b ro console=tty0
> console=ttyS0,115200n8 ftrace_dump_on_oops earlyprintk=ttyS0,115200n8 tsc_early_khz=2095070 no-kvmclock
>
> 1) tsc_early_khz is required because KVM does not provide the frequency
> leaf.
>
> 2) no-kvmclock is required because KVM thinks it is more important than
> everything else.
>
> Both issues are completely unrelated to the problem at hand.
I don't really want to add more complexity to the boot code, or complicate any other timing
mechanisms in the kernel, just to instrument a few early printks.
Adding additional code, command line variables, etc. is antithetical to the goal of reducing boot time
and reducing technical debt.
The instrumentation that I want to add is for printks that are currently showing as zero, while
also allowing to get useful timing data on temporary printks in that early region of boot.
>
> [ 0.009415] BIOS-provided physical RAM map:
> [ 0.009416] BIOS-e820: [mem 0x0000000000000000-0x000000000009fbff] System RAM
> ...
> [ 0.009473] printk: legacy bootconsole [earlyser0] enabled
> ...
> [ 0.021037] Hypervisor detected: KVM
>
> Without 'no-kvmclock' KVM would take over sched clock and reset sched
> clock time to 0 because KVM... Trivial problem to be solved and left as
> an exercise for the reader.
>
> [ 0.021256] last_pfn = 0x7ffdb max_arch_pfn = 0x400000000
> [ 0.051429] tsc: Fast TSC calibration using PIT
> [ 0.051699] tsc: Detected 2094.808 MHz processor
> [ 0.051990] tsc: Detected 2095.070 MHz TSC
>
> That's in tsc_early_init() which enables the sched clock static key.
>
> [ 0.052871] e820: update [mem 0x00000000-0x00000fff] System RAM ==> device reserved
>
> Unsurprisingly time continues without a gap, jumping backwards or whatever.
That's not a problem I care about.
>
> If you want to have early timestamps then you have to go through the low
> level code on every architecture and do the proper sanity checks,
> enumerations, etc. no matter what. There is no guarantee that you can
> use any clock unconditionally during very early boot.
For the TSC on x86_64 and cntvct_el0 on arm64, I think you are incorrect.
In those architectures I believe you are guaranteed that the instruction to
get the cycle counter will work to retrieve a cycles value before any initialization
code in the kernel, even under emulation or virtualization. But even if not, I do not
believe it matters (see below).
Now, in order to get the clock frequency (and thus convert
to a units/sec timestamp value), additional work may be required (as shown in your POC)
OR you need some mechanism to provide the frequency to the kernel
(such as a command line option or kernel config parameter).
However, all this is moot.
I believe if a platform developer can't guarantee that the clock they decide
to use for this will be functional when the kernel starts (because either the
hardware or the pre-kernel firmware doesn't start it automatically before
the kernel loads), then the developer should not use my proposed early
times feature.
I don't think this feature has any value for regular production kernels,
and it is only intended to be useful for boot-time researchers and developers.
Indeed, it's probably better if it is left turned off for production or distro kernels.
>
> As you have to do that anyway, there is _zero_ justification for an
> extra facility side stepping sched clock, which would just create
> another pile of technical debt.
I think this POC adds much more complexity and technical debt
than any version of my early-times patch.
Since you want to get rid of technical debt on the use of get_cycles(),
I'll re-work the patch to eliminate use of that function, and revert
to something similar to my earlier version of the patch that was less
complicated.
>
> Quite the contrary, I'm going to get rid of technical debt:
>
> The get_cycles() related changes in tsc.h are going to end up in a
> obviously revised formal patch tomorrow as there is exactly _zero_
> requirement to provide this as a functional interface.
>
> 1) The default implementation in asm-generic returns 0
>
> Ergo any code depending on a functional implementation is broken
> by definition.
>
> 2) The ops/cycles metrics are as bogus as the infamous BogoMIPS metrics
>
> The "cycle" counter runs on almost all contemporary CPUs at a fixed
> frequency, which is completely unrelated to the actual CPU
> frequency and therefore to the actual CPU cycles.
>
> The only realistic metric is ops/sec and nothing else and that can
> be trivially achieved by using the generic time getter interfaces.
>
> Those might end up resulting in bogus benchmark results too if the
> underlying clocksource is jiffies, but that's again a matter of
> accepting reality.
>
> If people really mandate that ops/bogo_cycles is required for the
> very wrong reasons, then I'm happy to bring it back with a global
> name change from get_cycles() to get_bogo_cycles() which excludes
> it from any serious usage including printk.
>
> Thanks,
>
> tglx
> ---
> arch/x86/include/asm/tsc.h | 11 ++--
> arch/x86/kernel/platform-quirks.c | 2
> arch/x86/kernel/tsc.c | 96 ++++++++++++++++++++++++++++++--------
> 3 files changed, 85 insertions(+), 24 deletions(-)
>
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -76,10 +76,7 @@ extern void disable_TSC(void);
>
> static inline cycles_t get_cycles(void)
> {
> - if (!IS_ENABLED(CONFIG_X86_TSC) &&
> - !cpu_feature_enabled(X86_FEATURE_TSC))
> - return 0;
> - return rdtsc();
> + return 0;
Eliminating get_cycles() completely seems like an over-reaction to my patch.
But it sounds like something you've wanted to eliminate for a while now,
and I wouldn't stand in your way, or contribute to the technical debt associated
with it by using it in my proposals. By my count there are about 80 current users of
get_cycles() outside of arch-specific code, in the 7.0-rc6 kernel.
> }
> #define get_cycles get_cycles
>
> @@ -114,6 +111,12 @@ static inline void tsc_verify_tsc_adjust
> static inline void check_tsc_sync_target(void) { }
> #endif
>
> +#if defined(CONFIG_X86_TSC) && defined(CONFIG_X86_64)
> +void tsc_early_boot_setup(void);
> +#else
> +static inline void tsc_early_boot_setup(void) { }
> +#endif
> +
> extern int notsc_setup(char *);
> extern void tsc_save_sched_clock_state(void);
> extern void tsc_restore_sched_clock_state(void);
> --- a/arch/x86/kernel/platform-quirks.c
> +++ b/arch/x86/kernel/platform-quirks.c
> @@ -32,6 +32,8 @@ void __init x86_early_init_platform_quir
>
> if (x86_platform.set_legacy_features)
> x86_platform.set_legacy_features();
> +
> + tsc_early_boot_setup();
> }
>
> bool __init x86_pnpbios_disabled(void)
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -18,6 +18,7 @@
> #include <linux/static_call.h>
>
> #include <asm/cpuid/api.h>
> +#include <asm/cmdline.h>
> #include <asm/hpet.h>
> #include <asm/timer.h>
> #include <asm/vgtod.h>
> @@ -48,6 +49,7 @@ EXPORT_SYMBOL(tsc_khz);
> */
> static int __read_mostly tsc_unstable;
> static unsigned int __initdata tsc_early_khz;
> +static bool tsc_early_boot_enabled __ro_after_init;
>
> static DEFINE_STATIC_KEY_FALSE_RO(__use_tsc);
>
> @@ -202,12 +204,12 @@ static void set_cyc2ns_scale(unsigned lo
> /*
> * Initialize cyc2ns for boot cpu
> */
> -static void __init cyc2ns_init_boot_cpu(void)
> +static void __init cyc2ns_init_boot_cpu(unsigned long khz)
> {
> struct cyc2ns *c2n = this_cpu_ptr(&cyc2ns);
>
> seqcount_latch_init(&c2n->seq);
> - __set_cyc2ns_scale(tsc_khz, smp_processor_id(), rdtsc());
> + __set_cyc2ns_scale(khz, smp_processor_id(), rdtsc());
> }
>
> /*
> @@ -231,17 +233,24 @@ static void __init cyc2ns_init_secondary
> }
> }
>
> +static __always_inline u64 __native_sched_clock(void)
> +{
> + u64 tsc_now = rdtsc();
> +
> + /* return the value in ns */
> + return __cycles_2_ns(tsc_now);
> +}
> +
> /*
> * Scheduler clock - returns current time in nanosec units.
> */
> noinstr u64 native_sched_clock(void)
> {
> - if (static_branch_likely(&__use_tsc)) {
> - u64 tsc_now = rdtsc();
> + if (static_branch_likely(&__use_tsc))
> + return __native_sched_clock();
>
> - /* return the value in ns */
> - return __cycles_2_ns(tsc_now);
> - }
> + if (tsc_early_boot_enabled)
> + return __native_sched_clock();
>
> /*
> * Fall back to jiffies if there's no TSC available:
> @@ -371,12 +380,12 @@ static u64 tsc_read_refs(u64 *p, int hpe
> int i;
>
> for (i = 0; i < MAX_RETRIES; i++) {
> - t1 = get_cycles();
> + t1 = rdtsc();
> if (hpet)
> *p = hpet_readl(HPET_COUNTER) & 0xFFFFFFFF;
> else
> *p = acpi_pm_read_early();
> - t2 = get_cycles();
> + t2 = rdtsc();
> if ((t2 - t1) < thresh)
> return t2;
> }
> @@ -468,13 +477,13 @@ static unsigned long pit_calibrate_tsc(u
> outb(latch & 0xff, 0x42);
> outb(latch >> 8, 0x42);
>
> - tsc = t1 = t2 = get_cycles();
> + tsc = t1 = t2 = rdtsc();
>
> pitcnt = 0;
> tscmax = 0;
> tscmin = ULONG_MAX;
> while ((inb(0x61) & 0x20) == 0) {
> - t2 = get_cycles();
> + t2 = rdtsc();
> delta = t2 - tsc;
> tsc = t2;
> if ((unsigned long) delta < tscmin)
> @@ -553,9 +562,9 @@ static inline int pit_expect_msb(unsigne
> if (!pit_verify_msb(val))
> break;
> prev_tsc = tsc;
> - tsc = get_cycles();
> + tsc = rdtsc();
> }
> - *deltap = get_cycles() - prev_tsc;
> + *deltap = rdtsc() - prev_tsc;
> *tscp = tsc;
>
> /*
> @@ -745,19 +754,23 @@ unsigned long native_calibrate_tsc(void)
>
> static unsigned long cpu_khz_from_cpuid(void)
> {
> - unsigned int eax_base_mhz, ebx_max_mhz, ecx_bus_mhz, edx;
> + struct cpuid_regs regs;
>
> - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> + /* Do it manually as this can be called _before_ boot_cpu_data is initialized */
> + cpuid_leaf(0, ®s);
> + /* Intel only */
> + if (regs.ebx != 0x756e6547 && regs.ecx != 0x6c65746e && regs.edx != 0x49656e69)
> return 0;
>
> - if (boot_cpu_data.cpuid_level < CPUID_LEAF_FREQ)
> + if (regs.eax < CPUID_LEAF_FREQ)
> return 0;
>
> - eax_base_mhz = ebx_max_mhz = ecx_bus_mhz = edx = 0;
> + memset(®s, 0, sizeof(®s));
>
> - cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx_max_mhz, &ecx_bus_mhz, &edx);
> + cpuid_leaf(CPUID_LEAF_FREQ, ®s);
>
> - return eax_base_mhz * 1000;
> + /* EAX contains base MHz */
> + return regs.eax * 1000;
> }
>
> /*
> @@ -1512,8 +1525,9 @@ static void __init tsc_enable_sched_cloc
>
> /* Sanitize TSC ADJUST before cyc2ns gets initialized */
> tsc_store_and_check_tsc_adjust(true);
> - cyc2ns_init_boot_cpu();
> + cyc2ns_init_boot_cpu(tsc_khz);
> static_branch_enable(&__use_tsc);
> + tsc_early_boot_enabled = false;
> }
>
> void __init tsc_early_init(void)
> @@ -1576,6 +1590,48 @@ void __init tsc_init(void)
> detect_art();
> }
>
> +/* Only 64bit guarantees that CPUID is available */
> +#ifdef CONFIG_X86_64
> +void __init tsc_early_boot_setup(void)
> +{
> + struct cpuid_regs regs;
> + unsigned long khz = 0;
> + char optstr[64];
> +
> + if (cmdline_find_option_bool(boot_command_line, "notsc"))
> + return;
> +
> + // FIXME: This needs way more sanity checks vs. virt etc.
> + if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
> + return;
> +
> + // Why don't we have proper leaf structs yet?
> + cpuid_leaf(0, ®s);
> + if (regs.eax < 1)
> + return;
> +
> + /* TSC enabled in CPUID? */
> + cpuid_leaf(1, ®s);
> + if (!(regs.edx & BIT(4)))
> + return;
> +
> + if (cmdline_find_option(boot_command_line, "tsc_early_khz", optstr, sizeof(optstr)) > 0) {
> + // Shut up must check by doing a pointless zeroing
> + if (kstrtoul(optstr, 10, &khz) < 0)
> + khz = 0;
> + }
> +
> + if (!khz)
> + khz = cpu_khz_from_cpuid();
> + if (!khz)
> + return;
> +
> + cyc2ns_init_boot_cpu(khz);
> + tsc_early_boot_enabled = true;
> + pr_info("Boot sched clock enabled\n");
> +}
> +#endif
> +
> #ifdef CONFIG_SMP
> /*
> * Check whether existing calibration data can be reused.
Overall, this code achieves the result of initializing clocks on some
x86_64 platforms earlier (and moreso when properly configured via the
command line). This achieves a lot of the value I was looking for.
Compared to my solution, it puts pre-timekeeping timestamps
into the same time sequence space as subsequent "regular" ones.
However, I don't think the discontinuity is actually a big problem.
This PoC does an admirable job of addressing the problem space I am interested in,
but at the expense of adding more complexity to the boot sequence.
At the same time, it still leaves a blind spot in the kernel bootup sequence.
The blind spot is much shorter, but still exists. And the PoCpatch is unfinished,
and supports only one architecture. I wouldn't ask someone else to finish
it for me, nor am I willing to commit to extend it (e.g. to finalize support
for AMD processors) or maintain it going forward.
The discontinuity in timestamps is a tradeoff I'm willing to make in order keep
the code simple and to cover more of the blind sport.
I prefer my original approach. I'll send an updated patch for consideration soon.
Thanks,
-- Tim
^ permalink raw reply [flat|nested] 4+ messages in thread