[PATCH] x86/pvclock-xen: zero last_value on resume

[PATCH] x86/pvclock-xen: zero last_value on resume

[Jeremy Fitzhardinge]

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



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 <jeremy.fitzhardinge@citrix.com>
Cc: Stable Kernel <stable@kernel.org>
Reported-by: Eelco Dolstra <e.dolstra@tudelft.nl>
Reported-by: Olivier Hanesse <olivier.hanesse@gmail.com>
Bisected-by: Cédric Schieli <cschieli@gmail.com>
Tested-by: Cédric Schieli <cschieli@gmail.com>

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index cd02f32..6226870 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -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 */
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 239427c..a4f07c1 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -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;
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index b2bb5aa..5da5e53 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;
 



[-- Attachment #2: x86-pvclock-resume.patch --]
[-- Type: text/plain, Size: 2064 bytes --]

From 29acbb4e1d93e719250648db1ce8c7a24144fd86 Mon Sep 17 00:00:00 2001
From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
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 <jeremy.fitzhardinge@citrix.com>
Cc: Stable Kernel <stable@kernel.org>
Reported-by: Eelco Dolstra <e.dolstra@tudelft.nl>
Reported-by: Olivier Hanesse <olivier.hanesse@gmail.com>
Bisected-by: Cédric Schieli <cschieli@gmail.com>
Tested-by: Cédric Schieli <cschieli@gmail.com>

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index cd02f32..6226870 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -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 */
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 239427c..a4f07c1 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -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;
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index b2bb5aa..5da5e53 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;
 

Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Glauber Costa]

On Tue, 2010-10-26 at 09:59 -0700, Jeremy Fitzhardinge wrote:
> 
> 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.

Re: [Xen-devel] Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Jeremy Fitzhardinge]

 On 10/26/2010 10:48 AM, Glauber Costa wrote:
> On Tue, 2010-10-26 at 09:59 -0700, Jeremy Fitzhardinge wrote:
>> 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.

Thanks.

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

Thanks,
    J

Re: [Xen-devel] Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[H. Peter Anvin]

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

"Jeremy Fitzhardinge" <jeremy@goop.org> wrote:

> On 10/26/2010 10:48 AM, Glauber Costa wrote:
>> On Tue, 2010-10-26 at 09:59 -0700, Jeremy Fitzhardinge wrote:
>>> 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.
>
>Thanks.
>
>HPA, do you want to take this, or shall I send it on?
>
>Thanks,
>    J

-- 
Sent from my mobile phone.  Please pardon any lack of formatting.

Re: Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Ian Campbell]

On Wed, 2010-10-27 at 13:59 -0700, H. Peter Anvin wrote:
> 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.

> 
> "Jeremy Fitzhardinge" <jeremy@goop.org> wrote:
> 
> > On 10/26/2010 10:48 AM, Glauber Costa wrote:
> >> On Tue, 2010-10-26 at 09:59 -0700, Jeremy Fitzhardinge wrote:
> >>> 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.
> >
> >Thanks.
> >
> >HPA, do you want to take this, or shall I send it on?
> >
> >Thanks,
> >    J
> 

-- 
Ian Campbell

BOFH excuse #191:

Just type 'mv * /dev/null'.

Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Jeremy Fitzhardinge]

On 10/26/2010 09:59 AM, Jeremy Fitzhardinge wrote:
>
> 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

> [ I don't know if kvm needs an analogous fix or not. ]
>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Cc: Stable Kernel <stable@kernel.org>
> Reported-by: Eelco Dolstra <e.dolstra@tudelft.nl>
> Reported-by: Olivier Hanesse <olivier.hanesse@gmail.com>
> Bisected-by: Cédric Schieli <cschieli@gmail.com>
> Tested-by: Cédric Schieli <cschieli@gmail.com>
>
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index cd02f32..6226870 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -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 */
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 239427c..a4f07c1 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -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;
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index b2bb5aa..5da5e53 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -426,6 +426,8 @@ void xen_timer_resume(void)
>  {
>  	int cpu;
>  
> +	pvclock_resume();
> +
>  	if (xen_clockevent != &xen_vcpuop_clockevent)
>  		return;
>  
>
>
>
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> On 10/26/2010 09:59 AM, Jeremy Fitzhardinge wrote:
> >
> > 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

Re: [PATCH] x86/pvclock-xen: zero last_value on resume

[Jeremy Fitzhardinge]

On 11/26/2010 02:40 AM, Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>> On 10/26/2010 09:59 AM, Jeremy Fitzhardinge wrote:
>>> 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 <jeremy.fitzhardinge@citrix.com>

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7f7e577..31d84ac 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -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,
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 008b91e..42eb330 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -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;
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index b2bb5aa..5da5e53 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -426,6 +426,8 @@ void xen_timer_resume(void)
 {
 	int cpu;
 
+	pvclock_resume();
+
 	if (xen_clockevent != &xen_vcpuop_clockevent)
 		return;