Hi Pavel,
At 11/15/2017 05:46 AM, Pavel Tatashin wrote:
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.
Indeed.
>> + tsc_early_disable();
>> + __sched_clock_offset = sched_clock_early() - sched_clock();
>> + pr_info("sched clock early is finished, offset[%lld.%09lds]\n", t, r);
>> +}
s/sched clock early/early sched clock/
>> +#endif /* CONFIG_X86_TSC */
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
Agree with removing #ifdef CONFIG_X86_TSC
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.
Yes, the function names confused me, the actually purpose of
tsc_early_fini() is switching schedule clock to the final one. right?
How about replacing *return;* with *goto final_sched_clock;*
Thanks,
dou.
BTW, seems you forgot to cc Peter Zijlstra <pet...@infradead.org> 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
