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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2026 12:02, Roger Pau Monné wrote:
> 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();

Yes, this would prevent stime going backwards. But at the same time it
would latch the imprecise calculations done initially. The farther off
the initial scale (e.g. when using "high" in case "nominal" isn't
available, as discussed previously), the bigger the error. That may be
mainly (only?) a cosmetic thing, but still.

Actually, if we did that, what would prevent stime going backwards by
the update done in init_percpu_time()? Aiui we really want .local_stime
to be written there for the very first time.

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

That function is ripe for removal anyway, so I'm hesitant to derive
anything from it.

Jan