Re: [PATCH v8 6/6] x86/tsc: use tsc early

[Pavel Tatashin]

Hi Dou,

Great comments, my replies below:

>>  static inline unsigned long long paravirt_sched_clock(void)
>>  {
>> -       return PVOP_CALL0(unsigned long long, pv_time_ops.sched_clock);
>> +       return PVOP_CALL0(unsigned long long,
>> pv_time_ops.active_sched_clock);
>>  }
>>
>
> Should in the 5th patch

Actually, it has to be in patch 6, because otherwise patch 5 without
patch 6 would cause native_sched_clock() to be used even when a
platform specific clock is set, thus may cause performance regressions
where it is not optimal to use tsc for clock.

> Add definitions for the situation of X86_TSC = no :
>
> #else /* CONFIG_X86_TSC */
> static inline void tsc_early_init(unsigned int khz) { }
> static inline void tsc_early_fini(void) { }

Excellent point, I totally forgot about  X86_TSC = no, however, a
better fix is to simply remove #ifdef CONFIG_X86_TSC from my
functions. Apparently, even with X86_TSC=no we can use TSC unless
notsc kernel parameter is passed. This will be in the next patchset.

>
> According to tsc_early_delay_calibrate(), if (!boot_cpu_has(X86_FEATURE_TSC
> || !tsc_khz ), tsc_early_init(tsc_khz)
> will never be called, so here is  redundant.
>
>>                 return;
>>         }
>>
>> @@ -1302,6 +1385,7 @@ void __init tsc_init(void)
>>         if (!tsc_khz) {
>>                 mark_tsc_unstable("could not calculate TSC khz");
>>                 setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
>> +               tsc_early_fini();
>
>
> ditto

Right, in both case we still want to call tsc_early_fini(). Because,
it calls tsc_early_disable() even when tsc_early_init() was never
called.  tsc_early_disable()  either sets static branch
__tsc_early_static to false or changes active_sched_clock to be
platform specific, depending on CONFIG_PARAVIRT.

> BTW, seems you forgot to cc Peter Zijlstra <peterz@xxxxxxxxxxxxx> in both V7
> and V8 patchsets.

Thank you for noticing this! I will include Peter when I send out
patchset version 9.

Thank you,
Pavel