[PATCH 0/4] hv_util: adjust system time smoothly

[PATCH 0/4] hv_util: adjust system time smoothly

[Vitaly Kuznetsov]

(First, I have to admit I'm not a timekeeping expert ...)

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.

Vitaly Kuznetsov (4):
  timekeeping: export do_adjtimex() to modules
  hv_util: switch to using timespec64
  hv_util: use do_adjtimex() to update system time
  hv_util: improve time adjustment accuracy by disabling interrupts

 drivers/hv/hv_util.c      | 33 +++++++++++++++++++++++++++++----
 kernel/time/timekeeping.c |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.9.3

[PATCH 1/4] timekeeping: export do_adjtimex() to modules

[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 <vkuznets@redhat.com>
---
 kernel/time/timekeeping.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index da233cd..ae4f24f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2312,6 +2312,7 @@ int do_adjtimex(struct timex *txc)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(do_adjtimex);
 
 #ifdef CONFIG_NTP_PPS
 /**
-- 
2.9.3

[PATCH 2/4] hv_util: switch to using timespec64

[Vitaly Kuznetsov]

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

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/hv_util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index e770774..94719eb 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -184,7 +184,7 @@ static void hv_set_host_time(struct work_struct *work)
 	struct adj_time_work	*wrk;
 	s64 host_tns;
 	u64 newtime;
-	struct timespec host_ts;
+	struct timespec64 host_ts;
 
 	wrk = container_of(work, struct adj_time_work, work);
 
@@ -203,9 +203,9 @@ static void hv_set_host_time(struct work_struct *work)
 		newtime += (current_tick - wrk->ref_time);
 	}
 	host_tns = (newtime - WLTIMEDELTA) * 100;
-	host_ts = ns_to_timespec(host_tns);
+	host_ts = ns_to_timespec64(host_tns);
 
-	do_settimeofday(&host_ts);
+	do_settimeofday64(&host_ts);
 }
 
 /*
-- 
2.9.3

[PATCH 3/4] hv_util: use do_adjtimex() to update system time

[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 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().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/hv_util.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 94719eb..4c0fbb0 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -182,9 +182,10 @@ 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};
 
 	wrk = container_of(work, struct adj_time_work, work);
 
@@ -205,7 +206,25 @@ static void hv_set_host_time(struct work_struct *work)
 	host_tns = (newtime - WLTIMEDELTA) * 100;
 	host_ts = ns_to_timespec64(host_tns);
 
-	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;
+
+	/* Try adjusting time by using phase adjustment if possible */
+	if (abs(delta) > MAXPHASE) {
+		do_settimeofday64(&host_ts);
+		return;
+	}
+
+	txc.modes = ADJ_TICK | ADJ_FREQUENCY | ADJ_OFFSET | ADJ_NANO |
+		ADJ_STATUS;
+	txc.tick = TICK_USEC;
+	txc.freq = 0;
+	txc.status = STA_PLL;
+	txc.offset = delta;
+	do_adjtimex(&txc);
 }
 
 /*
-- 
2.9.3

[PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts

[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 <vkuznets@redhat.com>
---
 drivers/hv/hv_util.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 4c0fbb0..233d5cb 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -186,6 +186,9 @@ static void hv_set_host_time(struct work_struct *work)
 	u64 newtime;
 	struct timespec64 host_ts, our_ts;
 	struct timex txc = {0};
+	unsigned long flags;
+
+	local_irq_save(flags);
 
 	wrk = container_of(work, struct adj_time_work, work);
 
@@ -214,6 +217,7 @@ static void hv_set_host_time(struct work_struct *work)
 
 	/* Try adjusting time by using phase adjustment if possible */
 	if (abs(delta) > MAXPHASE) {
+		local_irq_restore(flags);
 		do_settimeofday64(&host_ts);
 		return;
 	}
@@ -225,6 +229,8 @@ static void hv_set_host_time(struct work_struct *work)
 	txc.status = STA_PLL;
 	txc.offset = delta;
 	do_adjtimex(&txc);
+
+	local_irq_restore(flags);
 }
 
 /*
-- 
2.9.3

Re: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts

[Stephen Hemminger]

On Mon,  2 Jan 2017 20:41:14 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> 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 <vkuznets@redhat.com>
> ---
>  drivers/hv/hv_util.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 4c0fbb0..233d5cb 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -186,6 +186,9 @@ static void hv_set_host_time(struct work_struct *work)
>  	u64 newtime;
>  	struct timespec64 host_ts, our_ts;
>  	struct timex txc = {0};
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
>  
>  	wrk = container_of(work, struct adj_time_work, work);
>  
> @@ -214,6 +217,7 @@ static void hv_set_host_time(struct work_struct *work)
>  
>  	/* Try adjusting time by using phase adjustment if possible */
>  	if (abs(delta) > MAXPHASE) {
> +		local_irq_restore(flags);
>  		do_settimeofday64(&host_ts);
>  		return;
>  	}
> @@ -225,6 +229,8 @@ static void hv_set_host_time(struct work_struct *work)
>  	txc.status = STA_PLL;
>  	txc.offset = delta;
>  	do_adjtimex(&txc);
> +
> +	local_irq_restore(flags);
 

