On Thu, Apr 14, 2022, Anton Romanov wrote:
> On Thu, Apr 14, 2022 at 12:33 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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
