Re: [PATCH] KVM: x86: Use current rather than snapshotted TSC frequency if it is constant

[Sean Christopherson <seanjc@google.com>]

On Thu, Apr 14, 2022, Anton Romanov wrote:
> On Thu, Apr 14, 2022 at 12:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > On Thu, Apr 14, 2022, Anton Romanov wrote:
> > >  /* 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?
> 
> It looks like cpu_tsc_khz being zero is used as an indicator of CPU
> being unplugged here.

That's ok, the unplug issue was that kvm_get_time_scale() got stuck in an infinite
loop due to cpu_tsc_khz being zero.  Using tsc_khz is ok even though the CPU is
about to go offline, the CPU going offline doesn't make the calculation wrong.

> I don't think your proposed change is right in this case either.
> How about we still keep tsc_khz_changed untouched as well as this line?
> Potentially adding here a comment that on this line we only read it to
> see if CPU is not being unplugged yet