[PATCH 0/3] Sync TSC for guest when vcpu has been migrated to another cpu

[PATCH 0/3] Sync TSC for guest when vcpu has been migrated to another cpu

[Leonard Norrgard]

When a vcpu is migrated to another cpu, an unexpected difference in
current and previous cpu TSC values can cause a guest to wait
indefinitely. A known workaround has been to run kvm with taskset,
locking it to a single cpu. This patch series modifies the guest TSC
offset as needed to guarantee that the guest sees a monotonically
increasing TSC, adds a statistic for vcpu_migrated and updates kvm_stat
so the first column isn't truncated.

[1/3] Sync guest viewable TSC when vcpu migrated
[2/3] Add a vcpu_migrated statistic
[3/3] Widen key column in kvm_stat

-- Leonard

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH 1/3] Sync guest viewable TSC when vcpu migrated

[Leonard Norrgard]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

When a vcpu is migrated to another cpu, an unexpected difference in
current and previous cpu TSC values can cause a guest to wait
indefinitely. A known workaround has been to run kvm with taskset,
locking it to a single cpu. This patch modifies the guest viewable TSC
so that the TSC is guaranteed to be monotonically increasing.

Thanks to Avi Kivity for help in pinpointing this issue and advice for
fixing it.

Signed-off-by: Leonard Norrgård <vinsci-g2GXA8XeJSExHbG02/KK1g@public.gmane.org>


[-- Attachment #2: 01_sync_tsc_when_vcpu_migrated.patch --]
[-- Type: text/x-diff, Size: 2286 bytes --]

Index: linux-2.6/drivers/kvm/svm.c
===================================================================
--- linux-2.6.orig/drivers/kvm/svm.c	2007-03-02 17:29:57.000000000 +0200
+++ linux-2.6/drivers/kvm/svm.c	2007-03-02 17:31:15.000000000 +0200
@@ -598,9 +598,45 @@
 	kfree(vcpu->svm);
 }
 
+#ifdef CONFIG_SMP
+static void ipi_rdtscll(void *arg)
+{
+	u64 *tsc = arg;
+	rdtscll(*tsc);
+}
+#endif
+
+/*
+ * Switches to specified vcpu, until a matching vcpu_put(), but assumes
+ * vcpu mutex is already taken.
+ */
 static struct kvm_vcpu *svm_vcpu_load(struct kvm_vcpu *vcpu)
 {
-	get_cpu();
+	int cpu;
+
+	cpu = get_cpu();
+
+#ifdef CONFIG_SMP
+	if (vcpu->cpu != cpu) {
+		if (vcpu->cpu != -1) {
+			u64 tsc_this, tsc_previous;
+
+			/* Get TSC value for this and the previous cpu. */
+			rdtscll(tsc_this);
+			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
+						 &tsc_previous, 0, 1);
+
+			/*
+			 * Make sure that the guest sees a monotonically
+			 * increasing TSC.
+			 */
+			vcpu->svm->vmcb->control.tsc_offset +=
+						tsc_previous - tsc_this;
+		}
+	}
+#endif
+
+	vcpu->cpu = cpu;
 	return vcpu;
 }
 
Index: linux-2.6/drivers/kvm/vmx.c
===================================================================
--- linux-2.6.orig/drivers/kvm/vmx.c	2007-03-02 17:30:01.000000000 +0200
+++ linux-2.6/drivers/kvm/vmx.c	2007-03-02 17:30:17.000000000 +0200
@@ -200,6 +200,14 @@
 #endif
 }
 
+#ifdef CONFIG_SMP
+static void ipi_rdtscll(void *arg)
+{
+	u64 *tsc = arg;
+	rdtscll(*tsc);
+}
+#endif
+
 /*
  * Switches to specified vcpu, until a matching vcpu_put(), but assumes
  * vcpu mutex is already taken.
@@ -230,6 +238,25 @@
 		struct descriptor_table dt;
 		unsigned long sysenter_esp;
 
+#ifdef CONFIG_SMP
+		if (vcpu->cpu != -1) {
+			u64 tsc_this, tsc_previous, guest_tsc_offset;
+
+			/* Get TSC value for this and the previous cpu. */
+			rdtscll(tsc_this);
+			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
+						 &tsc_previous, 0, 1);
+
+			/*
+			 * Make sure that the guest sees a monotonically
+			 * increasing TSC.
+			 */
+			guest_tsc_offset = vmcs_read64(TSC_OFFSET);
+			vmcs_write64(TSC_OFFSET, guest_tsc_offset +
+				     		tsc_previous - tsc_this);
+		}
+#endif
+
 		vcpu->cpu = cpu;
 		/*
 		 * Linux uses per-cpu TSS and GDT, so set these when switching

[-- Attachment #3: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 1/3] Sync guest viewable TSC when vcpu migrated

[Anthony Liguori]

Leonard Norrgard wrote:
> When a vcpu is migrated to another cpu, an unexpected difference in
> current and previous cpu TSC values can cause a guest to wait
> indefinitely. A known workaround has been to run kvm with taskset,
> locking it to a single cpu. This patch modifies the guest viewable TSC
> so that the TSC is guaranteed to be monotonically increasing.
>
> Thanks to Avi Kivity for help in pinpointing this issue and advice for
> fixing it.
>
> Signed-off-by: Leonard Norrgård <vinsci@refactor.fi>
>   

Hi Leonard,

Why not factor the common code into kvm_main.c and add a set_tsc_offset 
member to the kvm ops function table?

Regards,

Anthony Liguori

> ------------------------------------------------------------------------
>
> Index: linux-2.6/drivers/kvm/svm.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/svm.c	2007-03-02 17:29:57.000000000 +0200
> +++ linux-2.6/drivers/kvm/svm.c	2007-03-02 17:31:15.000000000 +0200
> @@ -598,9 +598,45 @@
>  	kfree(vcpu->svm);
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
> +/*
> + * Switches to specified vcpu, until a matching vcpu_put(), but assumes
> + * vcpu mutex is already taken.
> + */
>  static struct kvm_vcpu *svm_vcpu_load(struct kvm_vcpu *vcpu)
>  {
> -	get_cpu();
> +	int cpu;
> +
> +	cpu = get_cpu();
> +
> +#ifdef CONFIG_SMP
> +	if (vcpu->cpu != cpu) {
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			vcpu->svm->vmcb->control.tsc_offset +=
> +						tsc_previous - tsc_this;
> +		}
> +	}
> +#endif
> +
> +	vcpu->cpu = cpu;
>  	return vcpu;
>  }
>  
> Index: linux-2.6/drivers/kvm/vmx.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/vmx.c	2007-03-02 17:30:01.000000000 +0200
> +++ linux-2.6/drivers/kvm/vmx.c	2007-03-02 17:30:17.000000000 +0200
> @@ -200,6 +200,14 @@
>  #endif
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
>  /*
>   * Switches to specified vcpu, until a matching vcpu_put(), but assumes
>   * vcpu mutex is already taken.
> @@ -230,6 +238,25 @@
>  		struct descriptor_table dt;
>  		unsigned long sysenter_esp;
>  
> +#ifdef CONFIG_SMP
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous, guest_tsc_offset;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			guest_tsc_offset = vmcs_read64(TSC_OFFSET);
> +			vmcs_write64(TSC_OFFSET, guest_tsc_offset +
> +				     		tsc_previous - tsc_this);
> +		}
> +#endif
> +
>  		vcpu->cpu = cpu;
>  		/*
>  		 * Linux uses per-cpu TSS and GDT, so set these when switching
>   
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys-and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 1/3] Sync guest viewable TSC when vcpu migrated

[Leonard Norrgard]

> Hi Leonard,
> 
> Why not factor the common code into kvm_main.c and add a set_tsc_offset 
> member to the kvm ops function table?

Good idea, I put it on the todo list.

> Regards,
> 
> Anthony Liguori

Thanks,

-- Leonard

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH 1/3] Sync guest viewable TSC when vcpu migrated

[Avi Kivity]

Thanks for picking this up!  Some comments below.

Leonard Norrgard wrote:
> Index: linux-2.6/drivers/kvm/svm.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/svm.c	2007-03-02 17:29:57.000000000 +0200
> +++ linux-2.6/drivers/kvm/svm.c	2007-03-02 17:31:15.000000000 +0200
> @@ -598,9 +598,45 @@
>  	kfree(vcpu->svm);
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
> +/*
> + * Switches to specified vcpu, until a matching vcpu_put(), but assumes
> + * vcpu mutex is already taken.
> + */
>  static struct kvm_vcpu *svm_vcpu_load(struct kvm_vcpu *vcpu)
>  {
> -	get_cpu();
> +	int cpu;
> +
> +	cpu = get_cpu();
> +
> +#ifdef CONFIG_SMP
>   

Please avoid such #ifdefs.  Non-SMP virtualization capable processors 
are rare, and smp_call_function_single() is defined for the !CONFIG_SMP 
case.

> +	if (vcpu->cpu != cpu) {
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			vcpu->svm->vmcb->control.tsc_offset +=
> +						tsc_previous - tsc_this;
>   

>  
> Index: linux-2.6/drivers/kvm/vmx.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/vmx.c	2007-03-02 17:30:01.000000000 +0200
> +++ linux-2.6/drivers/kvm/vmx.c	2007-03-02 17:30:17.000000000 +0200
> @@ -200,6 +200,14 @@
>  #endif
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
>  /*
>   * Switches to specified vcpu, until a matching vcpu_put(), but assumes
>   * vcpu mutex is already taken.
> @@ -230,6 +238,25 @@
>  		struct descriptor_table dt;
>  		unsigned long sysenter_esp;
>  
> +#ifdef CONFIG_SMP
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous, guest_tsc_offset;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			guest_tsc_offset = vmcs_read64(TSC_OFFSET);
> +			vmcs_write64(TSC_OFFSET, guest_tsc_offset +
> +				     		tsc_previous - tsc_this);
> +		}
> +#endif
> +
>   

The vmx code already has an IPI (in vcpu_clear), so the two should be 
merged. IPIs are very expensive.


Perhaps call a vmx_vcpu_migrate IPI, which calls __vcpu_clear() and 
loads the tsc.  You can add a tsc_previous to struct kvm_vcpu so you 
have somewhere to store it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH 2/3] Add a vcpu_migrated statistic

[Leonard Norrgard]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

The previous patch introduces an IPI on every vcpu migration on SMP
machines to find out the TSC value on the previous cpu. We might be
able to avoid this IPI on systems that have a synchronized TSC by
checking for it in svm/vmx_vcpu_load:

+               /* If this isn't the first vcpu_load and if the TSC
+                  doesn't run at a constant rate, we must handle TSC
+                  offsets. */
+               if (vcpu->cpu != -1
+                   && !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {

Wether this is actually reliable and working in the intended way is
not know yet.  In the meantime, introduce a statistic for vcpu
migration, so we get some decision support for the needs for such an
optimization.

Signed-off-by: Leonard Norrgård <vinsci-g2GXA8XeJSExHbG02/KK1g@public.gmane.org>


[-- Attachment #2: 02_add_vcpu_migrated_statistic.patch --]
[-- Type: text/x-diff, Size: 1472 bytes --]

Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
--- linux-2.6.orig/drivers/kvm/kvm_main.c	2007-03-02 17:31:59.000000000 +0200
+++ linux-2.6/drivers/kvm/kvm_main.c	2007-03-02 17:34:13.000000000 +0200
@@ -67,6 +67,7 @@
 	{ "halt_exits", &kvm_stat.halt_exits },
 	{ "request_irq", &kvm_stat.request_irq_exits },
 	{ "irq_exits", &kvm_stat.irq_exits },
+	{ "vcpu_migrated", &kvm_stat.vcpu_migrated },
 	{ NULL, NULL }
 };
 
Index: linux-2.6/drivers/kvm/svm.c
===================================================================
--- linux-2.6.orig/drivers/kvm/svm.c	2007-03-02 17:32:02.000000000 +0200
+++ linux-2.6/drivers/kvm/svm.c	2007-03-02 17:33:13.000000000 +0200
@@ -621,6 +621,8 @@
 		if (vcpu->cpu != -1) {
 			u64 tsc_this, tsc_previous;
 
+			++kvm_stat.vcpu_migrated;
+
 			/* Get TSC value for this and the previous cpu. */
 			rdtscll(tsc_this);
 			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
Index: linux-2.6/drivers/kvm/vmx.c
===================================================================
--- linux-2.6.orig/drivers/kvm/vmx.c	2007-03-02 17:32:06.000000000 +0200
+++ linux-2.6/drivers/kvm/vmx.c	2007-03-02 17:33:34.000000000 +0200
@@ -242,6 +242,8 @@
 		if (vcpu->cpu != -1) {
 			u64 tsc_this, tsc_previous, guest_tsc_offset;
 
+			++kvm_stat.vcpu_migrated;
+
 			/* Get TSC value for this and the previous cpu. */
 			rdtscll(tsc_this);
 			smp_call_function_single(vcpu->cpu, ipi_rdtscll,

[-- Attachment #3: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] Add a vcpu_migrated statistic

[Avi Kivity]

Leonard Norrgard wrote:
> The previous patch introduces an IPI on every vcpu migration on SMP
> machines to find out the TSC value on the previous cpu. We might be
> able to avoid this IPI on systems that have a synchronized TSC by
> checking for it in svm/vmx_vcpu_load:
>
> +               /* If this isn't the first vcpu_load and if the TSC
> +                  doesn't run at a constant rate, we must handle TSC
> +                  offsets. */
> +               if (vcpu->cpu != -1
> +                   && !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
>
> Wether this is actually reliable and working in the intended way is
> not know yet.  

Reading the documentation for X86_FEATURE_CONSTANT_TSC, I'm not sure either.


> In the meantime, introduce a statistic for vcpu
> migration, so we get some decision support for the needs for such an
> optimization.
>
>   

The statistic is great (vcpu migrations are very expensive, we should 
know when they happen), but why two unrelated changes in a patch?  I 
think you can drop the constant tsc optimization for now, given we 
aren't certain it works as we expect.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH 3/3] Widen key column in kvm_stat

[Leonard Norrgard]

[-- Attachment #1: Type: text/plain, Size: 144 bytes --]

Make the key column wider, so the keys are fully shown.

Signed-off-by: Leonard Norrgård <vinsci-g2GXA8XeJSExHbG02/KK1g@public.gmane.org>


[-- Attachment #2: 03_widen_kvm_stat_column.patch --]
[-- Type: text/x-diff, Size: 587 bytes --]

=== kvm_stat
==================================================================
--- kvm_stat	(revision 20893)
+++ kvm_stat	(local)
@@ -39,9 +39,9 @@
         for key in sorted(s.keys()):
             values = s[key]
             screen.addstr(row, 1, key)
-            screen.addstr(row, 12, '%10d' % (values[0],))
+            screen.addstr(row, 14, '%10d' % (values[0],))
             if values[1] is not None:
-                screen.addstr(row, 22, '%8d' % (values[1],))
+                screen.addstr(row, 24, '%8d' % (values[1],))
             row += 1
         screen.refresh()
 

[-- Attachment #3: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel