[PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

Xen guests use the Xen wallclock as their persistent clock.  This is a
software only clock in the hypervisor that is used by guests instead
of a real hardware RTC.

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 but not dom0
  itself.

These limitations mean that guests started before NTP is synchronized
will start with an incorrect wallclock time and the hardware RTC will
not be updated (as on bare metal).

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

Changes since v4:

Dropped the change to disable non-boot CPUs during suspend on Xen as
migration downtime was too poor.  Instead, provide
hrtimers_late_resume() for use by Xen's resume code to replace the
call of clock_was_set().  Fix two unused variable warnings.

Changes since v3:

Add a new clock_was_set notifier chain. Use this instead of direct
calls to clock_was_set() from the timekeeping code.  Use this notifier
and a new timer to synchronize the Xen wallclock.

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

[PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[David Vrabel]

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

Xen suspends (and resumes) without disabling non-boot CPUs as doing so
adds considerable delay to live migrations.  A 4 VCPU guest had more
than 200 ms of additional downtime if disable_nonboot_cpus() was
called prior to suspending.

As a consequence, only high resolution timers on the current CPU are
retriggered when resuming.  The Xen resume path worked around this
with a call to clock_was_set() to retrigger timers on all the CPUs.

A subsequent change will make clock_was_set() internal to hrtimers so
add an new call (hrtimers_late_resume()) to do the same job.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/xen/manage.c    |    8 ++++++--
 include/linux/hrtimer.h |    1 +
 kernel/hrtimer.c        |    9 +++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 412b96c..75bc2d5 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -166,8 +166,12 @@ out_resume:
 
 	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
-	/* Make sure timer events get retriggered on all CPUs */
-	clock_was_set();
+	/*
+	 * syscore_resume() ends up calling hrtimer_resume() but this
+	 * only retriggers timer events on the current CPU.  We need
+	 * to retrigger the events on all the other CPUS.
+	 */
+	hrtimers_late_resume();
 
 out_thaw:
 #ifdef CONFIG_PREEMPT
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index d19a5c2..13df0fa 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -323,6 +323,7 @@ extern void timerfd_clock_was_set(void);
 static inline void timerfd_clock_was_set(void) { }
 #endif
 extern void hrtimers_resume(void);
+extern void hrtimers_late_resume(void);
 
 extern ktime_t ktime_get(void);
 extern ktime_t ktime_get_real(void);
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index fd4b13b..34384b4 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -784,6 +784,15 @@ void hrtimers_resume(void)
 	timerfd_clock_was_set();
 }
 
+/*
+ * If non-boot CPUs were online during resume, we need to retrigger
+ * the events for all the non-boot CPUs.
+ */
+void hrtimers_late_resume(void)
+{
+	clock_was_set();
+}
+
 static inline void timer_stats_hrtimer_set_start_info(struct hrtimer *timer)
 {
 #ifdef CONFIG_TIMER_STATS
-- 
1.7.2.5

[PATCH 2/4] time: add a notifier chain for when the system time is stepped

[David Vrabel]

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

The high resolution timer code gets notified of step changes to the
system time with clock_was_set() or clock_was_set_delayed() calls.  If
other parts of the kernel require similar notification there is no
clear place to hook into.

Add a clock_was_set atomic notifier chain
(clock_was_set_notifier_list) and call this in place of
clock_was_set().  If the timekeeping locks are held, the calls are
deferred to a new tasklet.

The hrtimer code adds a notifier block to this chain and uses it to
call (the now internal) clock_was_set().  Since the timekeeping code
does not call the chain from the timer irq clock_was_set_delayed() and
associated code can be removed.

For the delayed case, clock_was_set() used to be called at the
beginning of the hrtimer softirq and now it is called from a tasklet.
The tasklet softirq will be called before the hrtimer one so this
should give the same behaviour.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/hrtimer.h   |    7 -------
 include/linux/time.h      |    7 +++++++
 kernel/hrtimer.c          |   34 +++++++++++++---------------------
 kernel/time/timekeeping.c |   39 ++++++++++++++++++++++++++++++---------
 4 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 13df0fa..45e30f6 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -166,7 +166,6 @@ enum  hrtimer_base_type {
  * @lock:		lock protecting the base and associated clock bases
  *			and timers
  * @active_bases:	Bitfield to mark bases with active timers
- * @clock_was_set:	Indicates that clock was set from irq context.
  * @expires_next:	absolute time of the next event which was scheduled
  *			via clock_set_next_event()
  * @hres_active:	State of high resolution mode
@@ -180,7 +179,6 @@ enum  hrtimer_base_type {
 struct hrtimer_cpu_base {
 	raw_spinlock_t			lock;
 	unsigned int			active_bases;
-	unsigned int			clock_was_set;
 #ifdef CONFIG_HIGH_RES_TIMERS
 	ktime_t				expires_next;
 	int				hres_active;
@@ -289,8 +287,6 @@ extern void hrtimer_peek_ahead_timers(void);
 # define MONOTONIC_RES_NSEC	HIGH_RES_NSEC
 # define KTIME_MONOTONIC_RES	KTIME_HIGH_RES
 
-extern void clock_was_set_delayed(void);
-
 #else
 
 # define MONOTONIC_RES_NSEC	LOW_RES_NSEC
@@ -312,11 +308,8 @@ static inline int hrtimer_is_hres_active(struct hrtimer *timer)
 	return 0;
 }
 
-static inline void clock_was_set_delayed(void) { }
-
 #endif
 
-extern void clock_was_set(void);
 #ifdef CONFIG_TIMERFD
 extern void timerfd_clock_was_set(void);
 #else
diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..a0c08a7 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -184,6 +184,13 @@ extern void timekeeping_clocktai(struct timespec *ts);
 struct tms;
 extern void do_sys_times(struct tms *);
 
+struct notifier_block;
+
+/*
+ * Notifier chain called when system time is stepped.
+ */
+extern int register_clock_was_set_notifier(struct notifier_block *nb);
+
 /*
  * Similar to the struct tm in userspace <time.h>, but it needs to be here so
  * that the kernel source is self contained.
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 34384b4..a853f9b 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -721,19 +721,6 @@ static int hrtimer_switch_to_hres(void)
 	return 1;
 }
 
-/*
- * Called from timekeeping code to reprogramm the hrtimer interrupt
- * device. If called from the timer interrupt context we defer it to
- * softirq context.
- */
-void clock_was_set_delayed(void)
-{
-	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
-
-	cpu_base->clock_was_set = 1;
-	__raise_softirq_irqoff(HRTIMER_SOFTIRQ);
-}
-
 #else
 
 static inline int hrtimer_hres_active(void) { return 0; }
@@ -762,7 +749,7 @@ static inline void retrigger_next_event(void *arg) { }
  * resolution timer interrupts. On UP we just disable interrupts and
  * call the high resolution interrupt code.
  */
-void clock_was_set(void)
+static void clock_was_set(void)
 {
 #ifdef CONFIG_HIGH_RES_TIMERS
 	/* Retrigger the CPU local events everywhere */
@@ -1441,13 +1428,6 @@ void hrtimer_peek_ahead_timers(void)
 
 static void run_hrtimer_softirq(struct softirq_action *h)
 {
-	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
-
-	if (cpu_base->clock_was_set) {
-		cpu_base->clock_was_set = 0;
-		clock_was_set();
-	}
-
 	hrtimer_peek_ahead_timers();
 }
 
@@ -1785,11 +1765,23 @@ static struct notifier_block __cpuinitdata hrtimers_nb = {
 	.notifier_call = hrtimer_cpu_notify,
 };
 
+static int hrtimer_clock_was_set_notify(struct notifier_block *self,
+					unsigned long action, void *data)
+{
+	clock_was_set();
+	return NOTIFY_OK;
+}
+
+static struct notifier_block hrtimers_clock_was_set_nb = {
+	.notifier_call = hrtimer_clock_was_set_notify,
+};
+
 void __init hrtimers_init(void)
 {
 	hrtimer_cpu_notify(&hrtimers_nb, (unsigned long)CPU_UP_PREPARE,
 			  (void *)(long)smp_processor_id());
 	register_cpu_notifier(&hrtimers_nb);
+	register_clock_was_set_notifier(&hrtimers_clock_was_set_nb);
 #ifdef CONFIG_HIGH_RES_TIMERS
 	open_softirq(HRTIMER_SOFTIRQ, run_hrtimer_softirq);
 #endif
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..96c5c8e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -198,6 +198,30 @@ static inline s64 timekeeping_get_ns_raw(struct timekeeper *tk)
 	return nsec + get_arch_timeoffset();
 }
 
+static ATOMIC_NOTIFIER_HEAD(clock_was_set_notifier_list);
+
+int register_clock_was_set_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&clock_was_set_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(register_clock_was_set_notifier);
+
+static void timekeeping_clock_was_set(void)
+{
+	atomic_notifier_call_chain(&clock_was_set_notifier_list, 0, NULL);
+}
+
+static void timekeeping_clock_was_set_task(unsigned long d)
+{
+	timekeeping_clock_was_set();
+}
+DECLARE_TASKLET(clock_was_set_tasklet, timekeeping_clock_was_set_task, 0);
+
+static void timekeeping_clock_was_set_delayed(void)
+{
+	tasklet_schedule(&clock_was_set_tasklet);
+}
+
 static RAW_NOTIFIER_HEAD(pvclock_gtod_chain);
 
 static void update_pvclock_gtod(struct timekeeper *tk)
@@ -513,8 +537,7 @@ int do_settimeofday(const struct timespec *tv)
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
-	/* signal hrtimers about time change */
-	clock_was_set();
+	timekeeping_clock_was_set();
 
 	return 0;
 }
@@ -557,8 +580,7 @@ error: /* even if we error out, we forwarded the time, so call update */
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
-	/* signal hrtimers about time change */
-	clock_was_set();
+	timekeeping_clock_was_set();
 
 	return ret;
 }
@@ -607,7 +629,7 @@ void timekeeping_set_tai_offset(s32 tai_offset)
 	__timekeeping_set_tai_offset(tk, tai_offset);
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
-	clock_was_set();
+	timekeeping_clock_was_set();
 }
 
 /**
@@ -877,8 +899,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
-	/* signal hrtimers about time change */
-	clock_was_set();
+	timekeeping_clock_was_set();
 }
 
 /**
@@ -1260,7 +1281,7 @@ static inline void accumulate_nsecs_to_secs(struct timekeeper *tk)
 
 			__timekeeping_set_tai_offset(tk, tk->tai_offset - leap);
 
-			clock_was_set_delayed();
+			timekeeping_clock_was_set_delayed();
 		}
 	}
 }
@@ -1677,7 +1698,7 @@ int do_adjtimex(struct timex *txc)
 
 	if (tai != orig_tai) {
 		__timekeeping_set_tai_offset(tk, tai);
-		clock_was_set_delayed();
+		timekeeping_clock_was_set_delayed();
 	}
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
-- 
1.7.2.5

[PATCH 3/4] x86/xen: sync the wallclock when the system time is stepped

[David Vrabel]

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

The Xen wallclock is a software only clock within the Xen hypervisor
that is used by: a) PV guests as the equivalent of a hardware RTC; and
b) the hypervisor as the clock source for the emulated RTC provided to
HVM guests.

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 clock_was_set notifier chain to receive a notification when
the system time is stepped and update the wallclock to match the
current system time.

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

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..ad077ca 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -211,6 +211,25 @@ static int xen_set_wallclock(const struct timespec *now)
 
 	return HYPERVISOR_dom0_op(&op);
 }
+ 
+static int xen_clock_was_set_notify(struct notifier_block *nb, unsigned long unused,
+				    void *priv)
+{
+       struct timespec now;
+
+       /*
+        * Set the Xen wallclock from Linux system time.
+        */
+       now = current_kernel_time();
+       xen_set_wallclock(&now);
+
+       return NOTIFY_OK;
+}
+
+static struct notifier_block xen_clock_was_set_notifier = {
+	.notifier_call = xen_clock_was_set_notify,
+};
+
 
 static struct clocksource xen_clocksource __read_mostly = {
 	.name = "xen",
@@ -473,6 +492,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())
+		register_clock_was_set_notifier(&xen_clock_was_set_notifier);
 }
 
 void __init xen_init_time_ops(void)
-- 
1.7.2.5

[PATCH 4/4] 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 a software only clock
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 the clock_was_set notifier and a new timer,
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 |   48 +++++++++++++++++++++++++++++++-----------------
 1 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index ad077ca..f3d09eb 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -198,38 +198,50 @@ static void xen_get_wallclock(struct timespec *now)
 
 static int xen_set_wallclock(const struct timespec *now)
 {
+	return -1;
+}
+
+static void xen_wallclock_timer_func(unsigned long d);
+static DEFINE_TIMER(xen_wallclock_timer, xen_wallclock_timer_func, 0, 0);
+
+static void xen_sync_wallclock(void)
+{
+	struct timespec now;
 	struct xen_platform_op op;
 
-	/* do nothing for domU */
-	if (!xen_initial_domain())
-		return -1;
+	now = current_kernel_time();
 
 	op.cmd = XENPF_settime;
-	op.u.settime.secs = now->tv_sec;
-	op.u.settime.nsecs = now->tv_nsec;
+	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);
+	(void)HYPERVISOR_dom0_op(&op);
+
+	/*
+	 * Use a timer to correct for any drift in the Xen
+	 * wallclock.
+	 *
+	 * 11 minutes is the same period as sync_cmos_clock().
+	 */
+	mod_timer(&xen_wallclock_timer, round_jiffies(jiffies + 11*60*HZ));
 }
- 
+
 static int xen_clock_was_set_notify(struct notifier_block *nb, unsigned long unused,
 				    void *priv)
 {
-       struct timespec now;
-
-       /*
-        * Set the Xen wallclock from Linux system time.
-        */
-       now = current_kernel_time();
-       xen_set_wallclock(&now);
-
-       return NOTIFY_OK;
+	xen_sync_wallclock();
+	return NOTIFY_OK;
 }
 
 static struct notifier_block xen_clock_was_set_notifier = {
 	.notifier_call = xen_clock_was_set_notify,
 };
 
+static void xen_wallclock_timer_func(unsigned long d)
+{
+	xen_sync_wallclock();
+}
 
 static struct clocksource xen_clocksource __read_mostly = {
 	.name = "xen",
@@ -507,7 +519,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: [PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[John Stultz]

On 06/20/2013 12:16 PM, David Vrabel wrote:
> Xen guests use the Xen wallclock as their persistent clock.  This is a
> software only clock in the hypervisor that is used by guests instead
> of a real hardware RTC.
>
> 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 but not dom0
>    itself.
>
> These limitations mean that guests started before NTP is synchronized
> will start with an incorrect wallclock time and the hardware RTC will
> not be updated (as on bare metal).
>
> These series fixes the above limitations and depends on "x86: increase
> precision of x86_platform.get/set_wallclock()" which was previously
> posted.
>
> Changes since v4:
>
> Dropped the change to disable non-boot CPUs during suspend on Xen as
> migration downtime was too poor.  Instead, provide
> hrtimers_late_resume() for use by Xen's resume code to replace the
> call of clock_was_set().  Fix two unused variable warnings.

Ok, I've got these 4 in my pending stack. As long as Thomas doesn't 
object to the first two, and it doesn't run into any trouble in testing, 
I'll send them along for 3.12. (Acks from Xen maintainers would be nice 
for the last two as well).

Thanks for all the effort through all the revisions here!

thanks
-john

Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[Thomas Gleixner]

On Thu, 20 Jun 2013, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Xen suspends (and resumes) without disabling non-boot CPUs as doing so
> adds considerable delay to live migrations.  A 4 VCPU guest had more
> than 200 ms of additional downtime if disable_nonboot_cpus() was
> called prior to suspending.
> 
> As a consequence, only high resolution timers on the current CPU are
> retriggered when resuming.  The Xen resume path worked around this
> with a call to clock_was_set() to retrigger timers on all the CPUs.
> 
> A subsequent change will make clock_was_set() internal to hrtimers so
> add an new call (hrtimers_late_resume()) to do the same job.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/xen/manage.c    |    8 ++++++--
>  include/linux/hrtimer.h |    1 +
>  kernel/hrtimer.c        |    9 +++++++++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 412b96c..75bc2d5 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -166,8 +166,12 @@ out_resume:
>  
>  	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>  
> -	/* Make sure timer events get retriggered on all CPUs */
> -	clock_was_set();
> +	/*
> +	 * syscore_resume() ends up calling hrtimer_resume() but this
> +	 * only retriggers timer events on the current CPU.  We need
> +	 * to retrigger the events on all the other CPUS.
> +	 */
> +	hrtimers_late_resume();

This is the completely wrong approach. If an architecture does not
shut down the non boot cpus on suspend, then this wants to be handled
in the core code and not in some random arch specific driver.

Thanks,

	tglx

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[Thomas Gleixner]

On Thu, 20 Jun 2013, David Vrabel wrote:

> From: David Vrabel <david.vrabel@citrix.com>
> 
> The high resolution timer code gets notified of step changes to the
> system time with clock_was_set() or clock_was_set_delayed() calls.  If
> other parts of the kernel require similar notification there is no
> clear place to hook into.

You fail to explain why any other part of the kernel requires a
notification.
 
We went great length to confine timekeeping inside the core code and
now you add random notifiers along with totally ugly tasklet
constructs. What for?

Without a reasonable explanation of the problem you are trying to
solve this is going nowhere near the tree.

Thanks,

	tglx

Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[David Vrabel]

On 21/06/13 08:53, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Xen suspends (and resumes) without disabling non-boot CPUs as doing so
>> adds considerable delay to live migrations.  A 4 VCPU guest had more
>> than 200 ms of additional downtime if disable_nonboot_cpus() was
>> called prior to suspending.
>>
>> As a consequence, only high resolution timers on the current CPU are
>> retriggered when resuming.  The Xen resume path worked around this
>> with a call to clock_was_set() to retrigger timers on all the CPUs.
>>
>> A subsequent change will make clock_was_set() internal to hrtimers so
>> add an new call (hrtimers_late_resume()) to do the same job.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  drivers/xen/manage.c    |    8 ++++++--
>>  include/linux/hrtimer.h |    1 +
>>  kernel/hrtimer.c        |    9 +++++++++
>>  3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
>> index 412b96c..75bc2d5 100644
>> --- a/drivers/xen/manage.c
>> +++ b/drivers/xen/manage.c
>> @@ -166,8 +166,12 @@ out_resume:
>>  
>>  	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>>  
>> -	/* Make sure timer events get retriggered on all CPUs */
>> -	clock_was_set();
>> +	/*
>> +	 * syscore_resume() ends up calling hrtimer_resume() but this
>> +	 * only retriggers timer events on the current CPU.  We need
>> +	 * to retrigger the events on all the other CPUS.
>> +	 */
>> +	hrtimers_late_resume();
> 
> This is the completely wrong approach. If an architecture does not
> shut down the non boot cpus on suspend, then this wants to be handled
> in the core code and not in some random arch specific driver.

Agreed.  Does the following meet your requirements?

David

8<-------------------------------------------------------
hrtimers: support resuming with two or more CPUs online (but stopped)

hrtimers_resume() only reprograms the timers for the current CPU as it
assumes that all other CPUs are offline at this point in the resume
process.  If other CPUs are online then their timers will not be
corrected and they may fire at the wrong time.

When running as a Xen guest, this assumption is not true.  Non-boot
CPUs are only stopped with IRQs disabled instead of offlining them.
This is a performance optimization as disabling the CPUs would add an
unacceptable amount of additional downtime during a live migration (>
200 ms for a 4 VCPU guest).

hrtimers_resume() cannot call on_each_cpu(retrigger_next_event,...)
as the other CPUs will be stopped with IRQs disabled.  Instead, defer
the call to the next softirq.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/xen/manage.c |    3 ---
 kernel/hrtimer.c     |   10 +++++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 412b96c..421da85 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -166,9 +166,6 @@ out_resume:
 
 	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
-	/* Make sure timer events get retriggered on all CPUs */
-	clock_was_set();
-
 out_thaw:
 #ifdef CONFIG_PREEMPT
 	thaw_processes();
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index fd4b13b..74aa7c5 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -773,15 +773,19 @@ void clock_was_set(void)
 
 /*
  * During resume we might have to reprogram the high resolution timer
- * interrupt (on the local CPU):
+ * interrupt on all online CPUs.  However, all other CPUs will be
+ * stopped with IRQs interrupts disabled at this point so defer this
+ * to the softirq.
  */
 void hrtimers_resume(void)
 {
+	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
+
 	WARN_ONCE(!irqs_disabled(),
 		  KERN_INFO "hrtimers_resume() called with IRQs enabled!");
 
-	retrigger_next_event(NULL);
-	timerfd_clock_was_set();
+	cpu_base->clock_was_set = 1;
+	__raise_softirq_irqoff(HRTIMER_SOFTIRQ);
 }
 
 static inline void timer_stats_hrtimer_set_start_info(struct hrtimer *timer)
-- 
1.7.2.5

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[David Vrabel]

On 21/06/13 08:57, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, David Vrabel wrote:
> 
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> The high resolution timer code gets notified of step changes to the
>> system time with clock_was_set() or clock_was_set_delayed() calls.  If
>> other parts of the kernel require similar notification there is no
>> clear place to hook into.
> 
> You fail to explain why any other part of the kernel requires a
> notification.

This is needed by patch 3 in this series.

"The Xen wallclock is a software only clock within the Xen hypervisor
that is used by: a) PV guests as the equivalent of a hardware RTC; and
b) the hypervisor as the clock source for the emulated RTC provided to
HVM guests.

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 clock_was_set notifier chain to receive a notification when
the system time is stepped and update the wallclock to match the
current system time."

> We went great length to confine timekeeping inside the core code and
> now you add random notifiers along with totally ugly tasklet
> constructs.

I'm not sure I understand your objection to the use of a tasklet.  Using
the hrtimer softirq for something that is no longer hrtimer-specific did
not seem like the correct thing to do.

David

Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[Thomas Gleixner]

On Fri, 21 Jun 2013, David Vrabel wrote:
> On 21/06/13 08:53, Thomas Gleixner wrote:
> > This is the completely wrong approach. If an architecture does not
> > shut down the non boot cpus on suspend, then this wants to be handled
> > in the core code and not in some random arch specific driver.
> 
> Agreed.  Does the following meet your requirements?

Indeed. That's looks way more reasonable. Though...

> hrtimers_resume() cannot call on_each_cpu(retrigger_next_event,...)
> as the other CPUs will be stopped with IRQs disabled.  Instead, defer
> the call to the next softirq.

that's just working by chance and not by design as there is no
guarantee that the next interrupt, which invokes the softirq, will
arrive in time. So you want to make sure that an interrupt arrives.

Invoking retrigger_next_event(NULL) from hrtimer_resume() should do
the trick.

Thanks,

	tglx

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[John Stultz]

On 06/21/2013 12:57 AM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, David Vrabel wrote:
>
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> The high resolution timer code gets notified of step changes to the
>> system time with clock_was_set() or clock_was_set_delayed() calls.  If
>> other parts of the kernel require similar notification there is no
>> clear place to hook into.
> You fail to explain why any other part of the kernel requires a
> notification.
>   
> We went great length to confine timekeeping inside the core code and
> now you add random notifiers along with totally ugly tasklet
> constructs. What for?
>
> Without a reasonable explanation of the problem you are trying to
> solve this is going nowhere near the tree.

It took awhile for me to get why it was needed as well.

My understanding is that on Xen, they want the hypervisor's virtual RTC 
needs to be aligned with Dom0s system time (so that DomN guests boot 
with the correct time). Thus changes to the system time need to cause 
Dom0 to update the virtual RTC.

Its not terribly unlike the timerfd notification we do to userspace, but 
instead is done for the Dom0 Xen management code.

I do agree we need to keep users of the notification on a short leash 
(hopefully keeping the interface behind some sort of internal.h header 
file) so its easy to see when folks start trying to use it.

thanks
-john

Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[David Vrabel]

On 21/06/13 15:32, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, David Vrabel wrote:
>> On 21/06/13 08:53, Thomas Gleixner wrote:
>>> This is the completely wrong approach. If an architecture does not
>>> shut down the non boot cpus on suspend, then this wants to be handled
>>> in the core code and not in some random arch specific driver.
>>
>> Agreed.  Does the following meet your requirements?
> 
> Indeed. That's looks way more reasonable. Though...
> 
>> hrtimers_resume() cannot call on_each_cpu(retrigger_next_event,...)
>> as the other CPUs will be stopped with IRQs disabled.  Instead, defer
>> the call to the next softirq.
> 
> that's just working by chance and not by design as there is no
> guarantee that the next interrupt, which invokes the softirq, will
> arrive in time. So you want to make sure that an interrupt arrives.

That is a good point.

> Invoking retrigger_next_event(NULL) from hrtimer_resume() should do
> the trick.

But I'm not sure that would be sufficient, although I may not be
understanding how the hrtimers work or are used.

There may be timers on other CPUs that are supposed to fire earlier than
those on the current CPU.  There may even be no timers scheduled on the
current CPU.

I think there needs to be something like:

hrtimers_resume()
{
    retrigger_next_event(NULL);

    /* Timers on other CPUs might expire earlier.
       Program an earlier event and use this to kick the softirq to
       correctly reprogram the events on the other CPUs. */
    expires_next = cpu_base->expires_next;
    for_each_online_cpu(cpu) {
        expires = hrtimers_next_event_on_cpu(cpu)
        if (expires < expires_next)
            expires_next = expires;
   }
   tick_program_event(expires_next, 1);
   cpu_base->clock_was_set = 1;
   __raise_softirq_irqoff(HRTIMER_SOFTIRQ);
}

However, since hrtimers require the use of a one-shot ticker and when
one-shot timers are resumed they are armed to fire immediately (see
tick_resume_oneshot()) this interrupt is sufficient to kick the require
softirq.

So, as proposed before:

hrtimers_resume()
{
   /* This CPU's tick is armed to fire immediately by
      tick_oneshot_resume(). Just need raise a softirq to program
      the timers on all CPUs. */
   cpu_base->clock_was_set = 1;
   __raise_softirq_irqoff(HRTIMER_SOFTIRQ);
}

Do you agree or disagree?

David

Re: [PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[Konrad Rzeszutek Wilk]

On Thu, Jun 20, 2013 at 01:03:34PM -0700, John Stultz wrote:
> On 06/20/2013 12:16 PM, David Vrabel wrote:
> >Xen guests use the Xen wallclock as their persistent clock.  This is a
> >software only clock in the hypervisor that is used by guests instead
> >of a real hardware RTC.
> >
> >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 but not dom0
> >   itself.
> >
> >These limitations mean that guests started before NTP is synchronized
> >will start with an incorrect wallclock time and the hardware RTC will
> >not be updated (as on bare metal).
> >
> >These series fixes the above limitations and depends on "x86: increase
> >precision of x86_platform.get/set_wallclock()" which was previously
> >posted.
> >
> >Changes since v4:
> >
> >Dropped the change to disable non-boot CPUs during suspend on Xen as
> >migration downtime was too poor.  Instead, provide
> >hrtimers_late_resume() for use by Xen's resume code to replace the
> >call of clock_was_set().  Fix two unused variable warnings.
> 
> Ok, I've got these 4 in my pending stack. As long as Thomas doesn't
> object to the first two, and it doesn't run into any trouble in
> testing, I'll send them along for 3.12. (Acks from Xen maintainers
> would be nice for the last two as well).

Please consider them Acked-by.

Thanks!
> 
> Thanks for all the effort through all the revisions here!
> 
> thanks
> -john
> 

