[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/2] xen/x86: don't send IPI to sync TSC when it is reliable
On 08.07.2025 20:32, Stefano Stabellini wrote: > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -2297,11 +2297,7 @@ static void cf_check time_calibration(void *unused) > }; > > if ( clocksource_is_tsc() ) > - { > - local_irq_disable(); > - r.master_stime = read_platform_stime(&r.master_tsc_stamp); > - local_irq_enable(); > - } > + return; Assuming the rendezvous can indeed be entirely skipped, I agree that there's no point calling read_platform_stime() here. Yet to yield a consistent result, more changes are then necessary imo: - as indicated before, the invocation of this function from verify_tsc_reliability() when plt_tsc was chosen is then entirely pointless, - time_calibration_nop_rendezvous() would then apparently want purging, not the least to make clear that TIME_CALIBRATE_SOFTIRQ is never raised in this mode (one of your goals after all, aiui), - the function being a timer handler, it would be preferable if the timer wasn't ever activated in this mode (at which point rather than returning early, the code above could simply be purged, maybe replaced by e.g. an assertion), - the above in particular requires dealing with cpu_frequency_change() (the other of the two places where the timer is actually activated). Some care may be needed in all of this taking into consideration that the platform timer change to TSC happens late. Albeit commit f954a1bf5f74 ("x86/time: change initiation of the calibration timer") has imo eliminated the main concern here. As to skipping the rendezvous: Besides invoking the calibration softirq, time_calibration_nop_rendezvous() also updates the per-CPU cpu_calibration fields. There would thus need to be a pretty formal proof that calculations involving ->local_stime or ->local_tsc can't possibly degrade or even degenerate when they remain at their boot-time values. (As to ->master_stime, afaict the field simply isn't used at all in that mode, which is a fair part of the reason why the code change above is okay _if_ the rendezvous itself can be eliminated. The justification for that could also do with extending some, considering that much of the involved code is pretty subtle.) Alternatively, if such a proof turned out impossible, another way of updating the fields every once in a while would need adding. Finally, what you do here isn't entirely reliable as to your apparent end goal: "clocksource=tsc" is respected only when tsc_check_reliability() completes with an acceptable outcome. There's certainly some variability in this across multiple runs, i.e. if things went extremely bad, once in blue moon you may end up with the TSC being rejected for use as platform timer. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |