hv_util: adjust system time smoothly

While do_adjtimex() is available to userspace via adjtimex syscall it is not available to modules which may want to implement in-kernel 'NTP clients'. Hyper-V hv_utils is going to be the first one.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 kernel/time/timekeeping.c | 1 +
 1 file changed, 1 insertion(+)

kernel/time/timekeeping.c +0 -0

index da233cd..ae4f24f 100644

Quoted 15 lines · Vitaly Kuznetsov

While do_adjtimex() is available to userspace via adjtimex syscall it is not available to modules which may want to implement in-kernel 'NTP clients'. Hyper-V hv_utils is going to be the first one.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 kernel/time/timekeeping.c | 1 +
 1 file changed, 1 insertion(+)

kernel/time/timekeeping.c +0 -0

index da233cd..ae4f24f 100644
@@ -2312,6 +2312,7 @@ int do_adjtimex(struct timex *txc)
 

kernel/time/timekeeping.c int do_adjtimex(struct timex *txc) · line 2315

Quoted 3 lines

kernel/time/timekeeping.c +1 -0

        return ret;
 }
+EXPORT_SYMBOL_GPL(do_adjtimex);

No real objections to this, although I do want to better understand the benefits (and drawbacks) of doing the adjtimex in the kernel driver rather then via userspace, to make sure the need is sane.

do_settimeofday() is deprecated, use do_settimeofday64() instead.

Signed-off-by Vitaly Kuznetsov <[email protected]>

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP client is run in parallel things may go south, e.g. when an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY the Hyper-V module will not see this changes and time will oscillate and never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Instead of calling do_settimeofday64() we can pretend being an NTP client and use do_adjtimex(). Do do_settimeofday64() in case the difference is too big or ICTIMESYNCFLAG_SYNC flag was set in the request.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
Changes since v1:
- do do_settimeofday64() when ICTIMESYNCFLAG_SYNC flag is present in the request (Alex Ng)
- add pr_debug() for the case when do_adjtimex() fails (Alex Ng)

---
 drivers/hv/hv_util.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

drivers/hv/hv_util.c +4 -2

index 94719eb..7e97231 100644
@@ -182,9 +182,11 @@ struct adj_time_work {
 static void hv_set_host_time(struct work_struct *work)
 {
 	struct adj_time_work	*wrk;
-	s64 host_tns;
+	s64 host_tns, our_tns, delta;
 	u64 newtime;
-	struct timespec64 host_ts;
+	struct timespec64 host_ts, our_ts;
+	struct timex txc = {0};
+	int ret;
 
 	wrk = container_of(work, struct adj_time_work, work);
 

Quoted 49 lines · On Wed, 4 Jan 2017 18:24:38 +0100 Vitaly Kuznetsov <[email protected]> wrote:

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP client is run in parallel things may go south, e.g. when an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY the Hyper-V module will not see this changes and time will oscillate and never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Instead of calling do_settimeofday64() we can pretend being an NTP client and use do_adjtimex(). Do do_settimeofday64() in case the difference is too big or ICTIMESYNCFLAG_SYNC flag was set in the request.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
Changes since v1:
- do do_settimeofday64() when ICTIMESYNCFLAG_SYNC flag is present in the request (Alex Ng)
- add pr_debug() for the case when do_adjtimex() fails (Alex Ng)

---
 drivers/hv/hv_util.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

drivers/hv/hv_util.c +4 -2

index 94719eb..7e97231 100644
@@ -182,9 +182,11 @@ struct adj_time_work {
 static void hv_set_host_time(struct work_struct *work)
 {
 	struct adj_time_work	*wrk;
-	s64 host_tns;
+	s64 host_tns, our_tns, delta;
 	u64 newtime;
-	struct timespec64 host_ts;
+	struct timespec64 host_ts, our_ts;
+	struct timex txc = {0};
+	int ret;
 
 	wrk = container_of(work, struct adj_time_work, work);
 

@@ -205,7 +207,31 @@ static void hv_set_host_time(struct work_struct *work)
 	host_tns = (newtime - WLTIMEDELTA) * 100;
 	host_ts = ns_to_timespec64(host_tns);
 

drivers/hv/hv_util.c static void hv_set_host_time(struct work_struct *work) · line 214

Quoted 6 lines

drivers/hv/hv_util.c +5 -1

-	do_settimeofday64(&host_ts);
+	getnstimeofday64(&our_ts);
+	our_tns = timespec64_to_ns(&our_ts);
+
+	/* Difference between our time and host time */
+	delta = host_tns - our_tns;

This looks correct to me.
Did you consider using ktime? It provides a cleaner abstraction for handling nanosecond time resolution.

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP client is run in parallel things may go south, e.g. when an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY the Hyper-V module will not see this changes and time will oscillate and never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Instead of calling do_settimeofday64() we can pretend being an NTP client and use do_adjtimex(). Do do_settimeofday64() in case the difference is too big or ICTIMESYNCFLAG_SYNC flag was set in the request.

If we happen to receive interrupts during hv_set_host_time() execution our adjustments may get inaccurate. Make the whole function atomic. Unfortunately, we can's call do_settimeofday64() with interrupts disabled as some cross-CPU work is being done but this call happens very rarely.

Signed-off-by Vitaly Kuznetsov <[email protected]>

Quoted 7 lines · On Wed, 4 Jan 2017 18:24:39 +0100 Vitaly Kuznetsov <[email protected]> wrote:

If we happen to receive interrupts during hv_set_host_time() execution our adjustments may get inaccurate. Make the whole function atomic. Unfortunately, we can's call do_settimeofday64() with interrupts disabled as some cross-CPU work is being done but this call happens very rarely.

Signed-off-by Vitaly Kuznetsov <[email protected]>

Ok, the race is between timer interrupts and calling do_adjtimex(). NTP has the same issue already.

The getnstimeofday64() (or ktime_get) return an atomic value. If a clock tick interrupt happens during this code, then the value is still correct just old.

If you want to avoid all races here, it looks like it would be better to get timekeeper_lock and call __do_adjtimex. The existing code in do_adjtimex() is expecting to be called from a system call and changing it's assumptions is probably not a good idea.

Rather than calling system call from user space. Maybe better to provide real kernel API in time subsystem for this use case. What does KVM do?

I was thinking about it but to me what do_adjtimex() does looks too low-level for drivers (e.g. calling write_seqcount_begin(), __timekeeping_set_tai_offset(), tk_update_leap_state()). To me (again, I probably know not that much about time keeping) it looks like we'll have to have all this stuff around the __do_adjtimex() call here.

Are there any particular concearns on calling do_adjtimex() directly?

If we happen to receive interrupts during hv_set_host_time() execution our adjustments may get inaccurate. Make the whole function atomic. Unfortunately, we can's call do_settimeofday64() with interrupts disabled as some cross-CPU work is being done but this call happens very rarely.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 drivers/hv/hv_util.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/hv/hv_util.c +4 -0

index 7e97231..4e50a42 100644
@@ -187,6 +187,9 @@ static void hv_set_host_time(struct work_struct *work)
        struct timespec64 host_ts, our_ts;
        struct timex txc = {0};
        int ret;
+       unsigned long flags;
+
+       local_irq_save(flags);
 
        wrk = container_of(work, struct adj_time_work, work);
 

@@ -218,6 +221,7 @@ static void hv_set_host_time(struct work_struct *work)
         * ordered to sync our time by the host.
         */
        if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) {
+               local_irq_restore(flags);
                do_settimeofday64(&host_ts);
                return;
        }

Quoted 37 lines · Vitaly Kuznetsov

If we happen to receive interrupts during hv_set_host_time() execution our adjustments may get inaccurate. Make the whole function atomic. Unfortunately, we can's call do_settimeofday64() with interrupts disabled as some cross-CPU work is being done but this call happens very rarely.

Signed-off-by Vitaly Kuznetsov <[email protected]>

---
 drivers/hv/hv_util.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/hv/hv_util.c +4 -0

index 7e97231..4e50a42 100644
@@ -187,6 +187,9 @@ static void hv_set_host_time(struct work_struct *work)
        struct timespec64 host_ts, our_ts;
        struct timex txc = {0};
        int ret;
+       unsigned long flags;
+
+       local_irq_save(flags);
 
        wrk = container_of(work, struct adj_time_work, work);
 

@@ -218,6 +221,7 @@ static void hv_set_host_time(struct work_struct *work)
         * ordered to sync our time by the host.
         */
        if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) {
+               local_irq_restore(flags);
                do_settimeofday64(&host_ts);
                return;
        }

@@ -232,6 +236,8 @@ static void hv_set_host_time(struct work_struct *work)
        ret = do_adjtimex(&txc);
        if (ret)
                pr_debug("Failed to adjust system time: %d\n", ret);

drivers/hv/hv_util.c static void hv_set_host_time(struct work_struct *work) · line 240

Quoted 2 lines

drivers/hv/hv_util.c +2 -0

+
+       local_irq_restore(flags);

This seems like a long time to disable irqs for what your trying to do. I'd guess you really only want to disable irqs while you aquire the host and guest timestamps (so they are as close together as possible). Since the delta is then calculated, I'm not sure what disabling irqs for calling adjtimex gets you.

Changes since v1:
- do do_settimeofday64() when ICTIMESYNCFLAG_SYNC flag is present in the request (Alex Ng)
- add pr_debug() for the case when do_adjtimex() fails (Alex Ng)

Original description:

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP client is run in parallel things may go south, e.g. when an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY the Hyper-V module will not see this changes and time will oscillate and never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

With this series I suggest to use do_adjtimex() to adjust time. My tests show that such method gives equally good time convergence but avoids all the drawbacks described above.

Quoted 20 lines · Vitaly Kuznetsov

Changes since v1:
- do do_settimeofday64() when ICTIMESYNCFLAG_SYNC flag is present in the request (Alex Ng)
- add pr_debug() for the case when do_adjtimex() fails (Alex Ng)

Original description:

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP client is run in parallel things may go south, e.g. when an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY the Hyper-V module will not see this changes and time will oscillate and never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Quoted 3 lines

With this series I suggest to use do_adjtimex() to adjust time. My tests show that such method gives equally good time convergence but avoids all the drawbacks described above.

To be honest, I think all of this is just tinkering.

1) do_adjtimex() is assuming that there is a single client connected which is responsible for the updates. So I seriously doubt that a NTP client running in the guest will cooperate nicely with that timesync magic under all circumstances.

2) There is still the possibility to force do_settimeofday() calls which will upset NTP clients and have other side effects.