Re: [PATCH 1/4] hrtimers: provide a hrtimers_late_resume() call

[Thomas Gleixner]

On Fri, 21 Jun 2013, David Vrabel wrote:
> On 21/06/13 15:32, Thomas Gleixner wrote:
> However, since hrtimers require the use of a one-shot ticker and when
> one-shot timers are resumed they are armed to fire immediately (see
> tick_resume_oneshot()) this interrupt is sufficient to kick the require
> softirq.
> 
> So, as proposed before:
> 
> hrtimers_resume()
> {
>    /* This CPU's tick is armed to fire immediately by
>       tick_oneshot_resume(). Just need raise a softirq to program
>       the timers on all CPUs. */
>    cpu_base->clock_was_set = 1;
>    __raise_softirq_irqoff(HRTIMER_SOFTIRQ);
> }
> 
> Do you agree or disagree?

Fair enough. I did not think about that. With the comment in place it
is clear. It might be a bit more elaborate for the casual reader.

Thanks,

	tglx

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[Thomas Gleixner]

On Fri, 21 Jun 2013, David Vrabel wrote:
> On 21/06/13 08:57, Thomas Gleixner wrote:
> > On Thu, 20 Jun 2013, David Vrabel wrote:
> > 
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> The high resolution timer code gets notified of step changes to the
> >> system time with clock_was_set() or clock_was_set_delayed() calls.  If
> >> other parts of the kernel require similar notification there is no
> >> clear place to hook into.
> > 
> > You fail to explain why any other part of the kernel requires a
> > notification.
> 
> This is needed by patch 3 in this series.
> 
> "The Xen wallclock is a software only clock within the Xen hypervisor
> that is used by: a) PV guests as the equivalent of a hardware RTC; and
> b) the hypervisor as the clock source for the emulated RTC provided to
> HVM guests.
> 
> 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.

What you are saying is, that you are fixing Xens failure to implement
a proper RTC emulation by hacking a notifier into the core code. You
can't be serious about that.

According to your changelog:

  Currently the Xen wallclock is only updated every 11 minutes if NTP is
  synchronized to its clocksource.

How is that related to clock_was_set() ?

clock_was_set*() is invoked from:

	do_settimeofday()
	timekeeping_inject_offset()
	timekeeping_set_tai_offset()
	timekeeping_inject_sleeptime()
	update_wall_time()
	do_adjtimex()

The only function which calls clock_was_set() and can affect RTC is
do_adjtimex(). Though you claim that the natural place to add a
notifier is clock_was_set().

So you went the other way round this time. In the hrtimers case you
tried to fix shortcomings of the core code in some random Xen
code. With this patch you try to fix Xen nonsense in the core code.

Can you please provide a proper explanation of the problem you are
trying to solve? This means that you should explain the semantics of
the desired XEN RTC emulation and not the desired workarounds to fix
the shortcommings current implementation.

Thanks,

	tglx

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[David Vrabel]

On 22/06/13 00:06, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, David Vrabel wrote:
>> On 21/06/13 08:57, Thomas Gleixner wrote:
>>> On Thu, 20 Jun 2013, David Vrabel wrote:
>>>
>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>
>>>> The high resolution timer code gets notified of step changes to the
>>>> system time with clock_was_set() or clock_was_set_delayed() calls.  If
>>>> other parts of the kernel require similar notification there is no
>>>> clear place to hook into.
>>>
>>> You fail to explain why any other part of the kernel requires a
>>> notification.
>>
>> This is needed by patch 3 in this series.
>>
>> "The Xen wallclock is a software only clock within the Xen hypervisor
>> that is used by: a) PV guests as the equivalent of a hardware RTC; and
>> b) the hypervisor as the clock source for the emulated RTC provided to
>> HVM guests.
>>
>> 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.
> 
> What you are saying is, that you are fixing Xens failure to implement
> a proper RTC emulation by hacking a notifier into the core code. You
> can't be serious about that.

Xen does provide proper emulation of an RTC for guests.  Both full
hardware emulation for fully-virtualized guest (HVM) and a lighter
weight interface for paravirtualized guests (PV).

As with any emulated RTC, there needs to be underlying clocksource
providing the time.  Under Xen, this is the Xen wallclock and it is
implemented as a record of the date/time timestamped with the
corresponding Xen clocksource value[1].

KVM provides an identical wallclock to its guests -- see the common
pvclock_read_wallclock() function.

Xen has hardware drivers for only the minimal amount of hardware
necessary for the scheduling and isolation of guests.  It does not have
drivers for any hardware RTC nor does it have a network stack or an
implementation of NTP.  Therefore it has no way to maintain the
correctness of the Xen wallclock and relies on the control domain (dom0)
to do this.

Dom0 updates the Xen wallclock with the XENPF_settime platform_op hypercall.

To ensure the correctness of the Xen wallclock it is kept in sync with
dom0's system time (which is assumed to be correct and would typically
be corrected by NTP).

This requires that the Xen wallclock is both:

a) updated on step changes to system time.
b) updated periodically to correct for any drift.

This behaviour (keeping the wallclock in sync with dom0 system time) is
part of the ABI provided by the kernel and changing it would break
existing user space.

This patch set is fixing the rare case where a guest is started before
NTP has synced and thus sees an incorrect wallclock time which may cause
the guest to fail to boot.

> According to your changelog:
> 
>   Currently the Xen wallclock is only updated every 11 minutes if NTP is
>   synchronized to its clocksource.
> 
> How is that related to clock_was_set() ?

It's not. This is the update_persistent_clock() call from the periodic
sync_cmos_clock() work.

