Message-ID: <871pp14pkr.ffs@tglx>
Date: Sun, 24 Aug 2025 00:53:56 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: vishnu singh <v-singh1@...com>, mark.rutland@....com, maz@...nel.org,
 catalin.marinas@....com, will@...nel.org, jstultz@...gle.com,
 sboyd@...nel.org, akpm@...ux-foundation.org, chenhuacai@...nel.org,
 pmladek@...e.com, agordeev@...ux.ibm.com, bigeasy@...utronix.de,
 urezki@...il.com, Llillian@...r-ark.net, francesco@...la.it,
 guoweikang.kernel@...il.com, alexander.shishkin@...ux.intel.com,
 rrangel@...omium.org, kpsingh@...nel.org, anna-maria@...utronix.de,
 mingo@...nel.org, frederic@...nel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH] time: introduce BOOT_TIME_TRACKER and minimal boot
 timestamp

On Sat, Aug 23 2025 at  9:49, vishnu singh wrote:
On Sat, Aug 23 2025 at 10:10, vishnu singh wrote:

Why are you sending the same thing twice within 20 minutes?

> From: Vishnu Singh <v-singh1@...com>
>
> Part of BOOT SIG Initiative,

What the heck is BOOT SIG Initiative? Are you seriously expecting me to
look this up?

Please don't provide me a random link to it because it's not relevant to
the rest of what I have to say about this.

> , This patch adds a tiny,opt-in facility to help measure kernel boot
>   duration without full tracing:

1) Any spell checker would have pointed out to you that 'This' after a
   comma is not a proper sentence and neither is 'tiny,opt-in facility'

2) You failed to read documentation:

   # git grep "This patch" Documentation/process/

3) This change log starts with the WHAT and fails completely to explain
   the WHY. See:

     https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> New CONFIG_BOOT_TIME_TRACKER in kernel/time/Kconfig.
> When enabled, the kernel logs two boot markers:
>     1. kernel entry in start_kernel()
>     2. first userspace start in kernel_init() before run_init_process()
>
> These markers are intended for post-boot parsers to compute coarse
> kernel boot time and to merge with bootloader/MCU/DSP records into
> a unified timeline.
>
> Core helper u64 boot_time_now() in kernel/time/boot_time_now.c,
> exporting a counter‑derived timestamp via small per-arch primitives.
> This series includes an initial arm64 primitive that uses CNTVCT_EL0
> as the source, other architectures can wire up equivalents.
>
> Files touched:
> kernel/time/Kconfig, kernel/time/Makefile
> kernel/time/boot_time_now.c (new core helper)
> arch/arm64/include/asm/boot_time_primitives.h (arm64 primitive)
> include/linux/boot_time_now.h (public API + IDs)
> init/main.c (print two markers)

Seriously? This can be seen from the diffstat and the patch itself.

You still fail to explain the problem you are trying to solve and
instead babble about WHAT you are doing, which means you never read the
documentation of the project which you are trying to contribute to.

Do you really think that the people who spent time on writing it, did
so just to be ignored?

> This complements U-Boot’s stashed bootstage records so a userspace tool
> can build a system-wide boot timeline across SPL, U-Boot, kernel and other
> subsystems.
>
> Reference boot-time parser utility:
>      https://github.com/v-singh1/boot-time-parser
>
> Sample boot time report:
> +--------------------------------------------------------------------+
>                  am62xx-evm Boot Time Report
> +--------------------------------------------------------------------+
> Device Power On         : 0 ms

<SNIP>

> IPC_SYNC_ALL                   =   6787 ms (+151 ms)
> +--------------------------------------------------------------------+

How are these 30 lines of useless information helpful to understand the
underlying problem?

That's what cover letters are for.

>  MAINTAINERS                                   |  3 +++
>  arch/arm64/include/asm/boot_time_primitives.h | 14 ++++++++++++++
>  include/linux/boot_time_now.h                 | 16 ++++++++++++++++
>  init/main.c                                   | 13 +++++++++++++
>  kernel/time/Kconfig                           | 10 ++++++++++
>  kernel/time/Makefile                          |  1 +
>  kernel/time/boot_time_now.c                   | 13 +++++++++++++

This does too many things at once. See Documentation.

One patch for creating the infrastructure with a proper rationale and
then one which hooks it up.

Again:

    Documentation has not been written to be ignored. RFC patches are
    not exempt from that.

> +static inline u64 arch_boot_counter_now(void)
> +{
> +	return ((arch_timer_read_cntvct_el0() * 1000000) / arch_timer_get_cntfrq());
> +}

Q: What guarantees that this timer is available and functional at this
   point?

A: Nothing

> +++ b/include/linux/boot_time_now.h

What means boot_time_now?

You couldn't come up with a less non-descriptive name, right?

> +enum kernel_bootstage_id {
> +	BOOTSTAGE_ID_KERNEL_START = 300,
> +	BOOTSTAGE_ID_KERNEL_END = 301,

Aside of the formatting (See Documentation), these are random numbers
pulled out of thin air without any explanation why they need to start at
300.

> +};
> +
> +/* Return boot time in nanoseconds using hardware counter */
> +u64 boot_time_now(void);

