[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/2] 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/2] 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

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

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: [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

[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