> clock_was_set*() is invoked from:
> 
> 	do_settimeofday()
> 	timekeeping_inject_offset()
> 	timekeeping_set_tai_offset()
> 	timekeeping_inject_sleeptime()
> 	update_wall_time()
> 	do_adjtimex()
> 
> The only function which calls clock_was_set() and can affect RTC is
> do_adjtimex(). Though you claim that the natural place to add a
> notifier is clock_was_set().
> 
> So you went the other way round this time. In the hrtimers case you
> tried to fix shortcomings of the core code in some random Xen
> code. With this patch you try to fix Xen nonsense in the core code.

KVM uses a very similar mechanism to maintain system time for a guest so
guest system time is synchronized with host system time.  See the
pvclock_gtod notifier chain and its usage in arch/x86/kvm/x86.c.

v3 of this series did use this existing notifier but it is called on
every timer tick so this is more expensive than necessary to meet the
requirements (see (a) and (b) above) for maintaining the Xen wallclock.
 John suggested hooking into clock_was_set().

> Can you please provide a proper explanation of the problem you are
> trying to solve? This means that you should explain the semantics of
> the desired XEN RTC emulation and not the desired workarounds to fix
> the shortcommings current implementation.

In summary, both Xen and KVM need to solve similar problems with keeping
time synchronized between a host and guests.

The key difference between the two hypervisors is that Xen synchronizes
the wallclock and KVM synchronizes system time.

David

[1] The Xen clocksource is monotonic, nanosecond resolution clocksource
provided by Xen for use internally and by guests.

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[Thomas Gleixner]

On Mon, 24 Jun 2013, David Vrabel wrote:
> On 22/06/13 00:06, Thomas Gleixner wrote:
> This patch set is fixing the rare case where a guest is started before
> NTP has synced and thus sees an incorrect wallclock time which may cause
> the guest to fail to boot.

You're not fixing it, you are just making the window smaller.

clock_was_set() is called outside of the timekeeper_lock protected
regions, so what prevents the guest to start before the notifier is
invoked?

We already have a synchronous notifier in place and the notifier call
itself is not expensive. What's expensive is the hypercall and there
is no way at the moment to figure out whether the update is relevant
for you or just a tick. Though that's trivial information to provide
without imposing another notifier including the surrounding mess on
the core code.

Completely untested patch below.

Thanks,

	tglx
---
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index baeeb5c..6e9f838 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -200,9 +200,9 @@ static inline s64 timekeeping_get_ns_raw(struct timekeeper *tk)
 
 static RAW_NOTIFIER_HEAD(pvclock_gtod_chain);
 
-static void update_pvclock_gtod(struct timekeeper *tk)
+static void update_pvclock_gtod(struct timekeeper *tk, bool cws)
 {
-	raw_notifier_call_chain(&pvclock_gtod_chain, 0, tk);
+	raw_notifier_call_chain(&pvclock_gtod_chain, cws, tk);
 }
 
 /**
@@ -216,7 +216,7 @@ int pvclock_gtod_register_notifier(struct notifier_block *nb)
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	ret = raw_notifier_chain_register(&pvclock_gtod_chain, nb);
-	update_pvclock_gtod(tk);
+	update_pvclock_gtod(tk, true);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
 	return ret;
@@ -241,14 +241,15 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
 EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
 
 /* must hold timekeeper_lock */
-static void timekeeping_update(struct timekeeper *tk, bool clearntp, bool mirror)
+static void timekeeping_update(struct timekeeper *tk, bool clearntp,
+			       bool mirror, bool cws)
 {
 	if (clearntp) {
 		tk->ntp_error = 0;
 		ntp_clear();
 	}
 	update_vsyscall(tk);
-	update_pvclock_gtod(tk);
+	update_pvclock_gtod(tk, cws);
 
 	if (mirror)
 		memcpy(&shadow_timekeeper, &timekeeper, sizeof(timekeeper));
@@ -508,7 +509,7 @@ int do_settimeofday(const struct timespec *tv)
 
 	tk_set_xtime(tk, tv);
 
-	timekeeping_update(tk, true, true);
+	timekeeping_update(tk, true, true, true);
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -552,7 +553,7 @@ int timekeeping_inject_offset(struct timespec *ts)
 	tk_set_wall_to_mono(tk, timespec_sub(tk->wall_to_monotonic, *ts));
 
 error: /* even if we error out, we forwarded the time, so call update */
-	timekeeping_update(tk, true, true);
+	timekeeping_update(tk, true, true, true);
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -633,7 +634,7 @@ static int change_clocksource(void *data)
 		if (old->disable)
 			old->disable(old);
 	}
-	timekeeping_update(tk, true, true);
+	timekeeping_update(tk, true, true, true);
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -872,7 +873,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 
 	__timekeeping_inject_sleeptime(tk, delta);
 
-	timekeeping_update(tk, true, true);
+	timekeeping_update(tk, true, true, true);
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -954,7 +955,7 @@ static void timekeeping_resume(void)
 	tk->cycle_last = clock->cycle_last = cycle_now;
 	tk->ntp_error = 0;
 	timekeeping_suspended = 0;
-	timekeeping_update(tk, false, true);
+	timekeeping_update(tk, false, true, true);
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
@@ -1236,9 +1237,10 @@ out_adjust:
  * It also calls into the NTP code to handle leapsecond processing.
  *
  */
-static inline void accumulate_nsecs_to_secs(struct timekeeper *tk)
+static inline bool accumulate_nsecs_to_secs(struct timekeeper *tk)
 {
 	u64 nsecps = (u64)NSEC_PER_SEC << tk->shift;
+	bool ret = false;
 
 	while (tk->xtime_nsec >= nsecps) {
 		int leap;
@@ -1261,8 +1263,10 @@ static inline void accumulate_nsecs_to_secs(struct timekeeper *tk)
 			__timekeeping_set_tai_offset(tk, tk->tai_offset - leap);
 
 			clock_was_set_delayed();
+			ret = true;
 		}
 	}