Yes, it should be atomic, but local irq save/restore is not sufficient protection
because it does not protect against premptible kernel. Why not a mutex? or a spinlock?

RE: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Alex Ng (LIS)]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, January 2, 2017 11:41 AM
> To: devel@linuxdriverproject.org
> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; John Stultz
> <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Alex Ng
> (LIS) <alexng@microsoft.com>
> Subject: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> 
> 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.

These are all good points. I am working on a patch to address point 2.
It will allow new TimeSync behavior to be disabled even if the TimeSync IC is
enabled from the host. This can be set to prevent TimeSync IC from interfering
with NTP client.

> 
> Instead of calling do_settimeofday64() we can pretend being an NTP client
> and use do_adjtimex().
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/hv_util.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> 94719eb..4c0fbb0 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -182,9 +182,10 @@ 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};
> 
>  	wrk = container_of(work, struct adj_time_work, work);
> 
> @@ -205,7 +206,25 @@ static void hv_set_host_time(struct work_struct
> *work)
>  	host_tns = (newtime - WLTIMEDELTA) * 100;
>  	host_ts = ns_to_timespec64(host_tns);
> 
> -	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;
> +
> +	/* Try adjusting time by using phase adjustment if possible */
> +	if (abs(delta) > MAXPHASE) {
> +		do_settimeofday64(&host_ts);
> +		return;
> +	}

We should also call do_settimeofday64() if the host sends flag
ICTIMESYNCFLAG_SYNC. This is a signal from host that the guest
shall sync with host time immediately (often when the guest has
just booted).

> +
> +	txc.modes = ADJ_TICK | ADJ_FREQUENCY | ADJ_OFFSET |
> ADJ_NANO |
> +		ADJ_STATUS;
> +	txc.tick = TICK_USEC;
> +	txc.freq = 0;

I'm not familiar with the ADJ_FREQUENCY flag. What does setting this to 'zero' achieve?
Are there any side-effects from doing this?

> +	txc.status = STA_PLL;
> +	txc.offset = delta;
> +	do_adjtimex(&txc);

Might be a good idea to handle the return code from do_adjtimex() and log something
in case of error.

>  }
> 
>  /*
> --
> 2.9.3

Re: [PATCH 4/4] hv_util: improve time adjustment accuracy by disabling interrupts

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Mon,  2 Jan 2017 20:41:14 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> 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 <vkuznets@redhat.com>
>> ---
>>  drivers/hv/hv_util.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
>> index 4c0fbb0..233d5cb 100644
>> --- a/drivers/hv/hv_util.c
>> +++ b/drivers/hv/hv_util.c
>> @@ -186,6 +186,9 @@ static void hv_set_host_time(struct work_struct *work)
>>  	u64 newtime;
>>  	struct timespec64 host_ts, our_ts;
>>  	struct timex txc = {0};
>> +	unsigned long flags;
>> +
>> +	local_irq_save(flags);
>>  
>>  	wrk = container_of(work, struct adj_time_work, work);
>>  
>> @@ -214,6 +217,7 @@ static void hv_set_host_time(struct work_struct *work)
>>  
>>  	/* Try adjusting time by using phase adjustment if possible */
>>  	if (abs(delta) > MAXPHASE) {
>> +		local_irq_restore(flags);
>>  		do_settimeofday64(&host_ts);
>>  		return;
>>  	}
>> @@ -225,6 +229,8 @@ static void hv_set_host_time(struct work_struct *work)
>>  	txc.status = STA_PLL;
>>  	txc.offset = delta;
>>  	do_adjtimex(&txc);
>> +
>> +	local_irq_restore(flags);
>
> Yes, it should be atomic, but local irq save/restore is not sufficient protection
> because it does not protect against premptible kernel. Why not a mutex? or a spinlock?

I may be missing something, but:

to make preemption happen we need to either get an interrupt or call
scheduling manually (directly or via preempt_enable(),
local_irq_restore(),...). Interrupts are disabled here and even if
something will trigger manual schedulling it won't happen as:

#define preemptible()   (preempt_count() == 0 && !irqs_disabled())

I don't see a good documentation but Documentation/preempt-locking.txt
says:

"PREVENTING PREEMPTION USING INTERRUPT DISABLING


It is possible to prevent a preemption event using local_irq_disable and
local_irq_save.  Note, when doing so, you must be very careful to not cause
an event that would set need_resched and result in a preemption check.  When
in doubt, rely on locking or explicit preemption disabling."

Spinlock with irqs disabled (spin_lock_irqsave()) would work too but
just because we're disabling interrupts. We don't need a lock here
because hv_set_host_time() is called from a workqueue and double
execution is impossible.

Mutex would not help at all as it is sleepable (so we may get a timer
interrupt).

The point I'm trying to make is: disabling interrupts is enough to
prevent other code from being executed on the same CPU in the middle of
hv_set_host_time(). The only exception I see is NMIs but we don't
usually get them and there is no easy way of protection.

-- 
  Vitaly

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Vitaly Kuznetsov]

"Alex Ng (LIS)" <alexng@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Monday, January 2, 2017 11:41 AM
>> To: devel@linuxdriverproject.org
>> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
>> Haiyang Zhang <haiyangz@microsoft.com>; John Stultz
>> <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Alex Ng
>> (LIS) <alexng@microsoft.com>
>> Subject: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
>> 
>> 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.
>
> These are all good points. I am working on a patch to address point 2.
> It will allow new TimeSync behavior to be disabled even if the TimeSync IC is
> enabled from the host. This can be set to prevent TimeSync IC from interfering
> with NTP client.
>

Good, this can happen in parallel to my series, right?

>> 
>> Instead of calling do_settimeofday64() we can pretend being an NTP client
>> and use do_adjtimex().
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/hv_util.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
>> 94719eb..4c0fbb0 100644
>> --- a/drivers/hv/hv_util.c
>> +++ b/drivers/hv/hv_util.c
>> @@ -182,9 +182,10 @@ 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};
>> 
>>  	wrk = container_of(work, struct adj_time_work, work);
>> 
>> @@ -205,7 +206,25 @@ static void hv_set_host_time(struct work_struct
>> *work)
>>  	host_tns = (newtime - WLTIMEDELTA) * 100;
>>  	host_ts = ns_to_timespec64(host_tns);
>> 
>> -	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;
>> +
>> +	/* Try adjusting time by using phase adjustment if possible */
>> +	if (abs(delta) > MAXPHASE) {
>> +		do_settimeofday64(&host_ts);
>> +		return;
>> +	}
>
> We should also call do_settimeofday64() if the host sends flag
> ICTIMESYNCFLAG_SYNC. This is a signal from host that the guest
> shall sync with host time immediately (often when the guest has
> just booted).

Ok, point taken, will do in v2. We don't get ICTIMESYNCFLAG_SYNC very
often, right?

>
>> +
>> +	txc.modes = ADJ_TICK | ADJ_FREQUENCY | ADJ_OFFSET |
>> ADJ_NANO |
>> +		ADJ_STATUS;
>> +	txc.tick = TICK_USEC;
>> +	txc.freq = 0;
>
> I'm not familiar with the ADJ_FREQUENCY flag. What does setting this to 'zero' achieve?
> Are there any side-effects from doing this?

Zero means no frequency adjustment required (we reset it in case it was
previously made by an NTP client).

>
>> +	txc.status = STA_PLL;
>> +	txc.offset = delta;
>> +	do_adjtimex(&txc);
>
> Might be a good idea to handle the return code from do_adjtimex() and log something
> in case of error.

I can add a debug message here but as this is a regular action we don't
want to get a flood of messages in case this fails permanently. I'd
avoid printing info messages here.

