+Vitaly
On Thu, Apr 14, 2022, Anton Romanov wrote:
> Don't snapshot tsc_khz into per-cpu cpu_tsc_khz if the
> host TSC is constant, in which case the actual TSC frequency will never
> change and thus capturing TSC during initialization is
> unnecessary, KVM can simply use tsc_khz.
> This value is snapshotted from
> kvm_timer_init->kvmclock_cpu_online->tsc_khz_changed(NULL)
Nit, please wrap changelogs at ~75 chars. It's not a hard rule, e.g. if running
over or cutting early improves readability, then by all means. But wrapping
somewhat randomly makes reading the changelog unnecessarily difficult.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 547ba00ef64f..4ae9a03f549d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2907,6 +2907,19 @@ static void kvm_update_masterclock(struct kvm *kvm)
> kvm_end_pvclock_update(kvm);
> }
>
> +/*
> + * If kvm is built into kernel it is possible that tsc_khz saved into
> + * per-cpu cpu_tsc_khz was yet unrefined value. If CPU provides CONSTANT_TSC it
> + * doesn't make sense to snapshot it anyway so just return tsc_khz
> + */
> +static unsigned long get_cpu_tsc_khz(void)
> +{
> + if (static_cpu_has(X86_FEATURE_CONSTANT_TSC))
> + return tsc_khz;
> + else
> + return __this_cpu_read(cpu_tsc_khz);
> +}
> +
> /* Called within read_seqcount_begin/retry for kvm->pvclock_sc. */
> static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
> {
> @@ -2917,7 +2930,7 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
> get_cpu();
>
> data->flags = 0;
> - if (ka->use_master_clock && __this_cpu_read(cpu_tsc_khz)) {
> + if (ka->use_master_clock && get_cpu_tsc_khz()) {
It might make sense to open code this to make it more obvious why the "else" path
exists. That'd also eliminate a condition branch on CPUs with a constant TSC,
though I don't know if we care that much about the performance here.
if (ka->use_master_clock &&
(static_cpu_has(X86_FEATURE_CONSTANT_TSC) || __this_cpu_read(cpu_tsc_khz)))
And/or add a comment about cpu_tsc_khz being zero when the CPU is being offlined?
> #ifdef CONFIG_X86_64
> struct timespec64 ts;
>
...
> @@ -8646,9 +8659,12 @@ static void tsc_khz_changed(void *data)
> struct cpufreq_freqs *freq = data;
> unsigned long khz = 0;
>
> + if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> + return;
Vitaly,
The Hyper-V guest code also sets cpu_tsc_khz, should we WARN if that notifier is
invoked and Hyper-V told us there's a constant TSC?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ab336f7c82e4..ca8e20f5ffc0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8701,6 +8701,8 @@ static void kvm_hyperv_tsc_notifier(void)
struct kvm *kvm;
int cpu;
+ WARN_ON_ONCE(boot_cpu_has(X86_FEATURE_CONSTANT_TSC));
+
mutex_lock(&kvm_lock);
list_for_each_entry(kvm, &vm_list, vm_list)
kvm_make_mclock_inprogress_request(kvm);
