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 <huishao@xxxxxxxxxxxxx> wrote: > >> -----Original Message----- >> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] >> Sent: Tuesday, October 14, 2014 7:19 PM >> To: Thomas Shao >> Cc: tglx@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux- >> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; >> apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
