In current hyper-v time sync service,it only gets the initial clock time
from the host. It didn't process the following time samples. This change
introduced a module parameter called host_time_sync. If it is set to true,
the guest will periodically sychronize it's time with the host clock using
host time sample. By default it is disabled, because we still recommend
user to configure NTP for time synchronization.
Signed-off-by: Thomas Shao <[email protected]>
Reviewed-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_util.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 106 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 3b9c9ef..1d8390c 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -51,11 +51,30 @@
#define HB_WS2008_MAJOR 1
#define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 | HB_MINOR)
+#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
+
+/*host sends time sample for every 5s.So the max polling interval
+ *is 128*5 = 640s.
+*/
+#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
+
static int sd_srv_version;
static int ts_srv_version;
static int hb_srv_version;
static int util_fw_version;
+/*host sends time sample for every 5s.So the initial polling interval
+ *is 5s.
+*/
+static s32 adj_interval = 1;
+
+/*The host_time_sync module parameter is used to control the time
+ sync between host and guest.
+*/
+static bool host_time_sync;
+module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
+MODULE_PARM_DESC(host_time_sync, "If the guest sync time with host");
+
static void shutdown_onchannelcallback(void *context);
static struct hv_util_service util_shutdown = {
.util_cb = shutdown_onchannelcallback,
@@ -163,15 +182,61 @@ static void shutdown_onchannelcallback(void *context)
/*
* Set guest time to host UTC time.
*/
-static inline void do_adj_guesttime(u64 hosttime)
+static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
{
- s64 host_tns;
- struct timespec host_ts;
+ s64 host_tns, guest_tns, diff;
+ struct timespec host_ts, guest_ts;
+ struct timex txc;
+ s64 tickchg;
+ int diff_sign;
host_tns = (hosttime - WLTIMEDELTA) * 100;
host_ts = ns_to_timespec(host_tns);
- do_settimeofday(&host_ts);
+ if (forceSync) {
+ do_settimeofday(&host_ts);
+ } else {
+ guest_ts = CURRENT_TIME;
+ guest_tns = timespec_to_ns(&guest_ts);
+ diff = host_tns - guest_tns;
+ if (diff >= 0) {
+ diff_sign = 1;
+ } else {
+ diff_sign = -1;
+ diff = -diff;
+ }
+
+ /*1s in nanosecond */
+ if (diff > 1000000000 || diff < -1000000000) {
+ do_settimeofday(&host_ts);
+ return;
+ }
+
+ /*1ms in nanosecond */
+ if (diff > 1000000 || diff < -1000000) {
+ /* get the current tick value */
+ txc.modes = 0;
+ do_adjtimex(&txc);
+
+ tickchg = diff * TICK_USEC /
+ (TIMESAMPLE_INTERVAL * adj_interval);
+
+ if (tickchg > TICK_USEC/10)
+ tickchg = TICK_USEC/10;
+
+ if (txc.tick == TICK_USEC + diff_sign * tickchg)
+ return;
+
+ txc.modes = ADJ_TICK;
+ txc.tick = TICK_USEC + diff_sign * tickchg;
+
+ do_adjtimex(&txc);
+ } else {
+ /* double the polling interval*/
+ if (adj_interval < TIME_ADJ_MAX_INTERVAL)
+ adj_interval = adj_interval * 2;
+ }
+ }
}
/*
@@ -179,8 +244,9 @@ static inline void do_adj_guesttime(u64 hosttime)
*/
struct adj_time_work {
- struct work_struct work;
+ struct work_struct work;
u64 host_time;
+ bool forceSync;
};
static void hv_set_host_time(struct work_struct *work)
@@ -188,7 +254,7 @@ static void hv_set_host_time(struct work_struct *work)
struct adj_time_work *wrk;
wrk = container_of(work, struct adj_time_work, work);
- do_adj_guesttime(wrk->host_time);
+ do_adj_guesttime(wrk->host_time, wrk->forceSync);
kfree(wrk);
}
@@ -202,11 +268,14 @@ static void hv_set_host_time(struct work_struct *work)
* thing is, systime is automatically set to emulated hardware clock which may
* not be UTC time or in the same time zone. So, to override these effects, we
* use the first 50 time samples for initial system time setting.
+ * If the host_time_sync module parameter is set, we will use the host time
+ * samples to adjust guest time after the first 50 samples.
*/
static inline void adj_guesttime(u64 hosttime, u8 flags)
{
struct adj_time_work *wrk;
static s32 scnt = 50;
+ static s32 sample_count;
wrk = kmalloc(sizeof(struct adj_time_work), GFP_ATOMIC);
if (wrk == NULL)
@@ -214,6 +283,7 @@ static inline void adj_guesttime(u64 hosttime, u8 flags)
wrk->host_time = hosttime;
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
+ wrk->forceSync = true;
INIT_WORK(&wrk->work, hv_set_host_time);
schedule_work(&wrk->work);
return;
@@ -221,10 +291,38 @@ static inline void adj_guesttime(u64 hosttime, u8 flags)
if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
+ wrk->forceSync = true;
INIT_WORK(&wrk->work, hv_set_host_time);
schedule_work(&wrk->work);
- } else
- kfree(wrk);
+ return;
+ }
+
+ if (host_time_sync) {
+ /*
+ * Use the Hyper-V time sample to adjust the guest time. The
+ * algorithm is: If the sample offsets exceeds 1 second, we
+ * directly set the clock to the server time. If the offset is
+ * less than 1ms, we ignore the time sample. Otherwise we adjust
+ * the clock.
+ */
+
+ if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0) {
+ if (sample_count < adj_interval) {
+ sample_count++;
+ goto cleanup;
+ }
+ /* reset the polling interval */
+ sample_count = 0;
+ wrk->forceSync = false;
+ INIT_WORK(&wrk->work, hv_set_host_time);
+ schedule_work(&wrk->work);
+ return;
+ }
+ }
+
+cleanup:
+ kfree(wrk);
+
}
/*
--
1.7.1
I had a couple small style nits.
On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 3b9c9ef..1d8390c 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -51,11 +51,30 @@
> #define HB_WS2008_MAJOR 1
> #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 | HB_MINOR)
>
> +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
If you wanted you could say:
#define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
> +
> +/*host sends time sample for every 5s.So the max polling interval
> + *is 128*5 = 640s.
> +*/
The comment still has problems throughout. Read
Documentation/CodingStyle.
> +#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
> +
> static int sd_srv_version;
> static int ts_srv_version;
> static int hb_srv_version;
> static int util_fw_version;
>
> +/*host sends time sample for every 5s.So the initial polling interval
> + *is 5s.
> +*/
> +static s32 adj_interval = 1;
Prefer mundane types instead there is a reason. This should be int
because it's not specified in a hardware spec. I know you are being
consistent with the surrounding code, but the surrounding code is bad so
don't emulate it.
> +
> +/*The host_time_sync module parameter is used to control the time
> + sync between host and guest.
> +*/
> +static bool host_time_sync;
> +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
> +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with host");
Maybe say: "Synchronize time with the host"?
> +
> static void shutdown_onchannelcallback(void *context);
> static struct hv_util_service util_shutdown = {
> .util_cb = shutdown_onchannelcallback,
> @@ -163,15 +182,61 @@ static void shutdown_onchannelcallback(void *context)
> /*
> * Set guest time to host UTC time.
> */
> -static inline void do_adj_guesttime(u64 hosttime)
> +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
I'm surprise checkpatch.pl does't complain about this CamelCase.
regards,
dan carpenter
On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> In current hyper-v time sync service,it only gets the initial clock time
> from the host. It didn't process the following time samples. This change
> introduced a module parameter called host_time_sync. If it is set to true,
> the guest will periodically sychronize it's time with the host clock using
> host time sample. By default it is disabled, because we still recommend
> user to configure NTP for time synchronization.
I really don't see the need for this. We have NTP. If the guests want
to, they may use it. Otherwise, they have a free running clock, just
like real machines.
> + /*
> + * Use the Hyper-V time sample to adjust the guest time. The
> + * algorithm is: If the sample offsets exceeds 1 second, we
> + * directly set the clock to the server time. If the offset is
So the guests will experience random time jumps in the kernel, without
any rhyme or reason?
> + * less than 1ms, we ignore the time sample. Otherwise we adjust
> + * the clock.
> + */
So when using this kernel module, the sychronization is never expected
to be better than one millisecond. That is not too good. I expect NTP
can do better. So what was the point of this change again?
Thanks,
Richard
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Tuesday, October 14, 2014 7:19 PM
> To: Thomas Shao
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> I had a couple small style nits.
>
> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> > 3b9c9ef..1d8390c 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -51,11 +51,30 @@
> > #define HB_WS2008_MAJOR 1
> > #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 |
> HB_MINOR)
> >
> > +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
>
> If you wanted you could say:
>
> #define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
>
> > +
> > +/*host sends time sample for every 5s.So the max polling interval
> > +*is 128*5 = 640s.
> > +*/
>
> The comment still has problems throughout. Read
> Documentation/CodingStyle.
>
I will correct the style of the comment.
> > +#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
> > +
> > static int sd_srv_version;
> > static int ts_srv_version;
> > static int hb_srv_version;
> > static int util_fw_version;
> >
> > +/*host sends time sample for every 5s.So the initial polling interval
> > +*is 5s.
> > +*/
> > +static s32 adj_interval = 1;
>
> Prefer mundane types instead there is a reason. This should be int because
> it's not specified in a hardware spec. I know you are being consistent with
> the surrounding code, but the surrounding code is bad so don't emulate it.
>
I agree with you. Maybe it's a good idea to correct the surrounding code in
another patch.
> > +
> > +/*The host_time_sync module parameter is used to control the time
> > + sync between host and guest.
> > +*/
> > +static bool host_time_sync;
> > +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
> > +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with
> host");
>
> Maybe say: "Synchronize time with the host"?
Sounds good.
>
> > +
> > static void shutdown_onchannelcallback(void *context); static struct
> > hv_util_service util_shutdown = {
> > .util_cb = shutdown_onchannelcallback, @@ -163,15 +182,61 @@
> static
> > void shutdown_onchannelcallback(void *context)
> > /*
> > * Set guest time to host UTC time.
> > */
> > -static inline void do_adj_guesttime(u64 hosttime)
> > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
>
> I'm surprise checkpatch.pl does't complain about this CamelCase.
I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to forcesync.
>
> regards,
> dan carpenter
> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Richard Cochran
> Sent: Tuesday, October 14, 2014 7:54 PM
> To: Thomas Shao
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> > In current hyper-v time sync service,it only gets the initial clock
> > time from the host. It didn't process the following time samples. This
> > change introduced a module parameter called host_time_sync. If it is
> > set to true, the guest will periodically sychronize it's time with the
> > host clock using host time sample. By default it is disabled, because
> > we still recommend user to configure NTP for time synchronization.
>
> I really don't see the need for this. We have NTP. If the guests want to, they
> may use it. Otherwise, they have a free running clock, just like real machines.
>
Sometimes the user can't setup NTP. For example the guest OS didn't have network
connection. And in some cases, they may want the guest time sync with host.
With the existing hyper-v time source, the system clock will has around 1.5 second
time drift per day. If the workload in the host is heavy, the number could be larger.
So this feature is really useful for some scenarios.
> > + /*
> > + * Use the Hyper-V time sample to adjust the guest time. The
> > + * algorithm is: If the sample offsets exceeds 1 second, we
> > + * directly set the clock to the server time. If the offset is
>
> So the guests will experience random time jumps in the kernel, without any
> rhyme or reason?
This behavior is designed for some extreme cases. Like manually setting guest time
to some value. Or the host resumes from a hibernate state. Normally, we should not
run into this.
>
> > + * less than 1ms, we ignore the time sample. Otherwise we
> adjust
> > + * the clock.
> > + */
>
> So when using this kernel module, the sychronization is never expected to
> be better than one millisecond. That is not too good. I expect NTP can do
> better. So what was the point of this change again?
The time sync component will try to slew the time if the time drift is larger than
1ms. It doesn't mean the time drift will always be larger than 1ms. This component
module is trying to keep the time sync with host. And NTP is to keep the time sync
with external time source from Internet. And yes, NTP has more complicated algorithm,
and it can do better.
>
> Thanks,
> Richard
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the
> body of a message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 14, 2014 at 12:50:23PM +0000, Thomas Shao wrote:
> > > -static inline void do_adj_guesttime(u64 hosttime)
> > > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
> >
> > I'm surprise checkpatch.pl does't complain about this CamelCase.
>
> I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to forcesync.
->force_sync is normal kernel style.
regards,
dan carpenter
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Tuesday, October 14, 2014 9:10 PM
> To: Thomas Shao
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> On Tue, Oct 14, 2014 at 12:50:23PM +0000, Thomas Shao wrote:
> > > > -static inline void do_adj_guesttime(u64 hosttime)
> > > > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
> > >
> > > I'm surprise checkpatch.pl does't complain about this CamelCase.
> >
> > I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to
> forcesync.
>
> ->force_sync is normal kernel style.
OK, got it. Thanks Dan!
>
> regards,
> dan carpenter
What is your expected value for TICK_USEC? I cant make the arithmetic work.
You double the check time if you are close but you never reduce the
check time if you are not.
Adjusting the tick count is a coarse adjustment of the clock. You
will end up chasing the host time but never stabilizing it.
Regarding the comment we have NTP for this I agree that would be
better than this implementation and I think Thomas agrees (as he said
NTP is the preferred option)
In order for this to be a good source of time for RTP and other time
sensitive stuff . you will have to have to re-implement parts of NTP
such as adjusting the clock frequency decreasing the check period when
error becomes too great etc. etc..
I still think there is a requirement for this if it is done more
comprehensively. For example depending on CPU loading some Hyperv
guests can give a drift of greater than 500ppm which NTP cant cope
with.
On Tue, Oct 14, 2014 at 1:50 PM, Thomas Shao <[email protected]> wrote:
>
>> -----Original Message-----
>> From: Dan Carpenter [mailto:[email protected]]
>> Sent: Tuesday, October 14, 2014 7:19 PM
>> To: Thomas Shao
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; KY Srinivasan
>> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
>> time sample
>>
>> I had a couple small style nits.
>>
>> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
>> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
>> > 3b9c9ef..1d8390c 100644
>> > --- a/drivers/hv/hv_util.c
>> > +++ b/drivers/hv/hv_util.c
>> > @@ -51,11 +51,30 @@
>> > #define HB_WS2008_MAJOR 1
>> > #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 |
>> HB_MINOR)
>> >
>> > +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
>>
>> If you wanted you could say:
>>
>> #define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
>>
>> > +
>> > +/*host sends time sample for every 5s.So the max polling interval
>> > +*is 128*5 = 640s.
>> > +*/
>>
>> The comment still has problems throughout. Read
>> Documentation/CodingStyle.
>>
>
> I will correct the style of the comment.
>
>> > +#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
>> > +
>> > static int sd_srv_version;
>> > static int ts_srv_version;
>> > static int hb_srv_version;
>> > static int util_fw_version;
>> >
>> > +/*host sends time sample for every 5s.So the initial polling interval
>> > +*is 5s.
>> > +*/
>> > +static s32 adj_interval = 1;
>>
>> Prefer mundane types instead there is a reason. This should be int because
>> it's not specified in a hardware spec. I know you are being consistent with
>> the surrounding code, but the surrounding code is bad so don't emulate it.
>>
> I agree with you. Maybe it's a good idea to correct the surrounding code in
> another patch.
>
>> > +
>> > +/*The host_time_sync module parameter is used to control the time
>> > + sync between host and guest.
>> > +*/
>> > +static bool host_time_sync;
>> > +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
>> > +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with
>> host");
>>
>> Maybe say: "Synchronize time with the host"?
>
> Sounds good.
>
>>
>> > +
>> > static void shutdown_onchannelcallback(void *context); static struct
>> > hv_util_service util_shutdown = {
>> > .util_cb = shutdown_onchannelcallback, @@ -163,15 +182,61 @@
>> static
>> > void shutdown_onchannelcallback(void *context)
>> > /*
>> > * Set guest time to host UTC time.
>> > */
>> > -static inline void do_adj_guesttime(u64 hosttime)
>> > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
>>
>> I'm surprise checkpatch.pl does't complain about this CamelCase.
>
> I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to forcesync.
>
>>
>> regards,
>> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 14, 2014 at 01:04:35PM +0000, Thomas Shao wrote:
> > > + /*
> > > + * Use the Hyper-V time sample to adjust the guest time. The
> > > + * algorithm is: If the sample offsets exceeds 1 second, we
> > > + * directly set the clock to the server time. If the offset is
> >
> > So the guests will experience random time jumps in the kernel, without any
> > rhyme or reason?
>
> This behavior is designed for some extreme cases. Like manually setting guest time
> to some value. Or the host resumes from a hibernate state. Normally, we should not
> run into this.
But when it *does* happen, the guest software will have no way of
knowing what happened. That stinks.
Taking your example, when the guest sets its time, the time will
suddenly jump somewhere else, and for no apparent reason.
>From the guest's point of view, this is really not acceptable.
Thanks,
Richard
On Tue, Oct 14, 2014 at 01:04:35PM +0000, Thomas Shao wrote:
> > I really don't see the need for this. We have NTP. If the guests want to, they
> > may use it. Otherwise, they have a free running clock, just like real machines.
> >
> Sometimes the user can't setup NTP. For example the guest OS didn't have network
> connection. And in some cases, they may want the guest time sync with host.
> With the existing hyper-v time source, the system clock will has around 1.5 second
> time drift per day. If the workload in the host is heavy, the number could be larger.
> So this feature is really useful for some scenarios.
Any real machine without networking (and without GPS etc) will
drift. That is just life, tough as it is. Why should we treat these
guests any differently than real machines?
Furthermore, without networking you really don't have a compelling
need for correct absolute time in the first place.
Thanks,
Richard
On Tue, 2014-10-14 at 14:19 +0300, Dan Carpenter wrote:
> I had a couple small style nits.
>
> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index 3b9c9ef..1d8390c 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -51,11 +51,30 @@
> > #define HB_WS2008_MAJOR 1
> > #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 | HB_MINOR)
> >
> > +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
>
> If you wanted you could say:
>
> #define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
ULL
> -----Original Message-----
> From: Mike Surcouf [mailto:[email protected]]
> Sent: Tuesday, October 14, 2014 9:17 PM
> To: Thomas Shao
> Cc: Dan Carpenter; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> What is your expected value for TICK_USEC? I cant make the arithmetic work.
The value for TICK_USEC is defined as ((1000000UL + USER_HZ/2) / USER_HZ).
In my box, it's 10000.
> You double the check time if you are close but you never reduce the check
> time if you are not.
> Adjusting the tick count is a coarse adjustment of the clock. You will end up
> chasing the host time but never stabilizing it.
>
The double polling schedule is just for the initial state. For example the VM just
get booted. So I didn't set the polling schedule back, once it is in stable state.
> Regarding the comment we have NTP for this I agree that would be better
> than this implementation and I think Thomas agrees (as he said NTP is the
> preferred option) In order for this to be a good source of time for RTP and
> other time sensitive stuff . you will have to have to re-implement parts of
> NTP such as adjusting the clock frequency decreasing the check period when
> error becomes too great etc. etc..
>
I don't think decreasing the check period will help a lot. And sometimes, if the check
period is too short, it might cause the time sync component to adjust time too frequently.
> I still think there is a requirement for this if it is done more comprehensively.
> For example depending on CPU loading some Hyperv guests can give a drift
> of greater than 500ppm which NTP cant cope with.
>
>
> On Tue, Oct 14, 2014 at 1:50 PM, Thomas Shao <[email protected]>
> wrote:
> >
> >> -----Original Message-----
> >> From: Dan Carpenter [mailto:[email protected]]
> >> Sent: Tuesday, October 14, 2014 7:19 PM
> >> To: Thomas Shao
> >> Cc: [email protected]; [email protected]; linux-
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; KY Srinivasan
> >> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using
> >> host time sample
> >>
> >> I had a couple small style nits.
> >>
> >> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> >> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> >> > 3b9c9ef..1d8390c 100644
> >> > --- a/drivers/hv/hv_util.c
> >> > +++ b/drivers/hv/hv_util.c
> >> > @@ -51,11 +51,30 @@
> >> > #define HB_WS2008_MAJOR 1
> >> > #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 |
> >> HB_MINOR)
> >> >
> >> > +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
> >>
> >> If you wanted you could say:
> >>
> >> #define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
> >>
> >> > +
> >> > +/*host sends time sample for every 5s.So the max polling interval
> >> > +*is 128*5 = 640s.
> >> > +*/
> >>
> >> The comment still has problems throughout. Read
> >> Documentation/CodingStyle.
> >>
> >
> > I will correct the style of the comment.
> >
> >> > +#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
> >> > +
> >> > static int sd_srv_version;
> >> > static int ts_srv_version;
> >> > static int hb_srv_version;
> >> > static int util_fw_version;
> >> >
> >> > +/*host sends time sample for every 5s.So the initial polling
> >> > +interval *is 5s.
> >> > +*/
> >> > +static s32 adj_interval = 1;
> >>
> >> Prefer mundane types instead there is a reason. This should be int
> >> because it's not specified in a hardware spec. I know you are being
> >> consistent with the surrounding code, but the surrounding code is bad so
> don't emulate it.
> >>
> > I agree with you. Maybe it's a good idea to correct the surrounding
> > code in another patch.
> >
> >> > +
> >> > +/*The host_time_sync module parameter is used to control the time
> >> > + sync between host and guest.
> >> > +*/
> >> > +static bool host_time_sync;
> >> > +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
> >> > +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with
> >> host");
> >>
> >> Maybe say: "Synchronize time with the host"?
> >
> > Sounds good.
> >
> >>
> >> > +
> >> > static void shutdown_onchannelcallback(void *context); static
> >> > struct hv_util_service util_shutdown = {
> >> > .util_cb = shutdown_onchannelcallback, @@ -163,15 +182,61 @@
> >> static
> >> > void shutdown_onchannelcallback(void *context)
> >> > /*
> >> > * Set guest time to host UTC time.
> >> > */
> >> > -static inline void do_adj_guesttime(u64 hosttime)
> >> > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
> >>
> >> I'm surprise checkpatch.pl does't complain about this CamelCase.
> >
> > I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to
> forcesync.
> >
> >>
> >> regards,
> >> dan carpenter
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-kernel" in the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Richard Cochran [mailto:[email protected]]
> Sent: Tuesday, October 14, 2014 9:20 PM
> To: Thomas Shao
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> On Tue, Oct 14, 2014 at 01:04:35PM +0000, Thomas Shao wrote:
> > > > + /*
> > > > + * Use the Hyper-V time sample to adjust the guest time. The
> > > > + * algorithm is: If the sample offsets exceeds 1 second, we
> > > > + * directly set the clock to the server time. If the offset is
> > >
> > > So the guests will experience random time jumps in the kernel,
> > > without any rhyme or reason?
> >
> > This behavior is designed for some extreme cases. Like manually
> > setting guest time to some value. Or the host resumes from a hibernate
> > state. Normally, we should not run into this.
>
> But when it *does* happen, the guest software will have no way of knowing
> what happened. That stinks.
>
> Taking your example, when the guest sets its time, the time will suddenly
> jump somewhere else, and for no apparent reason.
>
> From the guest's point of view, this is really not acceptable.
>
If the user chooses to sync guest time with host, that's the expected behavior, right?
> Thanks,
> Richard
> -----Original Message-----
> From: Richard Cochran [mailto:[email protected]]
> Sent: Tuesday, October 14, 2014 9:26 PM
> To: Thomas Shao
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> On Tue, Oct 14, 2014 at 01:04:35PM +0000, Thomas Shao wrote:
> > > I really don't see the need for this. We have NTP. If the guests
> > > want to, they may use it. Otherwise, they have a free running clock, just
> like real machines.
> > >
> > Sometimes the user can't setup NTP. For example the guest OS didn't
> > have network connection. And in some cases, they may want the guest
> time sync with host.
> > With the existing hyper-v time source, the system clock will has
> > around 1.5 second time drift per day. If the workload in the host is heavy,
> the number could be larger.
> > So this feature is really useful for some scenarios.
>
> Any real machine without networking (and without GPS etc) will drift. That is
> just life, tough as it is. Why should we treat these guests any differently than
> real machines?
>
> Furthermore, without networking you really don't have a compelling need
> for correct absolute time in the first place.
The host machine can be configure with NTP. And in this case, making guest time sync
with host is useful.
>
> Thanks,
> Richard
Even with networking I think there are other senarios where this would
be useful such as no access to an NTP server due to firewall rules or
no internal NTP or simply an admin without much knowledge of NTP.
HyperV host very likely has good time from AD and it would be good if
the Linux VM just synced its time from the host after a vanilla
install (just like windows VMs).
That would require no configuration and probably save a ton of support traffic.
However this patch requires a module parameter which really negates
the zero configuration argument.
Also please don't make this the default until the timesync component
is more comprehensive and provides a stable time of similar quality to
NTP.
VMware has put a lot of effort into host -> guest timesync so I think
there is a case for some form of host based time sync on HyperV.
On Tue, Oct 14, 2014 at 02:16:34PM +0100, Mike Surcouf wrote:
> What is your expected value for TICK_USEC? I cant make the arithmetic work.
> You double the check time if you are close but you never reduce the
> check time if you are not.
> Adjusting the tick count is a coarse adjustment of the clock. You
> will end up chasing the host time but never stabilizing it.
We should not be putting hardcoded servos into random drivers.
Instead, why not export the time offset to the guest as a series of
PPS samples, or the like? Then, a user space program in the guest can
decide whether it will use the information and how to filter the
signal.
> Regarding the comment we have NTP for this I agree that would be
> better than this implementation and I think Thomas agrees (as he said
> NTP is the preferred option)
> In order for this to be a good source of time for RTP and other time
> sensitive stuff . you will have to have to re-implement parts of NTP
> such as adjusting the clock frequency decreasing the check period when
> error becomes too great etc. etc..
No, lets not re-implement NTP. That would be a waste of effort.
Thanks,
Richard
Maybe John Stultz should also go onto CC.
Thanks,
Richard
On Tue, Oct 14, 2014 at 03:14:21PM +0100, Mike Surcouf wrote:
> Even with networking I think there are other senarios where this would
> be useful such as no access to an NTP server due to firewall rules or
> no internal NTP or simply an admin without much knowledge of NTP.
Perhaps, but ...
> HyperV host very likely has good time from AD and it would be good if
> the Linux VM just synced its time from the host after a vanilla
> install (just like windows VMs).
Just cause windows does it, doesn't make it a good idea.
> That would require no configuration and probably save a ton of support traffic.
> However this patch requires a module parameter which really negates
> the zero configuration argument.
> Also please don't make this the default until the timesync component
> is more comprehensive and provides a stable time of similar quality to
> NTP.
We really don't want to go there.
> VMware has put a lot of effort into host -> guest timesync so I think
> there is a case for some form of host based time sync on HyperV.
Again, that is fine for VMware, but it might not be the best way.
IMHO, you should let the guest steer its own clock. That gives the end
user the most flexibility. Just provide the offset information, and
let a dedicated service (like ntpd or linuxptp's phc2sys) do the rest.
Thanks,
Richard
Hi!
>> VMware has put a lot of effort into host -> guest timesync so I think
>> there is a case for some form of host based time sync on HyperV.
. . .
>
> IMHO, you should let the guest steer its own clock.
> That gives the end user the most flexibility.
No problem:
Time Sync can be turn off in VM settings
Best regards, Victor Miasnikov
Blog: http://vvm.blog.tut.by/
On Tue, Oct 14, 2014 at 04:33:46PM +0200, Richard Cochran wrote:
>
> IMHO, you should let the guest steer its own clock. That gives the end
> user the most flexibility. Just provide the offset information, and
> let a dedicated service (like ntpd or linuxptp's phc2sys) do the rest.
So if it really about the convenience of not having to run a service
on the guests, then why not expose the guest clock to the host as a
dynamic posix clock? Then you could use phc2sys to tune the guest
without writing even a line of servo code...
Thanks,
Richard
>The value for TICK_USEC is defined as ((1000000UL + USER_HZ/2) / USER_HZ).
> In my box, it's 10000.
OK got it thanks . Checked the algorithm OK by me.
Of course you can only adjust clock by a tick (smallest resolution) so
you could be flapping between 2 values so as you say NTP is preferred.
However it may be good enough for some scenarios.