+	return ret;
 }
 
 /**
@@ -1348,6 +1352,7 @@ static void update_wall_time(void)
 	cycle_t offset;
 	int shift = 0, maxshift;
 	unsigned long flags;
+	bool cws;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 
@@ -1399,7 +1404,7 @@ static void update_wall_time(void)
 	 * Finally, make sure that after the rounding
 	 * xtime_nsec isn't larger than NSEC_PER_SEC
 	 */
-	accumulate_nsecs_to_secs(tk);
+	cws = accumulate_nsecs_to_secs(tk);
 
 	write_seqcount_begin(&timekeeper_seq);
 	/* Update clock->cycle_last with the new value */
@@ -1415,7 +1420,7 @@ static void update_wall_time(void)
 	 * updating.
 	 */
 	memcpy(real_tk, tk, sizeof(*tk));
-	timekeeping_update(real_tk, false, false);
+	timekeeping_update(real_tk, false, false, cws);
 	write_seqcount_end(&timekeeper_seq);
 out:
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -1677,6 +1682,7 @@ int do_adjtimex(struct timex *txc)
 
 	if (tai != orig_tai) {
 		__timekeeping_set_tai_offset(tk, tai);
+		update_pvclock_gtod(tk, true);
 		clock_was_set_delayed();
 	}
 	write_seqcount_end(&timekeeper_seq);

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[David Vrabel]

On 24/06/13 17:30, Thomas Gleixner wrote:
> 
> We already have a synchronous notifier in place and the notifier call
> itself is not expensive. What's expensive is the hypercall and there
> is no way at the moment to figure out whether the update is relevant
> for you or just a tick. Though that's trivial information to provide
> without imposing another notifier including the surrounding mess on
> the core code.

This looks good, thanks.

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
[...]
> @@ -508,7 +509,7 @@ int do_settimeofday(const struct timespec *tv)
>  
>  	tk_set_xtime(tk, tv);
>  
> -	timekeeping_update(tk, true, true);
> +	timekeeping_update(tk, true, true, true);

These three booleans in a row is getting a bit opaque. How about I also
change it to a set of flags?  e.g.,

timekeeping_updated(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);

David

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[John Stultz]

On 06/24/2013 10:00 AM, David Vrabel wrote:
> On 24/06/13 17:30, Thomas Gleixner wrote:
>> @@ -508,7 +509,7 @@ int do_settimeofday(const struct timespec *tv)
>>   
>>   	tk_set_xtime(tk, tv);
>>   
>> -	timekeeping_update(tk, true, true);
>> +	timekeeping_update(tk, true, true, true);
> These three booleans in a row is getting a bit opaque. How about I also
> change it to a set of flags?  e.g.,
>
> timekeeping_updated(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);

Yea. I'm not a fan of the bool arguments to functions (which I have to 
look up every time as which bool is which).

The bitflag approach is nicer in my mind, since its a bit more explicit 
when reading the code.

The other approach would be to have different function calls 
(timekeeping_clear_ntp, timekeeping_mirror, timekeeping_clock_was_set), 
which call into the same back end logic.

thanks
-john

Re: [PATCH 2/4] time: add a notifier chain for when the system time is stepped

[Thomas Gleixner]

On Mon, 24 Jun 2013, David Vrabel wrote:

> On 24/06/13 17:30, Thomas Gleixner wrote:
> > 
> > We already have a synchronous notifier in place and the notifier call
> > itself is not expensive. What's expensive is the hypercall and there
> > is no way at the moment to figure out whether the update is relevant
> > for you or just a tick. Though that's trivial information to provide
> > without imposing another notifier including the surrounding mess on
> > the core code.
> 
> This looks good, thanks.
> 
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> [...]
> > @@ -508,7 +509,7 @@ int do_settimeofday(const struct timespec *tv)
> >  
> >  	tk_set_xtime(tk, tv);
> >  
> > -	timekeeping_update(tk, true, true);
> > +	timekeeping_update(tk, true, true, true);
> 
> These three booleans in a row is getting a bit opaque. How about I also
> change it to a set of flags?  e.g.,
> 
> timekeeping_updated(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);

Fair enough. Can you convert the existing booleans first and then put
the CLOCK_WAS_SET patch on top of that?

Thanks,

	tglx

Re: [PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

On 20/06/13 20:13, David Vrabel wrote:
> Xen guests use the Xen wallclock as their persistent clock.  This is a
> software only clock in the hypervisor that is used by guests instead
> of a real hardware RTC.

Sorry, I accidentally specified the wrong revision range and only
include 2 out 4 of the patches in the series.

I have resent the complete series.

David

[PATCHv5 0/4] xen: maintain an accurate persistent clock in more cases

[David Vrabel]

Xen guests use the Xen wallclock as their persistent clock.  This is a
software only clock in the hypervisor that is used by guests instead
of a real hardware RTC.

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 but not dom0
  itself.

These limitations mean that guests started before NTP is synchronized
will start with an incorrect wallclock time and the hardware RTC will
not be updated (as on bare metal).

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

Changes since v4:

Dropped the change to disable non-boot CPUs during suspend on Xen as
migration downtime was too poor.  Instead, provide
hrtimers_late_resume() for use by Xen's resume code to replace the
call of clock_was_set().  Fix two unused variable warnings.

Changes since v3:

Add a new clock_was_set notifier chain. Use this instead of direct
calls to clock_was_set() from the timekeeping code.  Use this notifier
and a new timer to synchronize the Xen wallclock.

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