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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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.)

Jan