Message-ID: <CD2CABCB2C0A0D4682C5F8AD840141540BFE97@HKXPRD3002MB006.064d.mgd.msft.net>
Date:	Tue, 14 Oct 2014 14:00:38 +0000
From:	Thomas Shao <huishao@...rosoft.com>
To:	Mike Surcouf <mps.surcouf.lkml@...il.com>
CC:	Dan Carpenter <dan.carpenter@...cle.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	KY Srinivasan <kys@...rosoft.com>
Subject: RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample


> -----Original Message-----
> From: Mike Surcouf [mailto:mps.surcouf.lkml@...il.com]
> Sent: Tuesday, October 14, 2014 9:17 PM
> To: Thomas Shao
> Cc: Dan Carpenter; tglx@...utronix.de; gregkh@...uxfoundation.org; linux-
> kernel@...r.kernel.org; devel@...uxdriverproject.org; olaf@...fle.de;
> apw@...onical.com; jasowang@...hat.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 ((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 <huishao@...rosoft.com>
> wrote:
> >
> >> -----Original Message-----
> >> From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
> >> Sent: Tuesday, October 14, 2014 7:19 PM
> >> To: Thomas Shao
> >> Cc: tglx@...utronix.de; gregkh@...uxfoundation.org; linux-
> >> kernel@...r.kernel.org; devel@...uxdriverproject.org; olaf@...fle.de;
> >> apw@...onical.com; jasowang@...hat.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_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 majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists