Skip to contentSkip to conversation

x86/pvclock-xen: zero last_value on resume

PATCH In review Jeremy Fitzhardinge 5 participants 15 messages Oct 26, 2010 – Nov 26, 2010

Subsystems (4)
x86 KVM virtio core Xen hypervisor x86
lore
Share & tools
lore

Jeremy Fitzhardinge

author
#0 lore raw

PATCH 1/1 x86/pvclock-xen: zero last_value on resume

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Stable Kernel <[email protected]>
Reported-by Eelco Dolstra <[email protected]>
Reported-by Olivier Hanesse <[email protected]>
Bisected-by Cédric Schieli <[email protected]>
Tested-by Cédric Schieli <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index cd02f32..6226870 100644
@@ -11,5 +11,6 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 #endif /* _ASM_X86_PVCLOCK_H */
arch/x86/kernel/pvclock.c +5 -0
index 239427c..a4f07c1 100644
@@ -120,6 +120,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;

From 29acbb4e1d93e719250648db1ce8c7a24144fd86 Mon Sep 17 00:00:00 2001
From: Jeremy Fitzhardinge <[email protected]>
Date: Mon, 25 Oct 2010 16:53:46 -0700
Subject: [PATCH] x86/pvclock: zero last_value on resume

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Stable Kernel <[email protected]>
Reported-by Eelco Dolstra <[email protected]>
Reported-by Olivier Hanesse <[email protected]>
Bisected-by Cédric Schieli <[email protected]>
Tested-by Cédric Schieli <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index cd02f32..6226870 100644
@@ -11,5 +11,6 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 #endif /* _ASM_X86_PVCLOCK_H */
arch/x86/kernel/pvclock.c +5 -0
index 239427c..a4f07c1 100644
@@ -120,6 +120,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;
#1 lore raw
Quoted 8 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees monotonicity.

I am not opposed to this patch in any way, however.

#8 lore raw
In reply to Jeremy Fitzhardinge
Quoted 8 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees monotonicity.

I am not opposed to this patch in any way, however.

author
#2 lore raw
In reply to Glauber Costa
Quoted 7 lines · Glauber Costa
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Quoted 5 lines
Quoted 1 line

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees monotonicity.

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

author
#7 lore raw
In reply to Glauber Costa
Quoted 7 lines · Glauber Costa
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Quoted 5 lines
Quoted 1 line

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees monotonicity.

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

reviewer
#3 lore raw
In reply to Jeremy Fitzhardinge

