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

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

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

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      | 40 ++++++++++++++++++++++++++++++++++++----
 kernel/time/timekeeping.c |  1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.9.3

[PATCH v2 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

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> 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);

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.

thanks
-john

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

[Vitaly Kuznetsov]

John Stultz <john.stultz@linaro.org> writes:

> On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> 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);
>
> 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.
>

Thank you for your comments John,

An alternative approach would be to make this module export an interface
for userspace and implement a daemon which would do adjtimex
syscalls. In theory this could be a part of an NTP client though I don't
think that NTP clients will want to accept such 'alternative' way of
getting time.

-- 
  Vitaly

[PATCH v2 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

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> do_settimeofday() is deprecated, use do_settimeofday64() instead.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>


Looks sane.

Acked-by: John Stultz <john.stultz@linaro.org>

thanks
-john

[PATCH v2 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(). Do do_settimeofday64() in case the difference is too
big or ICTIMESYNCFLAG_SYNC flag was set in the request.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
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(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 94719eb..7e97231 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -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);
 
-	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;
+
+	/*
+	 * Do raw do_settimeofday64() in case delta is too big or we were
+	 * ordered to sync our time by the host.
+	 */
+	if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) {
+		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;
+
+	ret = do_adjtimex(&txc);
+	if (ret)
+		pr_debug("Failed to adjust system time: %d\n", ret);
 }
 
 /*
-- 
2.9.3

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

[Stephen Hemminger]

On Wed,  4 Jan 2017 18:24:38 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> 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 <vkuznets@redhat.com>
> ---
> 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(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 94719eb..7e97231 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -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);
>  
> -	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.

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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Wed,  4 Jan 2017 18:24:38 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> 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 <vkuznets@redhat.com>
>> ---
>> 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(-)
>> 
>> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
>> index 94719eb..7e97231 100644
>> --- a/drivers/hv/hv_util.c
>> +++ b/drivers/hv/hv_util.c
>> @@ -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);
>>  
>> -	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.

I see. While s64 should work ktime seems preferable. I'll give it a try.

-- 
  Vitaly

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov <vkuznets@redhat.com> 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.

So how does having the guest kernel (on behalf of the host) calling
adjtimex internally interact with NTP clients running on the guest?
The kernel sort of assumes a single user of adjtimex (having multiple
clients adjusting the clock doesn't work out so well).

thanks
-john

[PATCH v2 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 7e97231..4e50a42 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -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);
+
+	local_irq_restore(flags);
 }
 
 /*
-- 
2.9.3

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

[Stephen Hemminger]

On Wed,  4 Jan 2017 18:24:39 +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>

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?

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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Wed,  4 Jan 2017 18:24:39 +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>
>
> 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.
>

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?
Yes, it was written for syscalls but I don't see any other required
things in adjtimex syscall, it's just a straitforward copy_from_user()
-> do_adjtimex() -> copy_to_user() chain.

I would very much appreciate if Thomas or John could comment what's
better here long-term.

> 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?

KVM doesn't have an NTP-like service, it just provides kvm_clock (which,
BTW, implements VCLOCK_PVCLOCK for vDSO calls -- unlike Hyper-V's TSC
page). hv_utils is going to be the first in-kernel 'NTP'.

-- 
  Vitaly

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

[Stephen Hemminger]

On Thu, 05 Jan 2017 13:35:58 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> 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?

With out holding timekeeper_lock, I don't see how you can do the adjtime
atomically.  The userspace NTP doesn't worry about it, but in the kernel
you can be more accurate. But to do that you would need to write a new
function that is kernel specific.

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, 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 7e97231..4e50a42 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -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);
> +
> +       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.

thanks
-john

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

[Vitaly Kuznetsov]

John Stultz <john.stultz@linaro.org> writes:

> On Wed, Jan 4, 2017 at 9:24 AM, 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 7e97231..4e50a42 100644
>> --- a/drivers/hv/hv_util.c
>> +++ b/drivers/hv/hv_util.c
>> @@ -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);
>> +
>> +       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.
>

True,

interrupts disabling would only be beneficial for the
do_settimeofday64() case but we can't actually do it there. I'll shorten
the interrupts disabling to the delta calculation only.

-- 
  Vitaly

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

[Thomas Gleixner]

On Wed, 4 Jan 2017, Vitaly Kuznetsov wrote:

> 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.

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

Thanks,

	tglx

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

[Vitaly Kuznetsov]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Wed, 4 Jan 2017, Vitaly Kuznetsov wrote:
>
>> 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.
>
> To be honest, I think all of this is just tinkering.
>

Thank you for your comments, Thomas,

> 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.

>
> 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.

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

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.

-- 
  Vitaly

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

[Thomas Gleixner]

Vitaly,

On Tue, 10 Jan 2017, Vitaly Kuznetsov wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> > 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.

I don't see how that's supposed to work.

> > 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.

Usually boot/resume time is pretty accurate through RTC readout, but IIRC
then Windows still insists on RTC being set to local time instead of UTC,
which does not really work well on Linux.

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

The PPS stuff has some expectations about the interval, but it's a rather
solid piece of art and I think it's worth a try.

Thanks,

	tglx

[PATCH RFC] hv_utils: implement Hyper-V PTP source

[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

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.

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

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 94719eb..16fb874c3 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -27,6 +27,7 @@
 #include <linux/sysctl.h>
 #include <linux/reboot.h>
 #include <linux/hyperv.h>
+#include <linux/ptp_clock_kernel.h>
 
 #include "hyperv_vmbus.h"
 
@@ -179,31 +180,35 @@ struct adj_time_work {
 	u8	flags;
 };
 
+static inline u64 get_timeadj_latency(u64 ref_time)
+{
+	u64 current_tick;
+
+	if (ts_srv_version <= TS_VERSION_3)
+		return 0;
+
+	/*
+	 * Some latency has been introduced since Hyper-V generated
+	 * its time sample. Take that latency into account before
+	 * using TSC reference time sample from Hyper-V.
+	 *
+	 * This sample is given by TimeSync v4 and above hosts.
+	 */
+
+	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+	return current_tick - ref_time;
+}
+
 static void hv_set_host_time(struct work_struct *work)
 {
 	struct adj_time_work	*wrk;
-	s64 host_tns;
 	u64 newtime;
 	struct timespec64 host_ts;
 
 	wrk = container_of(work, struct adj_time_work, work);
 
-	newtime = wrk->host_time;
-	if (ts_srv_version > TS_VERSION_3) {
-		/*
-		 * Some latency has been introduced since Hyper-V generated
-		 * its time sample. Take that latency into account before
-		 * using TSC reference time sample from Hyper-V.
-		 *
-		 * This sample is given by TimeSync v4 and above hosts.
-		 */
-		u64 current_tick;
-
-		rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
-		newtime += (current_tick - wrk->ref_time);
-	}
-	host_tns = (newtime - WLTIMEDELTA) * 100;
-	host_ts = ns_to_timespec64(host_tns);
+	newtime = wrk->host_time + get_timeadj_latency(wrk->ref_time);
+	host_ts = ns_to_timespec64((newtime - WLTIMEDELTA) * 100);
 
 	do_settimeofday64(&host_ts);
 }