Why is this call necessary at all? Just because it's in some spec?

3) What happens if you have a PTP capable network card mapped into your guest and the guest uses PTP for time synchronization? The outcome is
predictible: CRAP.

I can see the value for a host wide time synchronization, but please use mechanisms which do not interfere with the rest of the time eco system in Linux.

The timesync thing happens periodically every 5 seconds, which you can feed nicely into the PPS subsystem and then the guest side NTP daemon can utilize it (or not).

Quoted 4 lines · Thomas Gleixner

1) do_adjtimex() is assuming that there is a single client connected which is responsible for the updates. So I seriously doubt that a NTP client running in the guest will cooperate nicely with that timesync magic under all circumstances.

True, as Stephen suggested we'll probably need a way to inform (or block) the second NTP client about the ongoing timesync.

Quoted 4 lines

2) There is still the possibility to force do_settimeofday() calls which will upset NTP clients and have other side effects.

Why is this call necessary at all? Just because it's in some spec?

ICTIMESYNCFLAG_SYNC flag, demanding us to do so, is only set on the first packet (when our VM boots) and after suspend/resume/migration events. In these cases guest's time can be off by minutes/hours and do_settimeofday() is probably justified.

Quoted 5 lines

3) What happens if you have a PTP capable network card mapped into your guest and the guest uses PTP for time synchronization? The outcome is
predictible: CRAP.