I'll check it this evening when I'm at a working network again :(

Quoted 21 lines · Jeremy Fitzhardinge
Quoted 10 lines · Glauber Costa
Quoted 7 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees

monotonicity.

Quoted 2 lines

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

Signature · 1 line
Sent from my mobile phone.  Please pardon any lack of formatting.
reviewer
#6 lore raw
In reply to Jeremy Fitzhardinge

I'll check it this evening when I'm at a working network again :(

Quoted 21 lines · Jeremy Fitzhardinge
Quoted 10 lines · Glauber Costa
Quoted 7 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees

monotonicity.

Quoted 2 lines

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

Signature · 1 line
Sent from my mobile phone.  Please pardon any lack of formatting.
#4 lore raw
In reply to H. Peter Anvin
Quoted 1 line · H. Peter Anvin

I'll check it this evening when I'm at a working network again :(

Did this get applied? It seems to affect 2.6.32.x too
(http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=602273) so can we tag it for stable as well?

Thanks,
Ian.

Quoted 25 lines
Quoted 21 lines · Jeremy Fitzhardinge
Quoted 10 lines · Glauber Costa
Quoted 7 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees

monotonicity.

Quoted 2 lines

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

Signature · 5 lines
Ian Campbell

BOFH excuse #191:

Just type 'mv * /dev/null'.
#5 lore raw
In reply to H. Peter Anvin
Quoted 1 line · H. Peter Anvin

I'll check it this evening when I'm at a working network again :(

Did this get applied? It seems to affect 2.6.32.x too
(http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=602273) so can we tag it for stable as well?

Thanks,
Ian.

Quoted 25 lines
Quoted 21 lines · Jeremy Fitzhardinge
Quoted 10 lines · Glauber Costa
Quoted 7 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

[ I don't know if kvm needs an analogous fix or not. ]

After migration, save/restore, etc, we issue an ioctl where we tell the host the last clock value. That (in theory) guarantees

monotonicity.

Quoted 2 lines

I am not opposed to this patch in any way, however.

Thanks.

HPA, do you want to take this, or shall I send it on?

Thanks,
J

Signature · 5 lines
Ian Campbell

BOFH excuse #191:

Just type 'mv * /dev/null'.
author
#9 lore raw
In reply to Jeremy Fitzhardinge
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

J

Quoted 57 lines

[ I don't know if kvm needs an analogous fix or not. ]

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Stable Kernel <[email protected]>
Reported-by Eelco Dolstra <[email protected]>
Reported-by Olivier Hanesse <[email protected]>
Bisected-by Cédric Schieli <[email protected]>
Tested-by Cédric Schieli <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index cd02f32..6226870 100644
@@ -11,5 +11,6 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 #endif /* _ASM_X86_PVCLOCK_H */
arch/x86/kernel/pvclock.c +5 -0
index 239427c..a4f07c1 100644
@@ -120,6 +120,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

author
#14 lore raw
In reply to Jeremy Fitzhardinge
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

J

Quoted 53 lines

[ I don't know if kvm needs an analogous fix or not. ]

Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Stable Kernel <[email protected]>
Reported-by Eelco Dolstra <[email protected]>
Reported-by Olivier Hanesse <[email protected]>
Bisected-by Cédric Schieli <[email protected]>
Tested-by Cédric Schieli <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index cd02f32..6226870 100644
@@ -11,5 +11,6 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 #endif /* _ASM_X86_PVCLOCK_H */
arch/x86/kernel/pvclock.c +5 -0
index 239427c..a4f07c1 100644
@@ -120,6 +120,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;
Quoted 4 lines

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

#10 lore raw
In reply to Jeremy Fitzhardinge
Quoted 9 lines · Jeremy Fitzhardinge
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

Does not apply cleanly here - mind resending the latest version?

Thanks,

Ingo

#13 lore raw
In reply to Jeremy Fitzhardinge
Quoted 9 lines · Jeremy Fitzhardinge
Quoted 6 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

Does not apply cleanly here - mind resending the latest version?

Thanks,

Ingo

author
#11 lore raw
In reply to Ingo Molnar
Quoted 10 lines · Ingo Molnar
Quoted 7 lines · Jeremy Fitzhardinge
Quoted 5 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

Does not apply cleanly here - mind resending the latest version?

It rebased cleanly to 2.6.37-rc3. You can pull it from:

git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git pvclock-resume

Jeremy Fitzhardinge (1):
x86/pvclock: zero last_value on resume

 arch/x86/include/asm/pvclock.h |    1 +
 arch/x86/kernel/pvclock.c      |    5 +++++
 arch/x86/xen/time.c            |    2 ++
 3 files changed, 8 insertions(+), 0 deletions(-)

Thanks,
J

Subject: [PATCH] x86/pvclock: zero last_value on resume

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index 7f7e577..31d84ac 100644
@@ -11,6 +11,7 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 /*
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
arch/x86/kernel/pvclock.c +5 -0
index 008b91e..42eb330 100644
@@ -83,6 +83,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;
author
#12 lore raw
In reply to Ingo Molnar
Quoted 10 lines · Ingo Molnar
Quoted 7 lines · Jeremy Fitzhardinge
Quoted 5 lines · Jeremy Fitzhardinge

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Ping? Looks like this fell through the gaps.

Does not apply cleanly here - mind resending the latest version?

It rebased cleanly to 2.6.37-rc3. You can pull it from:

git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git pvclock-resume

Jeremy Fitzhardinge (1):
x86/pvclock: zero last_value on resume

 arch/x86/include/asm/pvclock.h |    1 +
 arch/x86/kernel/pvclock.c      |    5 +++++
 arch/x86/xen/time.c            |    2 ++
 3 files changed, 8 insertions(+), 0 deletions(-)

Thanks,
J

Subject: [PATCH] x86/pvclock: zero last_value on resume

If the guest domain has been suspend/resumed or migrated, then the system clock backing the pvclock clocksource may revert to a smaller value (ie, can be non-monotonic across the migration/save-restore). Make sure we zero last_value in that case so that the domain continues to see clock updates.

Signed-off-by Jeremy Fitzhardinge <[email protected]>
arch/x86/include/asm/pvclock.h +1 -0
index 7f7e577..31d84ac 100644
@@ -11,6 +11,7 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
 			    struct pvclock_vcpu_time_info *vcpu,
 			    struct timespec *ts);
+void pvclock_resume(void);
 
 /*
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
arch/x86/kernel/pvclock.c +5 -0
index 008b91e..42eb330 100644
@@ -83,6 +83,11 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src)
 
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
+void pvclock_resume(void)
+{
+	atomic64_set(&last_value, 0);
+}
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
arch/x86/xen/time.c +2 -0
index b2bb5aa..5da5e53 100644
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;