@@ -222,22 +227,39 @@ static void hv_set_host_time(struct work_struct *work)
  * to discipline the clock.
  */
 static struct adj_time_work  wrk;
-static inline void adj_guesttime(u64 hosttime, u64 reftime, u8 flags)
+
+static struct {
+	u64 host_time;
+	u64 ref_time;
+	spinlock_t lock;
+} host_ts;
+
+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;
+
+		if (ts_srv_version <= TS_VERSION_3)
+			rdmsrl(HV_X64_MSR_TIME_REF_COUNT, host_ts.ref_time);
+		else
+			host_ts.ref_time = reftime;
+		spin_unlock_irqrestore(&host_ts.lock, flags);
 	}
 }
 
@@ -470,14 +492,50 @@ static  struct hv_driver util_drv = {
 	.remove =  util_remove,
 };
 
+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);
+	*ts = ns_to_timespec64((newtime - WLTIMEDELTA) * 100);
+	spin_unlock_irqrestore(&host_ts.lock, flags);
+
+	return 0;
+}
+
+struct ptp_clock_info ptp_hyperv_info = {
+	.name		= "hyperv",
+	.enable         = hv_ptp_enable,
+	.gettime64      = hv_ptp_gettime,
+	.owner		= THIS_MODULE,
+};
+
+static struct ptp_clock *hv_ptp_clock;
+
 static int hv_timesync_init(struct hv_util_service *srv)
 {
 	INIT_WORK(&wrk.work, hv_set_host_time);
+
+	hv_ptp_clock = ptp_clock_register(&ptp_hyperv_info, NULL);
+	if (IS_ERR(hv_ptp_clock)) {
+		pr_err("cannot register PTP clock\n");
+		return PTR_ERR(hv_ptp_clock);
+	}
+
 	return 0;
 }
 
 static void hv_timesync_deinit(void)
 {
+	ptp_clock_unregister(hv_ptp_clock);
 	cancel_work_sync(&wrk.work);
 }
 