Quoted 8 lines

I can see the value for a host wide time synchronization, but please use mechanisms which do not interfere with the rest of the time eco system in Linux.

The timesync thing happens periodically every 5 seconds, which you can feed nicely into the PPS subsystem and then the guest side NTP daemon can utilize it (or not).

My understanding is that we have no guarantees from the host that these messages are sent every 5 seconds and even when they are the interval is not very precise. We can probably create a 'fake' pps signal out of these messages (e.g. these messages will just be adjusting the frequency of the signal. I can play with such approach if you think this is the way to go.

Instead of doing in-kernel time adjustments offload the work to an NTP client by exposing TimeSync messages as a PTP device. Users my now decide what they want to use as a source.

I tested the solution with chrony, the config was:

refclock PHC /dev/ptp0 poll 3 precision 1e-9

The result I'm seeing is accurate enough, the time delta between the guest and the host is almost always within [-10us, +10us], the in-kernel solution was giving us comparable results.

drivers/hv/hv_util.c +13 -0

+static int hv_ptp_enable(struct ptp_clock_info *info,
+			 struct ptp_clock_request *request, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static int hv_ptp_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	u64 newtime;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host_ts.lock, flags);
+	newtime = host_ts.host_time + get_timeadj_latency(host_ts.ref_time);

drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 517

Quoted 4 lines

drivers/hv/hv_util.c +4 -0

+struct ptp_clock_info ptp_hyperv_info = {
+	.name		= "hyperv",
+	.enable         = hv_ptp_enable,
+	.gettime64      = hv_ptp_gettime,

The code in drivers/ptp/ptp_clock.c calls

	.adjfreq (or adjfine)
	.adjtime
	.settime64

unconditionally, so you need to implement these returning EOPNOTSUPP. (See also Documentation/ptp/ptp.txt)

drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 528

Quoted 2 lines · Vitaly Kuznetsov

drivers/hv/hv_util.c +2 -0

+	hv_ptp_clock = ptp_clock_register(&ptp_hyperv_info, NULL);
+	if (IS_ERR(hv_ptp_clock)) {

Should that be IS_ERR_OR_NULL to catch "!IS_REACHABLE(CONFIG_PTP_1588_CLOCK)"?

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP server runs in parallel and uses something else for time sync (network, PTP,...) system time will never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Instead of doing in-kernel time adjustments offload the work to an NTP client by exposing TimeSync messages as a PTP device. Users my now decide what they want to use as a source.

I tested the solution with chrony, the config was:

refclock PHC /dev/ptp0 poll 3 precision 1e-9

The result I'm seeing is accurate enough, the time delta between the guest and the host is almost always within [-10us, +10us], the in-kernel solution was giving us comparable results.

I also tried implementing PPS device instead of PTP by using not currently used Hyper-V synthetic timers (we use only one of four for clockevent) but with PPS source only chrony wasn't able to give me the required accuracy, the delta often more that 100us.

drivers/hv/hv_util.c +5 -6

+static inline void adj_guesttime(u64 hosttime, u64 reftime, u8 adj_flags)
 {
+	unsigned long flags;
 
 	/*
 	 * This check is safe since we are executing in the
 	 * interrupt context and time synch messages arre always
 	 * delivered on the same CPU.
 	 */
-	if (work_pending(&wrk.work))
-		return;
+	if (adj_flags & ICTIMESYNCFLAG_SYNC) {
+		if (work_pending(&wrk.work))
+			return;
 
-	wrk.host_time = hosttime;
-	wrk.ref_time = reftime;
-	wrk.flags = flags;
-	if ((flags & (ICTIMESYNCFLAG_SYNC | ICTIMESYNCFLAG_SAMPLE)) != 0) {

Quoted 19 lines · Vitaly Kuznetsov

With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP server runs in parallel and uses something else for time sync (network, PTP,...) system time will never converge.
- Systemd starts annoying you by printing "Time has been changed" every 5 seconds to the system log.

Instead of doing in-kernel time adjustments offload the work to an NTP client by exposing TimeSync messages as a PTP device. Users my now decide what they want to use as a source.

I tested the solution with chrony, the config was:

refclock PHC /dev/ptp0 poll 3 precision 1e-9

Quoted 8 lines

The result I'm seeing is accurate enough, the time delta between the guest and the host is almost always within [-10us, +10us], the in-kernel solution was giving us comparable results.

I also tried implementing PPS device instead of PTP by using not currently used Hyper-V synthetic timers (we use only one of four for clockevent) but with PPS source only chrony wasn't able to give me the required accuracy, the delta often more that 100us.

Makes sense. The PTP based solution is really nice!

drivers/hv/hv_util.c struct adj_time_work { · line 206

Quoted 6 lines

static void hv_set_host_time(struct work_struct *work) {

drivers/hv/hv_util.c +0 -1

 	struct adj_time_work	*wrk;
-	s64 host_tns;
 	u64 newtime;
 	struct timespec64 host_ts;

Just a nitpick. Ordering variables in reverse fir tree (length) order:

struct adj_time_work *wrk;
struct timespec64 host_ts;
u64 newtime;

makes is simpler to parse

drivers/hv/hv_util.c static void hv_set_host_time(struct work_struct *work) · line 235

Quoted 6 lines

drivers/hv/hv_util.c +6 -0

+
+static struct {
+	u64 host_time;
+	u64 ref_time;
+	spinlock_t lock;
+} host_ts;

Another formatting nit. If you arrange the members in tabular fashion it becomes simpler to parse:

static struct {
	u64		host_time;
	u64		ref_time;
	spinlock_t 	lock;
} host_ts;

Also the struct might do with some comment explaning that it is the storage for the PTP machinery,

Quoted 26 lines

drivers/hv/hv_util.c +11 -6

+static inline void adj_guesttime(u64 hosttime, u64 reftime, u8 adj_flags)
 {
+	unsigned long flags;
 
 	/*
 	 * This check is safe since we are executing in the
 	 * interrupt context and time synch messages arre always
 	 * delivered on the same CPU.
 	 */
-	if (work_pending(&wrk.work))
-		return;
+	if (adj_flags & ICTIMESYNCFLAG_SYNC) {
+		if (work_pending(&wrk.work))
+			return;
 
-	wrk.host_time = hosttime;
-	wrk.ref_time = reftime;
-	wrk.flags = flags;
-	if ((flags & (ICTIMESYNCFLAG_SYNC | ICTIMESYNCFLAG_SAMPLE)) != 0) {
+		wrk.host_time = hosttime;
+		wrk.ref_time = reftime;
+		wrk.flags = adj_flags;
 		schedule_work(&wrk.work);
+	} else {
+		spin_lock_irqsave(&host_ts.lock, flags);
+		host_ts.host_time = hosttime;

drivers/hv/hv_util.c static void hv_set_host_time(struct work_struct *work) · line 259

Quoted 3 lines

drivers/hv/hv_util.c +3 -0

+
+		if (ts_srv_version <= TS_VERSION_3)
+			rdmsrl(HV_X64_MSR_TIME_REF_COUNT, host_ts.ref_time);

I'm confused here. The reftime / hosttime pair is accurate at sampling time on the host. So why reading the MSR here? I'm certainly missing something, but then this wants to have a comment like the other one in get_timeadj_latency().

drivers/hv/hv_util.c static void hv_set_host_time(struct work_struct *work) · line 264

Quoted 6 lines

drivers/hv/hv_util.c +3 -0

+		else
+			host_ts.ref_time = reftime;
+		spin_unlock_irqrestore(&host_ts.lock, flags);
 	}
 }

Other than that: Nice work!