[PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

The kernel has limited support for updating the persistent clock or
RTC when NTP is synced.  This has the following limitations:

* The persistent clock is not updated on step changes.  This leaves a
  window where it will be incorrect (while NTP resyncs).

* Xen guests use the Xen wallclock as their persistent clock.  dom0
  maintains this clock so it is persistent for domUs and not dom0
  itself.

These series fixes the above limitations and depends on "x86: increase
precision of x86_platform.get/set_wallclock()" which was previously
posted.

[ On a related note, with CONFIG_HZ=1000 sync_cmos_clock() is always
  scheduled ~3ms too late which causes it to repeatedly try to
  reschedule in ~997 ms and ends up never calling
  updated_persistent_clock().  With HZ=250, the error is ~1ms too late
  which is close enough.

  It is not clear where this systematic error comes from or whether
  this is only a Xen specific bug.  I don't have time to investigate
  right now. ]

Changes since v2:

Don't peek at the timekeeper internals (use __current_kernel_time()
instead).  Use the native set_wallclock hook in dom0.

Changes since v1:

Reworked to use the pvclock_gtod notifier to sync the wallclock (this
looked similar to what a KVM host does).  update_persistent_clock()
will now only update the CMOS RTC.

David

[1] http://lists.xen.org/archives/html/xen-devel/2013-05/msg01402.html

[PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Currently the Xen wallclock is only updated every 11 minutes if NTP is
synchronized to its clock source.  If a guest is started before NTP is
synchronized it may see an incorrect wallclock time.

Use the pvclock_gtod notifier chain to receive a notification when the
system time has changed and update the wallclock to match.

This chain is called on every timer tick and we want to avoid an extra
(expensive) hypercall on every tick.  Because dom0 has historically
never provided a very accurate wallclock and guests do not expect one,
we can do this simply.  The wallclock is only updated if the
difference between now and the last update is more than 0.5 s.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..60b7d1f 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/pvclock_gtod.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct timespec *now)
 	return HYPERVISOR_dom0_op(&op);
 }
 
+static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
+				   void *priv)
+{
+	static struct timespec last, next;
+	struct timespec now;
+	struct xen_platform_op op;
+	int ret;
+
+	/*
+	 * Set the Xen wallclock from Linux system time.
+	 *
+	 * dom0 hasn't historically maintained a very accurate
+	 * wallclock so guests don't expect it. We can therefore
+	 * reduce the number of expensive hypercalls by only updating
+	 * the wallclock every 0.5 s.
+	 */
+
+	now = __current_kernel_time();
+
+	if (timespec_compare(&now, &last) > 0
+	    && timespec_compare(&now, &next) < 0)
+		return 0;
+
+	op.cmd = XENPF_settime;
+	op.u.settime.secs = now.tv_sec;
+	op.u.settime.nsecs = now.tv_nsec;
+	op.u.settime.system_time = xen_clocksource_read();
+
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return 0;
+
+	last = now;
+	next = timespec_add(now, ns_to_timespec(NSEC_PER_SEC / 2));
+
+	return 0;
+}
+
+static struct notifier_block xen_pvclock_gtod_notifier = {
+	.notifier_call = xen_pvclock_gtod_notify,
+};
+
 static struct clocksource xen_clocksource __read_mostly = {
 	.name = "xen",
 	.rating = 400,
@@ -473,6 +516,9 @@ static void __init xen_time_init(void)
 	xen_setup_runstate_info(cpu);
 	xen_setup_timer(cpu);
 	xen_setup_cpu_clockevents();
+
+	if (xen_initial_domain())
+		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
 }
 
 void __init xen_init_time_ops(void)
-- 
1.7.2.5

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[John Stultz]

On 05/30/2013 07:25 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Currently the Xen wallclock is only updated every 11 minutes if NTP is
> synchronized to its clock source.  If a guest is started before NTP is
> synchronized it may see an incorrect wallclock time.

Ok.. So this is maybe starting to make a little more sense.

I was under the mistaken impression domN guests referenced dom0's system 
time when they called xen_get_wallclock(), but looking at this a bit 
closer, its starting to make a bit more sense that xen_get_wallclock() 
is just shared hypervisor data that is updated by dom0, and guests can 
access this data without interacting with dom0.

Thus I can finally see the 11 minute update interval for dom0 to update 
the hypervisor wallclock data causes guests to get invalid time values 
when they initialize, reading the unset by dom0 hypervisor wallclock 
data. And thus I finally see the need for dom0 to more frequently update 
the hypervisor wallclock data.

So first, sorry for being so dense through all of this. But this really 
could use a clearer explanation as to the nature of the issue.

Few minor issues below.

> Use the pvclock_gtod notifier chain to receive a notification when the
> system time has changed and update the wallclock to match.
>
> This chain is called on every timer tick and we want to avoid an extra
> (expensive) hypercall on every tick.  Because dom0 has historically
> never provided a very accurate wallclock and guests do not expect one,
> we can do this simply.  The wallclock is only updated if the
> difference between now and the last update is more than 0.5 s.


So given (at least I think ) I get why this is needed, is there a reason 
you're using the notifier chain instead of a regular timer in dom0 to 
update the hypervisor's wallclock data?


>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..60b7d1f 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/pvclock_gtod.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct timespec *now)
>   	return HYPERVISOR_dom0_op(&op);
>   }
>   
> +static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> +				   void *priv)
> +{
> +	static struct timespec last, next;
> +	struct timespec now;
> +	struct xen_platform_op op;
> +	int ret;
> +
> +	/*
> +	 * Set the Xen wallclock from Linux system time.
> +	 *
> +	 * dom0 hasn't historically maintained a very accurate
> +	 * wallclock so guests don't expect it. We can therefore
> +	 * reduce the number of expensive hypercalls by only updating
> +	 * the wallclock every 0.5 s.

This comment needs some improvement. It doesn't explain why Xen needs to 
set the virtual RTC so frequently, but then goes on to say it can be 
done every half second because guests don't really expect it anyway.

> +	 */
> +
> +	now = __current_kernel_time();

You don't seem to be holding the timekeeping lock here, so why are you 
calling the internal __ prefixed current_kernel_time() accessor?


> +
> +	if (timespec_compare(&now, &last) > 0

Not sure I understand why you're bothering with the last value? Aren't 
you just wanting to trigger when now is greater then next?


So again, apologies for some of the runaround in the discussion! Lets 
sort out the above minor issues and I'll be fine to queue this (given 
Xen maintainer acks) without grumbling.

thanks
-john

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[John Stultz]

On 05/30/2013 07:25 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Currently the Xen wallclock is only updated every 11 minutes if NTP is
> synchronized to its clock source.  If a guest is started before NTP is
> synchronized it may see an incorrect wallclock time.

Ok.. So this is maybe starting to make a little more sense.

I was under the mistaken impression domN guests referenced dom0's system 
time when they called xen_get_wallclock(), but looking at this a bit 
closer, its starting to make a bit more sense that xen_get_wallclock() 
is just shared hypervisor data that is updated by dom0, and guests can 
access this data without interacting with dom0.

Thus I can finally see the 11 minute update interval for dom0 to update 
the hypervisor wallclock data causes guests to get invalid time values 
when they initialize, reading the unset by dom0 hypervisor wallclock 
data. And thus I finally see the need for dom0 to more frequently update 
the hypervisor wallclock data.

So first, sorry for being so dense through all of this. But this really 
could use a clearer explanation as to the nature of the issue.

Few minor issues below.

> Use the pvclock_gtod notifier chain to receive a notification when the
> system time has changed and update the wallclock to match.
>
> This chain is called on every timer tick and we want to avoid an extra
> (expensive) hypercall on every tick.  Because dom0 has historically
> never provided a very accurate wallclock and guests do not expect one,
> we can do this simply.  The wallclock is only updated if the
> difference between now and the last update is more than 0.5 s.


So given (at least I think ) I get why this is needed, is there a reason 
you're using the notifier chain instead of a regular timer in dom0 to 
update the hypervisor's wallclock data?


>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..60b7d1f 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/pvclock_gtod.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct timespec *now)
>   	return HYPERVISOR_dom0_op(&op);
>   }
>   
> +static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> +				   void *priv)
> +{
> +	static struct timespec last, next;
> +	struct timespec now;
> +	struct xen_platform_op op;
> +	int ret;
> +
> +	/*
> +	 * Set the Xen wallclock from Linux system time.
> +	 *
> +	 * dom0 hasn't historically maintained a very accurate
> +	 * wallclock so guests don't expect it. We can therefore
> +	 * reduce the number of expensive hypercalls by only updating
> +	 * the wallclock every 0.5 s.

This comment needs some improvement. It doesn't explain why Xen needs to 
set the virtual RTC so frequently, but then goes on to say it can be 
done every half second because guests don't really expect it anyway.

> +	 */
> +
> +	now = __current_kernel_time();

You don't seem to be holding the timekeeping lock here, so why are you 
calling the internal __ prefixed current_kernel_time() accessor?


> +
> +	if (timespec_compare(&now, &last) > 0

Not sure I understand why you're bothering with the last value? Aren't 
you just wanting to trigger when now is greater then next?


So again, apologies for some of the runaround in the discussion! Lets 
sort out the above minor issues and I'll be fine to queue this (given 
Xen maintainer acks) without grumbling.

thanks
-john

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[David Vrabel]

On 31/05/13 01:30, John Stultz wrote:
> On 05/30/2013 07:25 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Currently the Xen wallclock is only updated every 11 minutes if NTP is
>> synchronized to its clock source.  If a guest is started before NTP is
>> synchronized it may see an incorrect wallclock time.
> 
> Ok.. So this is maybe starting to make a little more sense.
> 
> I was under the mistaken impression domN guests referenced dom0's system
> time when they called xen_get_wallclock(), but looking at this a bit
> closer, its starting to make a bit more sense that xen_get_wallclock()
> is just shared hypervisor data that is updated by dom0, and guests can
> access this data without interacting with dom0.
> 
> Thus I can finally see the 11 minute update interval for dom0 to update
> the hypervisor wallclock data causes guests to get invalid time values
> when they initialize, reading the unset by dom0 hypervisor wallclock
> data. And thus I finally see the need for dom0 to more frequently update
> the hypervisor wallclock data.

This is correct.  I'll add an explanatory paragraph about the Xen
wallclock to the changelog.

>> Use the pvclock_gtod notifier chain to receive a notification when the
>> system time has changed and update the wallclock to match.
>>
>> This chain is called on every timer tick and we want to avoid an extra
>> (expensive) hypercall on every tick.  Because dom0 has historically
>> never provided a very accurate wallclock and guests do not expect one,
>> we can do this simply.  The wallclock is only updated if the
>> difference between now and the last update is more than 0.5 s.
> 
> 
> So given (at least I think ) I get why this is needed, is there a reason
> you're using the notifier chain instead of a regular timer in dom0 to
> update the hypervisor's wallclock data?

Using the notifier allows step changes to be noticed immediately, using
just a timer would leave a window after any step change where the Xen
wallclock is wrong.

Ideally, I would like a notification of step changes and a long period
timer (to correct for drift).  Can you think of a good way to do this?

>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct
>> timespec *now)
>>       return HYPERVISOR_dom0_op(&op);
>>   }
>>   +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
>> unsigned long unused,
>> +                   void *priv)
>> +{
>> +    static struct timespec last, next;
>> +    struct timespec now;
>> +    struct xen_platform_op op;
>> +    int ret;
>> +
>> +    /*
>> +     * Set the Xen wallclock from Linux system time.
>> +     *
>> +     * dom0 hasn't historically maintained a very accurate
>> +     * wallclock so guests don't expect it. We can therefore
>> +     * reduce the number of expensive hypercalls by only updating
>> +     * the wallclock every 0.5 s.
> 
> This comment needs some improvement. It doesn't explain why Xen needs to
> set the virtual RTC so frequently, but then goes on to say it can be
> done every half second because guests don't really expect it anyway.

This would probably be better done as:

if abs(current_wallclock - current_kernel_time) > threshold)
    update_wallclock();

i.e., we're correcting the wallclock if it is wrong.

>> +     */
>> +
>> +    now = __current_kernel_time();
> 
> You don't seem to be holding the timekeeping lock here, so why are you
> calling the internal __ prefixed current_kernel_time() accessor?

The notifier chain is called from timekeeping_update() which is called
in a write_seqcount_begin/end(&timekeeper_seq) block.

>> +
>> +    if (timespec_compare(&now, &last) > 0
> 
> Not sure I understand why you're bothering with the last value? Aren't
> you just wanting to trigger when now is greater then next?

This is to handle step changes that go backwards.

> So again, apologies for some of the runaround in the discussion! Lets
> sort out the above minor issues and I'll be fine to queue this (given
> Xen maintainer acks) without grumbling.

No problem.

David

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[David Vrabel]

On 31/05/13 01:30, John Stultz wrote:
> On 05/30/2013 07:25 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Currently the Xen wallclock is only updated every 11 minutes if NTP is
>> synchronized to its clock source.  If a guest is started before NTP is
>> synchronized it may see an incorrect wallclock time.
> 
> Ok.. So this is maybe starting to make a little more sense.
> 
> I was under the mistaken impression domN guests referenced dom0's system
> time when they called xen_get_wallclock(), but looking at this a bit
> closer, its starting to make a bit more sense that xen_get_wallclock()
> is just shared hypervisor data that is updated by dom0, and guests can
> access this data without interacting with dom0.
> 
> Thus I can finally see the 11 minute update interval for dom0 to update
> the hypervisor wallclock data causes guests to get invalid time values
> when they initialize, reading the unset by dom0 hypervisor wallclock
> data. And thus I finally see the need for dom0 to more frequently update
> the hypervisor wallclock data.

This is correct.  I'll add an explanatory paragraph about the Xen
wallclock to the changelog.

>> Use the pvclock_gtod notifier chain to receive a notification when the
>> system time has changed and update the wallclock to match.
>>
>> This chain is called on every timer tick and we want to avoid an extra
>> (expensive) hypercall on every tick.  Because dom0 has historically
>> never provided a very accurate wallclock and guests do not expect one,
>> we can do this simply.  The wallclock is only updated if the
>> difference between now and the last update is more than 0.5 s.
> 
> 
> So given (at least I think ) I get why this is needed, is there a reason
> you're using the notifier chain instead of a regular timer in dom0 to
> update the hypervisor's wallclock data?

Using the notifier allows step changes to be noticed immediately, using
just a timer would leave a window after any step change where the Xen
wallclock is wrong.

Ideally, I would like a notification of step changes and a long period
timer (to correct for drift).  Can you think of a good way to do this?

>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct
>> timespec *now)
>>       return HYPERVISOR_dom0_op(&op);
>>   }
>>   +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
>> unsigned long unused,
>> +                   void *priv)
>> +{
>> +    static struct timespec last, next;
>> +    struct timespec now;
>> +    struct xen_platform_op op;
>> +    int ret;
>> +
>> +    /*
>> +     * Set the Xen wallclock from Linux system time.
>> +     *
>> +     * dom0 hasn't historically maintained a very accurate
>> +     * wallclock so guests don't expect it. We can therefore
>> +     * reduce the number of expensive hypercalls by only updating
>> +     * the wallclock every 0.5 s.
> 
> This comment needs some improvement. It doesn't explain why Xen needs to
> set the virtual RTC so frequently, but then goes on to say it can be
> done every half second because guests don't really expect it anyway.

This would probably be better done as:

if abs(current_wallclock - current_kernel_time) > threshold)
    update_wallclock();

i.e., we're correcting the wallclock if it is wrong.

>> +     */
>> +
>> +    now = __current_kernel_time();
> 
> You don't seem to be holding the timekeeping lock here, so why are you
> calling the internal __ prefixed current_kernel_time() accessor?

The notifier chain is called from timekeeping_update() which is called
in a write_seqcount_begin/end(&timekeeper_seq) block.

>> +
>> +    if (timespec_compare(&now, &last) > 0
> 
> Not sure I understand why you're bothering with the last value? Aren't
> you just wanting to trigger when now is greater then next?

This is to handle step changes that go backwards.

> So again, apologies for some of the runaround in the discussion! Lets
> sort out the above minor issues and I'll be fine to queue this (given
> Xen maintainer acks) without grumbling.

No problem.

David

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[John Stultz]

On 05/31/2013 02:49 AM, David Vrabel wrote:
> On 31/05/13 01:30, John Stultz wrote:
>> On 05/30/2013 07:25 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> Currently the Xen wallclock is only updated every 11 minutes if NTP is
>>> synchronized to its clock source.  If a guest is started before NTP is
>>> synchronized it may see an incorrect wallclock time.
>> Ok.. So this is maybe starting to make a little more sense.
>>
>> I was under the mistaken impression domN guests referenced dom0's system
>> time when they called xen_get_wallclock(), but looking at this a bit
>> closer, its starting to make a bit more sense that xen_get_wallclock()
>> is just shared hypervisor data that is updated by dom0, and guests can
>> access this data without interacting with dom0.
>>
>> Thus I can finally see the 11 minute update interval for dom0 to update
>> the hypervisor wallclock data causes guests to get invalid time values
>> when they initialize, reading the unset by dom0 hypervisor wallclock
>> data. And thus I finally see the need for dom0 to more frequently update
>> the hypervisor wallclock data.
> This is correct.  I'll add an explanatory paragraph about the Xen
> wallclock to the changelog.

Thanks! I appreciate it!


>
>>> Use the pvclock_gtod notifier chain to receive a notification when the
>>> system time has changed and update the wallclock to match.
>>>
>>> This chain is called on every timer tick and we want to avoid an extra
>>> (expensive) hypercall on every tick.  Because dom0 has historically
>>> never provided a very accurate wallclock and guests do not expect one,
>>> we can do this simply.  The wallclock is only updated if the
>>> difference between now and the last update is more than 0.5 s.
>>
>> So given (at least I think ) I get why this is needed, is there a reason
>> you're using the notifier chain instead of a regular timer in dom0 to
>> update the hypervisor's wallclock data?
> Using the notifier allows step changes to be noticed immediately, using
> just a timer would leave a window after any step change where the Xen
> wallclock is wrong.
>
> Ideally, I would like a notification of step changes and a long period
> timer (to correct for drift).  Can you think of a good way to do this?

So we have the clock_was_set() hook that we use to notify the hrtimer 
code and we use that for the timerfd notification as well (which allows 
userland to detect changes to CLOCK_REALTIME).

Maybe that hook should get extended for this use?

>
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct
>>> timespec *now)
>>>        return HYPERVISOR_dom0_op(&op);
>>>    }
>>>    +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
>>> unsigned long unused,
>>> +                   void *priv)
>>> +{
>>> +    static struct timespec last, next;
>>> +    struct timespec now;
>>> +    struct xen_platform_op op;
>>> +    int ret;
>>> +
>>> +    /*
>>> +     * Set the Xen wallclock from Linux system time.
>>> +     *
>>> +     * dom0 hasn't historically maintained a very accurate
>>> +     * wallclock so guests don't expect it. We can therefore
>>> +     * reduce the number of expensive hypercalls by only updating
>>> +     * the wallclock every 0.5 s.
>> This comment needs some improvement. It doesn't explain why Xen needs to
>> set the virtual RTC so frequently, but then goes on to say it can be
>> done every half second because guests don't really expect it anyway.
> This would probably be better done as:
>
> if abs(current_wallclock - current_kernel_time) > threshold)
>      update_wallclock();
>
> i.e., we're correcting the wallclock if it is wrong.

Yea, this makes more sense (though reading the current_wallclock may be 
too expensive each time?).


>>> +     */
>>> +
>>> +    now = __current_kernel_time();
>> You don't seem to be holding the timekeeping lock here, so why are you
>> calling the internal __ prefixed current_kernel_time() accessor?
> The notifier chain is called from timekeeping_update() which is called
> in a write_seqcount_begin/end(&timekeeper_seq) block.

Ok. Please add a comment just to be clear.

While I was ok with it when it was merged, calling the pvclock notifier 
chain while holding the timekeeping locks is striking me as not the 
smartest approach. So this may need to change in the future.

>>> +
>>> +    if (timespec_compare(&now, &last) > 0
>> Not sure I understand why you're bothering with the last value? Aren't
>> you just wanting to trigger when now is greater then next?
> This is to handle step changes that go backwards.

Ok, thanks for the clarification.

Send me the next revision and we can get it queued up unless you want to 
look at doing something with clock_was_set instead.

thanks
-john

Re: [PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[John Stultz]

On 05/31/2013 02:49 AM, David Vrabel wrote:
> On 31/05/13 01:30, John Stultz wrote:
>> On 05/30/2013 07:25 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> Currently the Xen wallclock is only updated every 11 minutes if NTP is
>>> synchronized to its clock source.  If a guest is started before NTP is
>>> synchronized it may see an incorrect wallclock time.
>> Ok.. So this is maybe starting to make a little more sense.
>>
>> I was under the mistaken impression domN guests referenced dom0's system
>> time when they called xen_get_wallclock(), but looking at this a bit
>> closer, its starting to make a bit more sense that xen_get_wallclock()
>> is just shared hypervisor data that is updated by dom0, and guests can
>> access this data without interacting with dom0.
>>
>> Thus I can finally see the 11 minute update interval for dom0 to update
>> the hypervisor wallclock data causes guests to get invalid time values
>> when they initialize, reading the unset by dom0 hypervisor wallclock
>> data. And thus I finally see the need for dom0 to more frequently update
>> the hypervisor wallclock data.
> This is correct.  I'll add an explanatory paragraph about the Xen
> wallclock to the changelog.

Thanks! I appreciate it!


>
>>> Use the pvclock_gtod notifier chain to receive a notification when the
>>> system time has changed and update the wallclock to match.
>>>
>>> This chain is called on every timer tick and we want to avoid an extra
>>> (expensive) hypercall on every tick.  Because dom0 has historically
>>> never provided a very accurate wallclock and guests do not expect one,
>>> we can do this simply.  The wallclock is only updated if the
>>> difference between now and the last update is more than 0.5 s.
>>
>> So given (at least I think ) I get why this is needed, is there a reason
>> you're using the notifier chain instead of a regular timer in dom0 to
>> update the hypervisor's wallclock data?
> Using the notifier allows step changes to be noticed immediately, using
> just a timer would leave a window after any step change where the Xen
> wallclock is wrong.
>
> Ideally, I would like a notification of step changes and a long period
> timer (to correct for drift).  Can you think of a good way to do this?

So we have the clock_was_set() hook that we use to notify the hrtimer 
code and we use that for the timerfd notification as well (which allows 
userland to detect changes to CLOCK_REALTIME).

Maybe that hook should get extended for this use?

>
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct
>>> timespec *now)
>>>        return HYPERVISOR_dom0_op(&op);
>>>    }
>>>    +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
>>> unsigned long unused,
>>> +                   void *priv)
>>> +{
>>> +    static struct timespec last, next;
>>> +    struct timespec now;
>>> +    struct xen_platform_op op;
>>> +    int ret;
>>> +
>>> +    /*
>>> +     * Set the Xen wallclock from Linux system time.
>>> +     *
>>> +     * dom0 hasn't historically maintained a very accurate
>>> +     * wallclock so guests don't expect it. We can therefore
>>> +     * reduce the number of expensive hypercalls by only updating
>>> +     * the wallclock every 0.5 s.
>> This comment needs some improvement. It doesn't explain why Xen needs to
>> set the virtual RTC so frequently, but then goes on to say it can be
>> done every half second because guests don't really expect it anyway.
> This would probably be better done as:
>
> if abs(current_wallclock - current_kernel_time) > threshold)
>      update_wallclock();
>
> i.e., we're correcting the wallclock if it is wrong.

Yea, this makes more sense (though reading the current_wallclock may be 
too expensive each time?).


>>> +     */
>>> +
>>> +    now = __current_kernel_time();
>> You don't seem to be holding the timekeeping lock here, so why are you
>> calling the internal __ prefixed current_kernel_time() accessor?
> The notifier chain is called from timekeeping_update() which is called
> in a write_seqcount_begin/end(&timekeeper_seq) block.

Ok. Please add a comment just to be clear.

While I was ok with it when it was merged, calling the pvclock notifier 
chain while holding the timekeeping locks is striking me as not the 
smartest approach. So this may need to change in the future.

>>> +
>>> +    if (timespec_compare(&now, &last) > 0
>> Not sure I understand why you're bothering with the last value? Aren't
>> you just wanting to trigger when now is greater then next?
> This is to handle step changes that go backwards.

Ok, thanks for the clarification.

Send me the next revision and we can get it queued up unless you want to 
look at doing something with clock_was_set instead.

thanks
-john

[PATCH 1/2] x86/xen: sync the wallclock when the system time changes

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Currently the Xen wallclock is only updated every 11 minutes if NTP is
synchronized to its clock source.  If a guest is started before NTP is
synchronized it may see an incorrect wallclock time.

Use the pvclock_gtod notifier chain to receive a notification when the
system time has changed and update the wallclock to match.

This chain is called on every timer tick and we want to avoid an extra
(expensive) hypercall on every tick.  Because dom0 has historically
never provided a very accurate wallclock and guests do not expect one,
we can do this simply.  The wallclock is only updated if the
difference between now and the last update is more than 0.5 s.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..60b7d1f 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/pvclock_gtod.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -212,6 +213,48 @@ static int xen_set_wallclock(const struct timespec *now)
 	return HYPERVISOR_dom0_op(&op);
 }
 
+static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
+				   void *priv)
+{
+	static struct timespec last, next;
+	struct timespec now;
+	struct xen_platform_op op;
+	int ret;
+
+	/*
+	 * Set the Xen wallclock from Linux system time.
+	 *
+	 * dom0 hasn't historically maintained a very accurate
+	 * wallclock so guests don't expect it. We can therefore
+	 * reduce the number of expensive hypercalls by only updating
+	 * the wallclock every 0.5 s.
+	 */
+
+	now = __current_kernel_time();
+
+	if (timespec_compare(&now, &last) > 0
+	    && timespec_compare(&now, &next) < 0)
+		return 0;
+
+	op.cmd = XENPF_settime;
+	op.u.settime.secs = now.tv_sec;
+	op.u.settime.nsecs = now.tv_nsec;
+	op.u.settime.system_time = xen_clocksource_read();
+
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return 0;
+
+	last = now;
+	next = timespec_add(now, ns_to_timespec(NSEC_PER_SEC / 2));
+
+	return 0;
+}
+
+static struct notifier_block xen_pvclock_gtod_notifier = {
+	.notifier_call = xen_pvclock_gtod_notify,
+};
+
 static struct clocksource xen_clocksource __read_mostly = {
 	.name = "xen",
 	.rating = 400,
@@ -473,6 +516,9 @@ static void __init xen_time_init(void)
 	xen_setup_runstate_info(cpu);
 	xen_setup_timer(cpu);
 	xen_setup_cpu_clockevents();
+
+	if (xen_initial_domain())
+		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
 }
 
 void __init xen_init_time_ops(void)
-- 
1.7.2.5

[PATCH 2/2] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Adjustments to Xen's persistent_clock via update_persistent_clock()
don't actually persist, as the Xen wallclock is just used by domU
guests, and modifications to it do not modify the underlying CMOS RTC.

The x86_platform.set_wallclock hook can be used to keep the hardware
RTC synchronized (as on bare metal).  Because the Xen wallclock is now
kept synchronized by xen_pvclock_gtod_notify() on any system time
change, xen_set_wallclock() need not do this and dom0 can simply use
the native implementation.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 60b7d1f..0e69994 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -199,18 +199,7 @@ static void xen_get_wallclock(struct timespec *now)
 
 static int xen_set_wallclock(const struct timespec *now)
 {
-	struct xen_platform_op op;
-
-	/* do nothing for domU */
-	if (!xen_initial_domain())
-		return -1;
-
-	op.cmd = XENPF_settime;
-	op.u.settime.secs = now->tv_sec;
-	op.u.settime.nsecs = now->tv_nsec;
-	op.u.settime.system_time = xen_clocksource_read();
-
-	return HYPERVISOR_dom0_op(&op);
+	return -1;
 }
 
 static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
@@ -531,7 +520,9 @@ void __init xen_init_time_ops(void)
 
 	x86_platform.calibrate_tsc = xen_tsc_khz;
 	x86_platform.get_wallclock = xen_get_wallclock;
-	x86_platform.set_wallclock = xen_set_wallclock;
+	/* Dom0 uses the native method to set the hardware RTC. */
+	if (!xen_initial_domain())
+		x86_platform.set_wallclock = xen_set_wallclock;
 }
 
 #ifdef CONFIG_XEN_PVHVM
-- 
1.7.2.5

[PATCH 2/2] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Adjustments to Xen's persistent_clock via update_persistent_clock()
don't actually persist, as the Xen wallclock is just used by domU
guests, and modifications to it do not modify the underlying CMOS RTC.

The x86_platform.set_wallclock hook can be used to keep the hardware
RTC synchronized (as on bare metal).  Because the Xen wallclock is now
kept synchronized by xen_pvclock_gtod_notify() on any system time
change, xen_set_wallclock() need not do this and dom0 can simply use
the native implementation.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 60b7d1f..0e69994 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -199,18 +199,7 @@ static void xen_get_wallclock(struct timespec *now)
 
 static int xen_set_wallclock(const struct timespec *now)
 {
-	struct xen_platform_op op;
-
-	/* do nothing for domU */
-	if (!xen_initial_domain())
-		return -1;
-
-	op.cmd = XENPF_settime;
-	op.u.settime.secs = now->tv_sec;
-	op.u.settime.nsecs = now->tv_nsec;
-	op.u.settime.system_time = xen_clocksource_read();
-
-	return HYPERVISOR_dom0_op(&op);
+	return -1;
 }
 
 static int xen_pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
@@ -531,7 +520,9 @@ void __init xen_init_time_ops(void)
 
 	x86_platform.calibrate_tsc = xen_tsc_khz;
 	x86_platform.get_wallclock = xen_get_wallclock;
-	x86_platform.set_wallclock = xen_set_wallclock;
+	/* Dom0 uses the native method to set the hardware RTC. */
+	if (!xen_initial_domain())
+		x86_platform.set_wallclock = xen_set_wallclock;
 }
 
 #ifdef CONFIG_XEN_PVHVM
-- 
1.7.2.5

Re: [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[John Stultz]

On 05/30/2013 07:25 AM, David Vrabel wrote:
> The kernel has limited support for updating the persistent clock or
> RTC when NTP is synced.  This has the following limitations:
>
> * The persistent clock is not updated on step changes.  This leaves a
>    window where it will be incorrect (while NTP resyncs).
>
> * Xen guests use the Xen wallclock as their persistent clock.  dom0
>    maintains this clock so it is persistent for domUs and not dom0
>    itself.


So I'm still skeptical of the urgency to the first patch in this series, 
as I feel its a little overzealous in trying to enforce strict 
RTC/system-time synchronization.  But that said, these patches are now 
done in a way that doesn't affect the timekeeping core, so I'm ok with 
stepping out of the way and leaving the decision to merge it up to the 
Xen maintainers (Modulo a few nits I still have).


> These series fixes the above limitations and depends on "x86: increase
> precision of x86_platform.get/set_wallclock()" which was previously
> posted.

This is the only area that will need some coordination cross the Xen 
tree and tip/timers/core (once that patch and the fix for it I have 
queued lands in -tip). The options here are:

* I queue these two patches with proper Xen maintainer's acks/review 
(possibly adding my grumbles to the commit)
* Wait until the requried patch lands tip/timers/core, then Xen 
maintainers merge tip/timers/core into their tree as well
* These patches get rewritten so they don't depend on the "increase 
precision" patch, then we sort out the merge in -next


The first is probably the easiest, but I do want to make sure that Xen 
maintainers agree that Xen really needs to be special here compared to 
every other platform and always enforce the RTC is synced with system time.


> [ On a related note, with CONFIG_HZ=1000 sync_cmos_clock() is always
>    scheduled ~3ms too late which causes it to repeatedly try to
>    reschedule in ~997 ms and ends up never calling
>    updated_persistent_clock().  With HZ=250, the error is ~1ms too late
>    which is close enough.
>
>    It is not clear where this systematic error comes from or whether
>    this is only a Xen specific bug.  I don't have time to investigate
>    right now. ]

I'd be very interested in hearing more about this issue!

thanks
-john

Re: [Xen-devel] [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[Jan Beulich]

>>> On 31.05.13 at 01:55, John Stultz <john.stultz@linaro.org> wrote:
> The first is probably the easiest, but I do want to make sure that Xen 
> maintainers agree that Xen really needs to be special here compared to 
> every other platform and always enforce the RTC is synced with system time.

Actually, during the v2 discussion I think we made clear that this
is not the goal: Sync-ing of the RTC should be just like native
does it. The only special thing Xen wants is that the hypervisor's
software wall clock wants to be kept up to date when NTP or
whatever else updates Dom0's wall clock.

Jan

Re: [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[Jan Beulich]

>>> On 31.05.13 at 01:55, John Stultz <john.stultz@linaro.org> wrote:
> The first is probably the easiest, but I do want to make sure that Xen 
> maintainers agree that Xen really needs to be special here compared to 
> every other platform and always enforce the RTC is synced with system time.

Actually, during the v2 discussion I think we made clear that this
is not the goal: Sync-ing of the RTC should be just like native
does it. The only special thing Xen wants is that the hypervisor's
software wall clock wants to be kept up to date when NTP or
whatever else updates Dom0's wall clock.

Jan

Re: [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

On 31/05/13 00:55, John Stultz wrote:
> On 05/30/2013 07:25 AM, David Vrabel wrote:
>> 
>> These series fixes the above limitations and depends on "x86: increase
>> precision of x86_platform.get/set_wallclock()" which was previously
>> posted.
> 
> This is the only area that will need some coordination cross the Xen
> tree and tip/timers/core (once that patch and the fix for it I have
> queued lands in -tip). The options here are:
> 
> * I queue these two patches with proper Xen maintainer's acks/review

I'm happy for this to be queues for 3.11.  Although the series does fix
a bug it's one that I don't think affects that many people (I don't
think I seen any bug reports from anyone else).

We're only rarely seeing the problem in our automated test system which
is not a typical setup.

David

Re: [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

On 31/05/13 00:55, John Stultz wrote:
> On 05/30/2013 07:25 AM, David Vrabel wrote:
>> 
>> These series fixes the above limitations and depends on "x86: increase
>> precision of x86_platform.get/set_wallclock()" which was previously
>> posted.
> 
> This is the only area that will need some coordination cross the Xen
> tree and tip/timers/core (once that patch and the fix for it I have
> queued lands in -tip). The options here are:
> 
> * I queue these two patches with proper Xen maintainer's acks/review

I'm happy for this to be queues for 3.11.  Although the series does fix
a bug it's one that I don't think affects that many people (I don't
think I seen any bug reports from anyone else).

We're only rarely seeing the problem in our automated test system which
is not a typical setup.

David

Re: [PATCHv3 0/2] xen: maintain an accurate persistent clock in more cases

[John Stultz]

On 05/30/2013 07:25 AM, David Vrabel wrote:
> The kernel has limited support for updating the persistent clock or
> RTC when NTP is synced.  This has the following limitations:
>
> * The persistent clock is not updated on step changes.  This leaves a
>    window where it will be incorrect (while NTP resyncs).
>
> * Xen guests use the Xen wallclock as their persistent clock.  dom0
>    maintains this clock so it is persistent for domUs and not dom0
>    itself.


So I'm still skeptical of the urgency to the first patch in this series, 
as I feel its a little overzealous in trying to enforce strict 
RTC/system-time synchronization.  But that said, these patches are now 
done in a way that doesn't affect the timekeeping core, so I'm ok with 
stepping out of the way and leaving the decision to merge it up to the 
Xen maintainers (Modulo a few nits I still have).


> These series fixes the above limitations and depends on "x86: increase
> precision of x86_platform.get/set_wallclock()" which was previously
> posted.

This is the only area that will need some coordination cross the Xen 
tree and tip/timers/core (once that patch and the fix for it I have 
queued lands in -tip). The options here are:

* I queue these two patches with proper Xen maintainer's acks/review 
(possibly adding my grumbles to the commit)
* Wait until the requried patch lands tip/timers/core, then Xen 
maintainers merge tip/timers/core into their tree as well
* These patches get rewritten so they don't depend on the "increase 
precision" patch, then we sort out the merge in -next


The first is probably the easiest, but I do want to make sure that Xen 
maintainers agree that Xen really needs to be special here compared to 
every other platform and always enforce the RTC is synced with system time.


> [ On a related note, with CONFIG_HZ=1000 sync_cmos_clock() is always
>    scheduled ~3ms too late which causes it to repeatedly try to
>    reschedule in ~997 ms and ends up never calling
>    updated_persistent_clock().  With HZ=250, the error is ~1ms too late
>    which is close enough.
>
>    It is not clear where this systematic error comes from or whether
>    this is only a Xen specific bug.  I don't have time to investigate
>    right now. ]

I'd be very interested in hearing more about this issue!

thanks
-john