Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Dan Carpenter 
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 50L  /* 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Thomas Shao 

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Tuesday, October 14, 2014 7:19 PM
 To: Thomas Shao
 Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
 ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
 a...@canonical.com; jasow...@redhat.com; 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_MAJOR1
   #define HB_WS2008_VERSION  (HB_WS2008_MAJOR  16 |
 HB_MINOR)
 
  +#define  TIMESAMPLE_INTERVAL 50L  /* 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Dan Carpenter 
On Tue, Oct 14, 2014 at 12:50:23PM +, 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Thomas Shao 

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Tuesday, October 14, 2014 9:10 PM
 To: Thomas Shao
 Cc: o...@aepfle.de; gre...@linuxfoundation.org; jasow...@redhat.com;
 linux-ker...@vger.kernel.org; a...@canonical.com;
 de...@linuxdriverproject.org; t...@linutronix.de
 Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample
 
 On Tue, Oct 14, 2014 at 12:50:23PM +, 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Mike Surcouf 
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 huis...@microsoft.com wrote:

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Tuesday, October 14, 2014 7:19 PM
 To: Thomas Shao
 Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
 ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
 a...@canonical.com; jasow...@redhat.com; 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_MAJOR1
   #define HB_WS2008_VERSION  (HB_WS2008_MAJOR  16 |
 HB_MINOR)
 
  +#define  TIMESAMPLE_INTERVAL 50L  /* 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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
On Tue, Oct 14, 2014 at 01:04:35PM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
On Tue, Oct 14, 2014 at 01:04:35PM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Thomas Shao 

 -Original Message-
 From: Mike Surcouf [mailto:mps.surcouf.l...@gmail.com]
 Sent: Tuesday, October 14, 2014 9:17 PM
 To: Thomas Shao
 Cc: Dan Carpenter; t...@linutronix.de; gre...@linuxfoundation.org; linux-
 ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
 a...@canonical.com; jasow...@redhat.com; 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 ((100UL + USER_HZ/2) / USER_HZ). 
 In my box, it's 1.

 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 huis...@microsoft.com
 wrote:
 
  -Original Message-
  From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
  Sent: Tuesday, October 14, 2014 7:19 PM
  To: Thomas Shao
  Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
  ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
  a...@canonical.com; jasow...@redhat.com; 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_MAJOR1
#define HB_WS2008_VERSION  (HB_WS2008_MAJOR  16 |
  HB_MINOR)
  
   +#define  TIMESAMPLE_INTERVAL 50L  /* 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 majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  Please read the FAQ at  http://www.tux.org/lkml

RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Thomas Shao 

 -Original Message-
 From: Richard Cochran [mailto:richardcoch...@gmail.com]
 Sent: Tuesday, October 14, 2014 9:20 PM
 To: Thomas Shao
 Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
 ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
 a...@canonical.com; jasow...@redhat.com; KY Srinivasan
 Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample
 
 On Tue, Oct 14, 2014 at 01:04:35PM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Thomas Shao 

 -Original Message-
 From: Richard Cochran [mailto:richardcoch...@gmail.com]
 Sent: Tuesday, October 14, 2014 9:26 PM
 To: Thomas Shao
 Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
 ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
 a...@canonical.com; jasow...@redhat.com; KY Srinivasan
 Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample
 
 On Tue, Oct 14, 2014 at 01:04:35PM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
Maybe John Stultz should also go onto CC.

Thanks,
Richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Victor Miasnikov 

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/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Richard Cochran 
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Mike Surcouf 
The value for TICK_USEC is defined as ((100UL + USER_HZ/2) / USER_HZ).
 In my box, it's 1.
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

2014-10-14 Thread Joe Perches 
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_MAJOR1
   #define HB_WS2008_VERSION  (HB_WS2008_MAJOR  16 | HB_MINOR)
   
  +#define  TIMESAMPLE_INTERVAL 50L  /* 5s in nanosecond */
 
 If you wanted you could say:
 
 #define  TIMESAMPLE_INTERVAL  (5 * NSEC_PER_SEC)

ULL


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel