Re: [PATCH] sched_clock: Add option to use absolute time against hardware clock reset

[Marc Zyngier]

On Mon, 07 Sep 2026 10:05:40 +0100,
Feng Tang <feng.tang@xxxxxxxxxxxxxxxxx> wrote:
> 
> On Sat, Sep 05, 2026 at 10:39:26PM +0200, Thomas Gleixner wrote:
> > On Wed, Sep 02 2026 at 16:21, Feng Tang wrote:
> > > Locally, it did help on chasing some RAS issues which needed cooperation
> > > between kernel, SCP firmware and ATF, by mapping the actions from each
> > > players into one timeline based on the timestamps in their logs.
> > 
> > I told you before that we don't care about your bug chasing war stories
> > at all. Educate your firmware people and stop pestering us with your
> > firmware debug hacks.
> 
> Hi Thomas,
> 
> Yes, this patch is hacky.
> 
> From the reply of Marc in this thread:
> 
> "
> I really have to ask: why isn't this just a one-off sampling of the
> counter, kept in some user accessible location (debugfs or something
> else), and ultimately post-processed to align your logs? People have
> been doing this... forever, and that has been "good enough" so far.
> "
> 
> IIUC, getting an absolute timestamp for logs is still practical in some
> cases. How about adding the offset-since-reset info into the message
> like Marc suggested:
> 
> ---
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index f3aaef695b8c..d5a54276b49d 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -176,7 +176,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>  
>  void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>  {
> -	u64 res, wrap, new_mask, new_epoch, cyc, ns;
> +	u64 res, wrap, new_mask, new_epoch, cyc, ns, reset_ns;
>  	u32 new_mult, new_shift;
>  	unsigned long r, flags;
>  	char r_unit;
> @@ -235,8 +235,11 @@ void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>  	/* 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);
> +	/* Calculate the time since last counter resetting to 0 */
> +	reset_ns = mul_u64_u64_div_u64(new_epoch, NSEC_PER_SEC, rate);
> +
> +	pr_info("sched_clock: %u bits at %lu%cHz, resolution %lluns, wraps every %lluns, has run %lluns since counter reset\n",
> +		bits, r, r_unit, res, wrap, reset_ns);
>  
>  	/* Enable IRQ time accounting if we have a fast enough sched_clock() */
>  	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
> 

Which is what people have done locally since the beginning of times.
And the only time this is actually useful is when bringing up new
HW/FW that is broken.

So why the need to put that in an upstream kernel, instead of being as
part of your debug toolbox? I don't think the kernel shouldn't be the
recipient of this sort of stuff.

	M.

-- 
Without deviation from the norm, progress is not possible.