>
>>  }
>> 
>>  /*
>> --
>> 2.9.3

-- 
  Vitaly

RE: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Alex Ng (LIS)]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 3, 2017 4:32 AM
> To: Alex Ng (LIS) <alexng@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY
> Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> John Stultz <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>
> Subject: Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> 
> "Alex Ng (LIS)" <alexng@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Monday, January 2, 2017 11:41 AM
> >> To: devel@linuxdriverproject.org
> >> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> >> Haiyang Zhang <haiyangz@microsoft.com>; John Stultz
> >> <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Alex
> >> Ng
> >> (LIS) <alexng@microsoft.com>
> >> Subject: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> >>
> >> 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.
> >
> > These are all good points. I am working on a patch to address point 2.
> > It will allow new TimeSync behavior to be disabled even if the
> > TimeSync IC is enabled from the host. This can be set to prevent
> > TimeSync IC from interfering with NTP client.
> >
> 
> Good, this can happen in parallel to my series, right?

Yes, that is correct.

> 
> >>
> >> Instead of calling do_settimeofday64() we can pretend being an NTP
> >> client and use do_adjtimex().
> >>
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  drivers/hv/hv_util.c | 25 ++++++++++++++++++++++---
> >>  1 file changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> >> 94719eb..4c0fbb0 100644
> >> --- a/drivers/hv/hv_util.c
> >> +++ b/drivers/hv/hv_util.c
> >> @@ -182,9 +182,10 @@ 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};
> >>
> >>  	wrk = container_of(work, struct adj_time_work, work);
> >>
> >> @@ -205,7 +206,25 @@ static void hv_set_host_time(struct work_struct
> >> *work)
> >>  	host_tns = (newtime - WLTIMEDELTA) * 100;
> >>  	host_ts = ns_to_timespec64(host_tns);
> >>
> >> -	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;
> >> +
> >> +	/* Try adjusting time by using phase adjustment if possible */
> >> +	if (abs(delta) > MAXPHASE) {
> >> +		do_settimeofday64(&host_ts);
> >> +		return;
> >> +	}
> >
> > We should also call do_settimeofday64() if the host sends flag
> > ICTIMESYNCFLAG_SYNC. This is a signal from host that the guest shall
> > sync with host time immediately (often when the guest has just
> > booted).
> 
> Ok, point taken, will do in v2. We don't get ICTIMESYNCFLAG_SYNC very
> often, right?

This is correct. SYNC flags are sent rarely and usually only after a guest has
been resumed from a pause.

> 
> >
> >> +
> >> +	txc.modes = ADJ_TICK | ADJ_FREQUENCY | ADJ_OFFSET |
> >> ADJ_NANO |
> >> +		ADJ_STATUS;
> >> +	txc.tick = TICK_USEC;
> >> +	txc.freq = 0;
> >
> > I'm not familiar with the ADJ_FREQUENCY flag. What does setting this to
> 'zero' achieve?
> > Are there any side-effects from doing this?
> 
> Zero means no frequency adjustment required (we reset it in case it was
> previously made by an NTP client).
> 
> >
> >> +	txc.status = STA_PLL;
> >> +	txc.offset = delta;
> >> +	do_adjtimex(&txc);
> >
> > Might be a good idea to handle the return code from do_adjtimex() and
> > log something in case of error.
> 
> I can add a debug message here but as this is a regular action we don't want
> to get a flood of messages in case this fails permanently. I'd avoid printing
> info messages here.
> 

Agree. A debug level message is reasonable.

