Re: [PATCH v2 RFC] x86/time: avoid early uses of NOW() to return zero

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 18, 2026 at 10:05:41AM +0200, Jan Beulich wrote:
> On 15.05.2026 15:12, Roger Pau Monné wrote:
> > On Fri, May 15, 2026 at 09:15:40AM +0200, Jan Beulich wrote:
> >> On 14.05.2026 17:56, Roger Pau Monné wrote:
> >>> On Wed, May 13, 2026 at 08:44:46AM +0200, Jan Beulich wrote:
> >>>> @@ -2623,6 +2640,21 @@ int __init init_xen_time(void)
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> +/* BSP-only function to pre-set an approximate TSC scale. */
> >>>> +void __init preset_tsc_scale(unsigned long freq)
> >>>> +{
> >>>> +    struct cpu_time *t = &this_cpu(cpu_time);
> >>>> +
> >>>> +    /*
> >>>> +     * The incoming frequency is only approximate (nominal).  Increase 
> >>>> it by
> >>>> +     * 1% to make NOW() output rather a little too slow than too fast, 
> >>>> thus
> >>>> +     * avoiding a possible backwards jump once the final scale is set.
> >>>> +     */
> >>>> +    freq += DIV_ROUND_UP(freq, 100);
> >>>
> >>> To avoid such possible jump backwards, won't it safer to also update
> >>> the ->local_stime and ->local_tsc fields at the time the new scale is
> >>> set?  Updatign those ahead of setting the new scale should avoid any
> >>> backward jumps.
> >>
> >> ->stamp.local_tsc does get updated; you merely dropped that line from reply
> >> context. As to local_stime - how could we possibly set that, when we didn't
> >> get through init_platform_timer() yet? Leaving it at 0 is the correct
> >> match for setting local_tsc to boot_tsc_stamp.
> > 
> > Please bear with me, maybe I'm not understanding exactly to what the
> > code comment refers to as "possible backwards jump once the final
> > scale is set".  I assume you refer to the setting of scale
> > early_time_init()?  The ->stamp.local_tsc value also gets updated at
> > that point, so it's not possible for the timer going backwards?
> 
> It is updated there, but only to boot_tsc_stamp. I.e. no change at all
> if preset_tsc_scale() set the field already.

Couldn't we do the following in early_init_time() to ensure time
doesn't go backwards:

    if ( t->tsc_scale.mul_frac )
    {
        /*
         * Update time snapshot to ensure time doesn't go backwards as a
         * result of the scale change done below.
         */
        t->stamp.local_tsc = rdtsc_ordered();
        t->stamp.local_stime = get_s_time_fixed(t->stamp.local_tsc);
    }
    else
        t->stamp.local_tsc = boot_tsc_stamp;

    set_time_scale(&t->tsc_scale, tmp);
    init_percpu_time();

That's kind of the same logic that's used in cpu_frequency_change()
ahead of calling set_time_scale().

> > This changed with the addition of the init_percpu_time() call in
> > early_time_init(), and makes the setting of "t->stamp.local_tsc =
> > boot_tsc_stamp" pointless, as it will get overwritten by the logic in
> > init_percpu_time() a couple of lines after?
> 
> When making these changes, I first thought so too. But no, that write
> isn't pointless: In case preset_tsc_scale() wasn't called, leaving the
> field at 0 would break the use of get_s_time_fixed() out of
> init_percpu_time(). (Iirc I only noticed this because of having put
> debug printk()s there for other purposes.)

I see, yes, that would be needed.

Regards, Roger.