Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1551022 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] hv_util: adjust system time smoothly Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-04 18:40 +0100
    [PATCH v2 4/4] hv_util: improve time adjustment accuracy by disabling interrupts Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-04 18:50 +0100
      Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by  disabling interrupts Stephen Hemminger <stephen@networkplumber.org> - 2017-01-04 20:20 +0100
        Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by disabling interrupts Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-05 13:40 +0100
          Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by  disabling interrupts Stephen Hemminger <stephen@networkplumber.org> - 2017-01-05 18:50 +0100
      Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by  disabling interrupts John Stultz <john.stultz@linaro.org> - 2017-01-07 02:10 +0100
        Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by disabling interrupts Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-09 14:10 +0100
    Re: [PATCH v2 0/4] hv_util: adjust system time smoothly Thomas Gleixner <tglx@linutronix.de> - 2017-01-09 22:30 +0100
      Re: [PATCH v2 0/4] hv_util: adjust system time smoothly Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-10 15:40 +0100
        Re: [PATCH v2 0/4] hv_util: adjust system time smoothly Thomas Gleixner <tglx@linutronix.de> - 2017-01-10 16:00 +0100
          [PATCH RFC] hv_utils: implement Hyper-V PTP source Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-13 14:10 +0100
            Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source Richard Cochran <richardcochran@gmail.com> - 2017-01-13 16:00 +0100
              Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-13 16:40 +0100
            Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source Olaf Hering <olaf@aepfle.de> - 2017-01-13 16:30 +0100
              Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-01-13 16:40 +0100

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

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

[toc] | [next] | [standalone]

#1551026 — [PATCH v2 4/4] hv_util: improve time adjustment accuracy by disabling interrupts

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

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1554729

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

[toc] | [prev] | [next] | [standalone]

#1555471

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

[toc] | [prev] | [next] | [standalone]

#1555522

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web