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.
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