> >
> >>  }
> >>
> >>  /*
> >> --
> >> 2.9.3
> 
> --
>   Vitaly

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Stephen Hemminger]

On Tue, 3 Jan 2017 19:48:29 +0000
"Alex Ng (LIS)" <alexng@microsoft.com> wrote:

> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Tuesday, January 3, 2017 4:32 AM
> > To: Alex Ng (LIS) <alexng@microsoft.com>
> > Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY
> > Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> > John Stultz <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>
> > Subject: Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> > 
> > "Alex Ng (LIS)" <alexng@microsoft.com> writes:
> >   
> > >> -----Original Message-----
> > >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > >> Sent: Monday, January 2, 2017 11:41 AM
> > >> To: devel@linuxdriverproject.org
> > >> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> > >> Haiyang Zhang <haiyangz@microsoft.com>; John Stultz
> > >> <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Alex
> > >> Ng
> > >> (LIS) <alexng@microsoft.com>
> > >> Subject: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> > >>
> > >> 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.  
> > >
> > > These are all good points. I am working on a patch to address point 2.
> > > It will allow new TimeSync behavior to be disabled even if the
> > > TimeSync IC is enabled from the host. This can be set to prevent
> > > TimeSync IC from interfering with NTP client.
> > >  
> > 
> > Good, this can happen in parallel to my series, right?  
> 
> Yes, that is correct.
> 
> >   
> > >>
> > >> Instead of calling do_settimeofday64() we can pretend being an NTP
> > >> client and use do_adjtimex().
> > >>
> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

An alternative would be for hyper-v util to provide a clocksource device and
let NTP manage the adjustment. The advantage of this would be HV util not fighting
with NTP, and using standard API's. The downside would be the complexity of configuring
NTP, and difficulty of writing a clock source pseudo device.

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> An alternative would be for hyper-v util to provide a clocksource device and
> let NTP manage the adjustment. The advantage of this would be HV util not fighting
> with NTP, and using standard API's. The downside would be the complexity of configuring
> NTP, and difficulty of writing a clock source pseudo device.

Yes, I see this option. But as I wrote to John I'm afraid we'll have to
come up with a custom interface from hv_util to userspace which no NTP
server will want to support (because, first of all, it's not about
'network time' any more). We can write our own daemon which will read
from this interface and do adjtimex but in this case I don't see much
value in this data traveling from kernel to userspace and back...

-- 
  Vitaly

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Stephen Hemminger]

On Mon, 09 Jan 2017 18:40:15 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > An alternative would be for hyper-v util to provide a clocksource device and
> > let NTP manage the adjustment. The advantage of this would be HV util not fighting
> > with NTP, and using standard API's. The downside would be the complexity of configuring
> > NTP, and difficulty of writing a clock source pseudo device.  
> 
> Yes, I see this option. But as I wrote to John I'm afraid we'll have to
> come up with a custom interface from hv_util to userspace which no NTP
> server will want to support (because, first of all, it's not about
> 'network time' any more). We can write our own daemon which will read
> from this interface and do adjtimex but in this case I don't see much
> value in this data traveling from kernel to userspace and back...
> 

Master NTP servers are connected to authoritative clock sources. I have no idea how
that is configured, but it should be possible.

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Mon, 09 Jan 2017 18:40:15 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > An alternative would be for hyper-v util to provide a clocksource device and
>> > let NTP manage the adjustment. The advantage of this would be HV util not fighting
>> > with NTP, and using standard API's. The downside would be the complexity of configuring
>> > NTP, and difficulty of writing a clock source pseudo device.  
>> 
>> Yes, I see this option. But as I wrote to John I'm afraid we'll have to
>> come up with a custom interface from hv_util to userspace which no NTP
>> server will want to support (because, first of all, it's not about
>> 'network time' any more). We can write our own daemon which will read
>> from this interface and do adjtimex but in this case I don't see much
>> value in this data traveling from kernel to userspace and back...
>> 
>
> Master NTP servers are connected to authoritative clock sources. I have no idea how
> that is configured, but it should be possible.

As far as I understand these servers are connected to GPS receivers or
something like that and we can probably pretend being one but I'm not
sure that the TimeSync v4 protocol fits there, we'll probably lose the
precision - currently we calculate the delta in a small kernel function
with interrupts disabled, in my tests I see it floating around several
hundred - few thousand nanoseconds from host's time.

-- 
  Vitaly

Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Stephen Hemminger]

On Mon, 09 Jan 2017 19:14:30 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Mon, 09 Jan 2017 18:40:15 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Stephen Hemminger <stephen@networkplumber.org> writes:
> >>   
> >> > An alternative would be for hyper-v util to provide a clocksource device and
> >> > let NTP manage the adjustment. The advantage of this would be HV util not fighting
> >> > with NTP, and using standard API's. The downside would be the complexity of configuring
> >> > NTP, and difficulty of writing a clock source pseudo device.    
> >> 
> >> Yes, I see this option. But as I wrote to John I'm afraid we'll have to
> >> come up with a custom interface from hv_util to userspace which no NTP
> >> server will want to support (because, first of all, it's not about
> >> 'network time' any more). We can write our own daemon which will read
> >> from this interface and do adjtimex but in this case I don't see much
> >> value in this data traveling from kernel to userspace and back...
> >>   
> >
> > Master NTP servers are connected to authoritative clock sources. I have no idea how
> > that is configured, but it should be possible.  
> 
> As far as I understand these servers are connected to GPS receivers or
> something like that and we can probably pretend being one but I'm not
> sure that the TimeSync v4 protocol fits there, we'll probably lose the
> precision - currently we calculate the delta in a small kernel function
> with interrupts disabled, in my tests I see it floating around several
> hundred - few thousand nanoseconds from host's time.

My understanding is that NTP doesn't work very well at small time intervals.
Probably the ideal solution is something where kernel corrects time but
there is also way to communicate to NTP server that the kernel time is being maintained by
other entitity.