hv_util: adjust system time smoothly
v3 cover hv_util: adjust system time smoothly
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 may now decide what they want to use as a source.
Changes since v2:
- Implement Hyper-V PTP device instead of doint in-kernel time sync.
Changes since "[PATCH RFC] hv_utils: implement Hyper-V PTP source":
- Richard Cochran: implement .adjfreq, .adjtime, .settime64 returning -EOPNOTSUPP.
- Olaf Hering: change IS_ERR->IS_ERR_OR_NULL as CONFIG_PTP_1588_CLOCK can be disabled.
- Thomas Gleixner: formatting fixes, comments added.
Vitaly Kuznetsov (2):
hv_util: switch to using timespec64
hv_utils: implement Hyper-V PTP source
drivers/hv/hv_util.c | 142 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 116 insertions(+), 26 deletions(-)
Signature · 1 line
2.9.3
PATCH v3 1/2 hv_util: switch to using timespec64
do_settimeofday() is deprecated, use do_settimeofday64() instead.
--- drivers/hv/hv_util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hv/hv_util.c +3 -3
@@ -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); }/*
Signature · 1 line
2.9.3
Re: [PATCH v3 1/2] hv_util: switch to using timespec64
Quoted 4 lines · Vitaly Kuznetsov
do_settimeofday()is deprecated, usedo_settimeofday64()instead.Signed-off-by Vitaly Kuznetsov <[email protected]>Acked-by John Stultz <[email protected]>
PATCH v3 2/2 hv_utils: implement Hyper-V PTP source
In reply to Vitaly KuznetsovWith 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 may now decide what they want to use as a source.
I tested the solution with chrony, the config was:
refclock PHC /dev/ptp0 poll 3 precision 1e-9
The result I'm seeing is accurate enough, the time delta between the guest and the host is almost always within [-10us, +10us], the in-kernel solution was giving us comparable results.
I also tried implementing PPS device instead of PTP by using not currently used Hyper-V synthetic timers (we use only one of four for clockevent) but with PPS source only chrony wasn't able to give me the required accuracy, the delta often more that 100us.
--- drivers/hv/hv_util.c | 140 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 115 insertions(+), 25 deletions(-)
drivers/hv/hv_util.c +115 -25
@@ -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; +u64 newtime; 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,52 @@ 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) + +/* +* The last time sample, received from the host. PTP device responds to +* requests by using this data and the current partition-wide time reference +* count. +*/ +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; + +/* +* Prior to version 4 TimeSync messages from the host don't +* contain any reference time (the time when the time sample +* was generated), save the current time reference count +* instead. This adds a small delta between the time sample +* generation and the reception of the sample here to the result +* but it's the best thing we can do. +*/ +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 +505,69 @@ 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_settime (struct ptp_clock_info *p,const struct timespec64 *ts) +{ +return -EOPNOTSUPP ; +} + +static int hv_ptp_adjfreq (struct ptp_clock_info *ptp,s32 delta) +{ +return -EOPNOTSUPP ; +} +static int hv_ptp_adjtime (struct ptp_clock_info *ptp,s64 delta) +{ +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, + .adjtime = hv_ptp_adjtime, + .adjfreq = hv_ptp_adjfreq, + .gettime64 = hv_ptp_gettime, + .settime64 = hv_ptp_settime, + .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_OR_NULL (hv_ptp_clock)) { +pr_err ("cannot register PTP clock: %ld\n" , +PTR_ERR (hv_ptp_clock)); + hv_ptp_clock =NULL ; + } +return 0 ; }static void hv_timesync_deinit (void ) { +if (hv_ptp_clock) +ptp_clock_unregister (hv_ptp_clock);cancel_work_sync (&wrk.work); }
Signature · 1 line
2.9.3
Re: [PATCH v3 2/2] hv_utils: implement Hyper-V PTP source
Quoted 23 lines · On Tue, 17 Jan 2017 16:27:19 +0100 Vitaly Kuznetsov <[email protected]> wrote:
With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP 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 may now decide what they want to use as a source.
I tested the solution with chrony, the config was:
refclock PHC
/dev/ptp0poll 3 precision 1e-9The 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.
Quoted 6 lines
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 <[email protected]>
Looks good. Minor style comments.
drivers/hv/hv_util.c struct adj_time_work { · line 183
Quoted 11 lines
--- drivers/hv/hv_util.c | 140 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 115 insertions(+), 25 deletions(-)drivers/hv/hv_util.c +1
-0+static inline u64 get_timeadj_latency (u64 ref_time)
inline not necessary on static functions. GCC inlines anyway
Quoted 5 lines
drivers/hv/hv_util.c +5
-0+{ +u64 current_tick; + +if (ts_srv_version <=TS_VERSION_3 ) +return 0 ;
drivers/hv/hv_util.c struct adj_time_work { · line 198
Quoted 10 lines
drivers/hv/hv_util.c +10
-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);
Personal preference is not to add blank line between comment and associated code.
...
drivers/hv/hv_util.c
Quoted 2 lines
drivers/hv/hv_util.c +2
-0+ +struct ptp_clock_info ptp_hyperv_info = {
This could be static?
Could it be const?
Quoted 9 lines
drivers/hv/hv_util.c +9
-0+ .name = "hyperv", + .enable = hv_ptp_enable, + .adjtime = hv_ptp_adjtime, + .adjfreq = hv_ptp_adjfreq, + .gettime64 = hv_ptp_gettime, + .settime64 = hv_ptp_settime, + .owner = THIS_MODULE, +}; +
drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 560
Quoted 10 lines
drivers/hv/hv_util.c +7
-0+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_OR_NULL (hv_ptp_clock)) { +pr_err ("cannot register PTP clock: %ld\n" , +PTR_ERR (hv_ptp_clock));
Why not return error to init routine in case of failure.
drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 561
Quoted 1 line
drivers/hv/hv_util.c +1
-0+ hv_ptp_clock =NULL ;
Why not return error to init routine? Rather than having user scan log.
Quoted 4 lines
drivers/hv/hv_util.c +2
-0+ } +return 0 ; }
Re: [PATCH v3 2/2] hv_utils: implement Hyper-V PTP source
Quoted 39 lines · Stephen Hemminger
Quoted 23 lines · On Tue, 17 Jan 2017 16:27:19 +0100 Vitaly Kuznetsov <[email protected]> wrote:
With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP 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 may now decide what they want to use as a source.
I tested the solution with chrony, the config was:
refclock PHC
/dev/ptp0poll 3 precision 1e-9The 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.
Quoted 6 lines
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 <[email protected]>Looks good. Minor style comments.
Quoted 4 lines
--- drivers/hv/hv_util.c | 140 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 115 insertions(+), 25 deletions(-)
Quoted 9 lines
drivers/hv/hv_util.c struct adj_time_work { · line 183
Quoted 6 lines
drivers/hv/hv_util.c +1
-0+static inline u64 get_timeadj_latency (u64 ref_time)inline not necessary on static functions. GCC inlines anyway
Even when we have multiple call sites? Interesting...
Quoted 19 lines
Quoted 5 lines
drivers/hv/hv_util.c +5
-0+{ +u64 current_tick; + +if (ts_srv_version <=TS_VERSION_3 ) +return 0 ;drivers/hv/hv_util.c struct adj_time_work { · line 198
Quoted 10 lines
drivers/hv/hv_util.c +10
-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);Personal preference is not to add blank line between comment and associated code.
Quoted 2 lines
...
Ok.
Quoted 6 lines
drivers/hv/hv_util.c
Quoted 2 lines
drivers/hv/hv_util.c +2
-0+ +struct ptp_clock_info ptp_hyperv_info = {This could be static?
Could it be const?
Could be both I think.
Quoted 22 lines
Quoted 9 lines
drivers/hv/hv_util.c +9
-0+ .name = "hyperv", + .enable = hv_ptp_enable, + .adjtime = hv_ptp_adjtime, + .adjfreq = hv_ptp_adjfreq, + .gettime64 = hv_ptp_gettime, + .settime64 = hv_ptp_settime, + .owner = THIS_MODULE, +}; +drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 560
Quoted 10 lines
drivers/hv/hv_util.c +7
-0+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_OR_NULL (hv_ptp_clock)) { +pr_err ("cannot register PTP clock: %ld\n" , +PTR_ERR (hv_ptp_clock));Why not return error to init routine in case of failure.
Quoted 5 lines
drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 561
Quoted 1 line
drivers/hv/hv_util.c +1
-0+ hv_ptp_clock =NULL ;Why not return error to init routine? Rather than having user scan log.
The idea here was to not depend on CONFIG_PTP_1588_CLOCK. In case CONFIG_PTP_1588_CLOCK is disabled ptp_clock_register() will return NULL but the Hyper-V timesync driver remains functional - it still handles the ICTIMESYNCFLAG_SYNC case, just the ptp device will be missing. We can:
1) Put PTP-related code under #ifdef CONFIG_PTP_1588_CLOCK
2) Handle errors and NULL returned from ptp_clock_register() differently, fail init in case we get an error and continue in case we see NULL.
3) Leave things as they are.
4) Always require CONFIG_PTP_1588_CLOCK.
My personal preference would be 2 or 3. What do you think?
Quoted 4 lines
Quoted 4 lines
drivers/hv/hv_util.c +2
-0+ } +return 0 ; }
Signature · 1 line
Vitaly
Re: [PATCH v3 2/2] hv_utils: implement Hyper-V PTP source
Quoted 29 lines · Vitaly Kuznetsov
Quoted 11 lines · Stephen Hemminger
drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 560
Quoted 8 lines
static int
hv_timesync_init(struct hv_util_service *srv){drivers/hv/hv_util.c +5
-0INIT_WORK (&wrk.work, hv_set_host_time); + + hv_ptp_clock =ptp_clock_register (&ptp_hyperv_info,NULL ); +if (IS_ERR_OR_NULL (hv_ptp_clock)) { +pr_err ("cannot register PTP clock: %ld\n" , +PTR_ERR (hv_ptp_clock));Why not return error to init routine in case of failure.
Quoted 5 lines
drivers/hv/hv_util.c static struct hv_driver util_drv = { · line 561
Quoted 1 line
drivers/hv/hv_util.c +1
-0+ hv_ptp_clock =NULL ;Why not return error to init routine? Rather than having user scan log.
The idea here was to not depend on
CONFIG_PTP_1588_CLOCK. In caseCONFIG_PTP_1588_CLOCKis disabledptp_clock_register()will return NULL but the Hyper-V timesync driver remains functional - it still handles theICTIMESYNCFLAG_SYNCcase, just the ptp device will be missing. We can:
1) Put PTP-related code under #ifdefCONFIG_PTP_1588_CLOCK
2) Handle errors and NULL returned fromptp_clock_register()differently, fail init in case we get an error and continue in case we see NULL.
3) Leave things as they are.
4) Always requireCONFIG_PTP_1588_CLOCK.
Quoted 1 line
My personal preference would be 2 or 3. What do you think?
Keep the current implementation and add a comment explaining the logic of keeping the driver functional for the the ICTIMESYNC case.
Thanks,
tglx
Quoted 25 lines · On Tue, 17 Jan 2017 16:27:17 +0100 Vitaly Kuznetsov <[email protected]> wrote:
With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP 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 may now decide what they want to use as a source.
Changes since v2:
- Implement Hyper-V PTP device instead of doint in-kernel time sync.Changes since "[PATCH RFC]
hv_utils: implement Hyper-V PTP source":
- Richard Cochran: implement .adjfreq, .adjtime, .settime64 returning-EOPNOTSUPP.
- Olaf Hering: changeIS_ERR->IS_ERR_OR_NULLasCONFIG_PTP_1588_CLOCKcan be disabled.
- Thomas Gleixner: formatting fixes, comments added.
Quoted 7 lines
Vitaly Kuznetsov (2):
hv_util: switch to using timespec64
hv_utils: implement Hyper-V PTP sourcedrivers/hv/hv_util.c | 142 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 116 insertions(+), 26 deletions(-)
It would be good to update Documentation files to describe any configuration needed.
Quoted 32 lines · Stephen Hemminger
Quoted 29 lines · On Tue, 17 Jan 2017 16:27:17 +0100 Vitaly Kuznetsov <[email protected]> wrote:
With TimeSync version 4 protocol support we started updating system time continuously through the whole lifetime of Hyper-V guests. Every 5 seconds there is a time sample from the host which triggers do_settimeofday[64](). While the time from the host is very accurate such adjustments may cause issues:
- Time is jumping forward and backward, some applications may misbehave.
- In case an NTP 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 may now decide what they want to use as a source.
Changes since v2:
- Implement Hyper-V PTP device instead of doint in-kernel time sync.Changes since "[PATCH RFC]
hv_utils: implement Hyper-V PTP source":
- Richard Cochran: implement .adjfreq, .adjtime, .settime64 returning-EOPNOTSUPP.
- Olaf Hering: changeIS_ERR->IS_ERR_OR_NULLasCONFIG_PTP_1588_CLOCKcan be disabled.
- Thomas Gleixner: formatting fixes, comments added.Vitaly Kuznetsov (2):
hv_util: switch to using timespec64
hv_utils: implement Hyper-V PTP source
Quoted 5 lines
Quoted 3 lines
drivers/hv/hv_util.c | 142 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 116 insertions(+), 26 deletions(-)It would be good to update Documentation files to describe any configuration needed.
This is just a PTP device, not any different for other PTP devices so users will be reading their NTP server docs to figure out how to add a PTP reference clock.
Or do you have any particular idea where to put an example?
Signature · 1 line
Vitaly