That's a function name which is as bad as is can be. This is about
getting an early time stamp and that needs to be properly named _AND_
encapsulated so it works universally without some magic hardware
dependency. If at all, see below.

>  #include <kunit/test.h>
>  
> +#ifdef CONFIG_BOOT_TIME_TRACKER
> +#include <linux/boot_time_now.h>
> +#endif

What's this ifdeffery for? Headers have to be written in a way that they
can be unconditionally included. IOW, put the ifdeffery into the header.

> @@ -929,6 +933,11 @@ void start_kernel(void)
>  	page_address_init();
>  	pr_notice("%s", linux_banner);
>  	setup_arch(&command_line);
> +
> +#ifdef CONFIG_BOOT_TIME_TRACKER
> +	pr_info("[BOOT TRACKER] - ID:%d, %s = %llu\n",
> +		BOOTSTAGE_ID_KERNEL_START, __func__, boot_time_now());
> +#endif

Seriously? Have you looked at all the functions invoked in this file?

Those which depend on a config have:

#ifdef CONFIG_FOO
void foo_init(void);
#else
static inline void foo_init(void) { }
#endif

in the headers to avoid this horrible #ifdef maze. No?

> diff --git a/kernel/time/boot_time_now.c b/kernel/time/boot_time_now.c
> new file mode 100644
> index 000000000000..6dc12d454be0
> --- /dev/null
> +++ b/kernel/time/boot_time_now.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: LGPL-2.0
> +
> +#include <linux/boot_time_now.h>
> +#include <asm/boot_time_primitives.h>
> +
> +u64 boot_time_now(void)
> +{
> +	return arch_boot_counter_now();
> +}
> +EXPORT_SYMBOL_GPL(boot_time_now);

Why does this need to exported for modules when the only users are
always built in?

> +
> +MODULE_DESCRIPTION("boot time tracker");
> +MODULE_LICENSE("GPL");

Why needs this a module description? This has always to be built in, no?

Copy and pasta from some boilerplate template is fine, but using brain
on what to paste is not optional.

But that's all irrelevant, because none of this is actually required in
the first place as there is existing infrastructure, which allows you to
gather most of that information already today.

Extending it to gain what you really want to achieve is trivial enough
when you actually start to look at the existing infrastructure instead
of blindly hacking some ill-defined mechanism into the kernel, which
relies on the assumption that a particular piece of hardware is
available and functional.

That assumption is not even true for ARM64 under all circumstances.

dmesg already exposes time stamps and while they might be coarse grained
until a sched clock is registered, you still can utilize that
registration:

--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -236,16 +236,14 @@ sched_clock_register(u64 (*read)(void),
 	/* Calculate the ns resolution of this counter */
 	res = cyc_to_ns(1ULL, new_mult, new_shift);
 
-	pr_info("sched_clock: %u bits at %lu%cHz, resolution %lluns, wraps every %lluns\n",
-		bits, r, r_unit, res, wrap);
+	pr_info("sched_clock: %pS: %u bits at %lu%cHz, resolution %lluns, wraps every %lluns hwcnt: %llu\n",
+		read, bits, r, r_unit, res, wrap, read());
 
 	/* Enable IRQ time accounting if we have a fast enough sched_clock() */
 	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
 		enable_sched_clock_irqtime();
 
 	local_irq_restore(flags);
-
-	pr_debug("Registered %pS as sched_clock source\n", read);
 }
 
 void __init generic_sched_clock_init(void)

That message provides you all the information you need for your pretty
printed postprocessing results by re-calculating all the other coarse
grained dmesg timestamps from there, no?

That obviously does not work on architectures which do no use the
sched_clock infrastructure. Some of them do not for a good reason, but
emitting the same information for them if anyone is interested is
trivial enough. And that's none of your problems.

If you really need some not yet existing dedicated time stamp in the
maze of dmesg, then add it unconditionlly and without introducing an
artifical subsystem which is of no value at all.

But I tell you that's not necessary at all. The points in dmesg are well
defined. Here is the relevant output on a arm64 machine:

[    0.000000] Linux version 6.17.0-rc1 ...
...
[    0.000008] sched_clock: 56 bits at 19MHz, resolution 52ns, wraps every 3579139424256ns

which is missing the actual hardware value, but see above...

So let's assume this give you

[    0.000008] sched_clock: 56 bits at 19MHz, resolution 52ns, wraps
                            every 3579139424256ns hwcnt: 19000000

Which means that the counter accumulated 19000000 increments since the
hardware was powered up, no?

So the [0.000008] timestamp happens exactly 1.0 seconds after power on.
At least to my understanding of basic math, but your favourite AI bot
might disagree with that.

So anything you need for your pretty printing boot record can be
retrieved from there without any magic 300 and 301 numbers.

Because there is another printk() which has been there forever:

[   11.651192] Run /init as init process

No?

Thanks,

        tglx

Powered by blists - more mailing lists