Re: [PATCH 04/18] Make cpu_tsc_khz updates use local CPU

[Zachary Amsden]

On 07/18/2010 04:45 AM, Avi Kivity wrote:
> On 07/13/2010 05:25 AM, Zachary Amsden wrote:
> > This simplifies much of the init code; we can now simply always
> > call tsc_khz_changed, optionally passing it a new value, or letting
> > it figure out the existing value (while interrupts are disabled, and
> > thus, by inference from the rule, not raceful against CPU hotplug or
> > frequency updates, which will issue IPIs to the local CPU to perform
> > this very same task).
> >
> >
> > @@ -893,6 +893,15 @@ static void kvm_set_time_scale(uint32_t tsc_khz, 
> > struct pvclock_vcpu_time_info *
> >
> >   static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
> >
> > +static inline int kvm_tsc_changes_freq(void)
> > +{
> > +    int cpu = get_cpu();
> > +    int ret = !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)&&
> > +          cpufreq_quick_get(cpu) != 0;
> > +    put_cpu();
> > +    return ret;
> > +}
> > +
> >   void guest_write_tsc(struct kvm_vcpu *vcpu, u64 data)
> >   {
> >       struct kvm *kvm = vcpu->kvm;
> > @@ -937,7 +946,7 @@ void guest_write_tsc(struct kvm_vcpu *vcpu, u64 
> > data)
> >   }
> >   EXPORT_SYMBOL_GPL(guest_write_tsc);
> >
> > -static void kvm_write_guest_time(struct kvm_vcpu *v)
> > +static int kvm_write_guest_time(struct kvm_vcpu *v)
> >   {
> >       struct timespec ts;
> >       unsigned long flags;
> > @@ -946,24 +955,27 @@ static void kvm_write_guest_time(struct 
> > kvm_vcpu *v)
> >       unsigned long this_tsc_khz;
> >
> >       if ((!vcpu->time_page))
> > -        return;
> > -
> > -    this_tsc_khz = get_cpu_var(cpu_tsc_khz);
> > -    if (unlikely(vcpu->hv_clock_tsc_khz != this_tsc_khz)) {
> > -        kvm_set_time_scale(this_tsc_khz,&vcpu->hv_clock);
> > -        vcpu->hv_clock_tsc_khz = this_tsc_khz;
> > -    }
> > -    put_cpu_var(cpu_tsc_khz);
> > +        return 0;
> >
> >       /* Keep irq disabled to prevent changes to the clock */
> >       local_irq_save(flags);
> >       kvm_get_msr(v, MSR_IA32_TSC,&vcpu->hv_clock.tsc_timestamp);
> >       ktime_get_ts(&ts);
> >       monotonic_to_bootbased(&ts);
> > +    this_tsc_khz = __get_cpu_var(cpu_tsc_khz);
> >       local_irq_restore(flags);
> >
> > -    /* With all the info we got, fill in the values */
> > +    if (unlikely(this_tsc_khz == 0)) {
> > +        kvm_make_request(KVM_REQ_KVMCLOCK_UPDATE, v);
> > +        return 1;
> > +    }
>
> Presumably, this will spin until cpufreq writes the frequency?

Only during CPU re-add; we can only get here if running before cpu 
notifiers have told us about the new CPU.

> > @@ -4131,9 +4138,23 @@ int kvm_fast_pio_out(struct kvm_vcpu *vcpu, 
> > int size, unsigned short port)
> >   }
> >   EXPORT_SYMBOL_GPL(kvm_fast_pio_out);
> >
> > -static void bounce_off(void *info)
> > +static void tsc_bad(void *info)
> > +{
> > +    __get_cpu_var(cpu_tsc_khz) = 0;
> > +}
> > +
> > +static void tsc_khz_changed(void *data)
> >   {
> > -    /* nothing */
> > +    struct cpufreq_freqs *freq = data;
> > +    unsigned long khz = 0;
> > +
> > +    if (data)
> > +        khz = freq->new;
> > +    else if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> > +        khz = cpufreq_quick_get(raw_smp_processor_id());
> > +    if (!khz)
> > +        khz = tsc_khz;
> > +    __get_cpu_var(cpu_tsc_khz) = khz;
> >   }
>
> Do we really need to cache cpufreq_quick_get()?  If it's really quick, 
> why not just use it everywhere instead of cacheing it?  Not a comment 
> on this patch.

If cpufreq is compiled in, but disabled, it returns zero, so we need 
some sort of logic.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/