> -----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 ((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 <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_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 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
