diff mbox

[PATCH:,2/6] Kill the confusing tsc_ref_khz and ref_freq variables.

Message ID 1253762945-5750-2-git-send-email-zamsden@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zachary Amsden Sept. 24, 2009, 3:29 a.m. UTC 
They are globals, not clearly protected by any ordering or locking, and
vulnerable to various startup races.

Instead, for variable TSC machines, register the cpufreq notifier and get
the TSC frequency directly from the cpufreq machinery.  Not only is it
always right, it is also perfectly accurate, as no error prone measurement
is required.  On such machines, also detect the frequency when bringing
a new CPU online; it isn't clear what frequency it will start with, and
it may not correspond to the reference.

Signed-off-by: Zachary Amsden <zamsden@redhat.com>
---
 arch/x86/kvm/x86.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)

Comments

Marcelo Tosatti Sept. 24, 2009, 3:10 p.m. UTC | #1 
On Wed, Sep 23, 2009 at 05:29:01PM -1000, Zachary Amsden wrote:
> They are globals, not clearly protected by any ordering or locking, and
> vulnerable to various startup races.
> 
> Instead, for variable TSC machines, register the cpufreq notifier and get
> the TSC frequency directly from the cpufreq machinery.  Not only is it
> always right, it is also perfectly accurate, as no error prone measurement
> is required.  On such machines, also detect the frequency when bringing
> a new CPU online; it isn't clear what frequency it will start with, and
> it may not correspond to the reference.
> 
> Signed-off-by: Zachary Amsden <zamsden@redhat.com>
> ---
>  arch/x86/kvm/x86.c |   38 ++++++++++++++++++++++++++++----------
>  1 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 15d2ace..35082dd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -650,6 +650,19 @@ 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 void kvm_get_cpu_khz(int cpu)
> +{
> +	unsigned int khz = cpufreq_get(cpu);

cpufreq_get does down_read, while kvm_arch_hardware_enable is called
either with a spinlock held or from interrupt context?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zachary Amsden Sept. 25, 2009, 12:47 a.m. UTC | #2 
Simplified the patch series a bit and fixed some bugs noticed by Marcelo.
Axed the hot-remove notifier (was not needed), fixed a locking bug by
using cpufreq_quick_get, fixed another bug in kvm_cpu_hotplug that was
filtering out online notifications when KVM was loaded but not in use.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 15d2ace..35082dd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -650,6 +650,19 @@  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 void kvm_get_cpu_khz(int cpu)
+{
+	unsigned int khz = cpufreq_get(cpu);
+	if (khz <= 0) {
+		static int warn = 0;
+		if (warn++ < 10)
+			printk(KERN_ERR "kvm: could not get frequency for CPU "
+				        "%d.  Timers may be inaccurate\n", cpu);
+		khz = tsc_khz;
+	}
+	per_cpu(cpu_tsc_khz, cpu) = khz;
+}
+
 static void kvm_write_guest_time(struct kvm_vcpu *v)
 {
 	struct timespec ts;
@@ -3061,9 +3074,6 @@  static void bounce_off(void *info)
 	/* nothing */
 }
 
-static unsigned int  ref_freq;
-static unsigned long tsc_khz_ref;
-
 static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 				     void *data)
 {
@@ -3071,15 +3081,15 @@  static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
 	struct kvm *kvm;
 	struct kvm_vcpu *vcpu;
 	int i, send_ipi = 0;
-
-	if (!ref_freq)
-		ref_freq = freq->old;
+	unsigned long old_khz;
 
 	if (val == CPUFREQ_PRECHANGE && freq->old > freq->new)
 		return 0;
 	if (val == CPUFREQ_POSTCHANGE && freq->old < freq->new)
 		return 0;
-	per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new);
+	old_khz = per_cpu(cpu_tsc_khz, freq->cpu);
+	per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(old_khz, freq->old,
+							freq->new);
 
 	spin_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list) {
@@ -3120,12 +3130,18 @@  static void kvm_timer_init(void)
 {
 	int cpu;
 
-	for_each_possible_cpu(cpu)
-		per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
 	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
-		tsc_khz_ref = tsc_khz;
 		cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block,
 					  CPUFREQ_TRANSITION_NOTIFIER);
+		for_each_online_cpu(cpu)
+			kvm_get_cpu_khz(cpu);
+	} else {
+		for_each_possible_cpu(cpu)
+			per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
+	}
+	for_each_possible_cpu(cpu) {
+		printk(KERN_DEBUG "kvm: cpu %d = %ld khz\n",
+			cpu, per_cpu(cpu_tsc_khz, cpu));
 	}
 }
 
@@ -4698,6 +4714,8 @@  int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 
 int kvm_arch_hardware_enable(void *garbage)
 {
+	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+		kvm_get_cpu_khz(raw_smp_processor_id());
 	return kvm_x86_ops->hardware_enable(garbage);
 }