-- 
2.9.3

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Richard Cochran]

On Fri, Jan 13, 2017 at 02:05:43PM +0100, Vitaly Kuznetsov wrote:
> 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.

This approach is much nicer than the previous one.

> +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);
> +	*ts = ns_to_timespec64((newtime - WLTIMEDELTA) * 100);
> +	spin_unlock_irqrestore(&host_ts.lock, flags);
> +
> +	return 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)

> +	.owner		= THIS_MODULE,
> +};

Thanks,
Richard

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Vitaly Kuznetsov]

Richard Cochran <richardcochran@gmail.com> writes:

>> +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)
>

Sure, thanks, will do in non-RFC version.

-- 
  Vitaly

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

On Fri, Jan 13, Vitaly Kuznetsov wrote:

> +	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)"?

Olaf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Vitaly Kuznetsov]

Olaf Hering <olaf@aepfle.de> writes:

> On Fri, Jan 13, Vitaly Kuznetsov wrote:
>
>> +	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)"?
>

Oh, yes. I missed the case when CONFIG_PTP_1588_CLOCK is disabled
completely. I'll also remove the return below to not fail the device
completely. Even if there is no PTP support in kernel the
ICTIMESYNCFLAG_SYNC case which triggers do_settimeofday64() is still
probably useful.

-- 
  Vitaly

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Thomas Gleixner]

On Fri, 13 Jan 2017, Vitaly Kuznetsov 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 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.

Makes sense. The PTP based solution is really nice!

>  static void hv_set_host_time(struct work_struct *work)
>  {
>  	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

> +
> +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,

> +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;
> +
> +		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().

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

Other than that: Nice work!

Thanks,

	tglx

Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Vitaly Kuznetsov]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Fri, 13 Jan 2017, Vitaly Kuznetsov 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 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.
>
> Makes sense. The PTP based solution is really nice!
>
>>  static void hv_set_host_time(struct work_struct *work)
>>  {
>>  	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
>
>> +
>> +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,
>
>> +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;
>> +
>> +		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().

Old TimeSync (vesion < 4.0) protocol messages don't specify any
reference time so we're 'faking' it by saving the reference time when
the message was received on the guest. This, of course, reduces the
precision of the device as we have a delta between the time sample
generation and its reception on the guest but there is no way we can
calculate this delta. I'll put a comment.

>
>> +		else
>> +			host_ts.ref_time = reftime;
>> +		spin_unlock_irqrestore(&host_ts.lock, flags);
>>  	}
>>  }
>
> Other than that: Nice work!
>

Thanks, I'll incorporate all your feedback and post non-RFC version.

-- 
  Vitaly