| Message ID | 20170920224522.GB20974@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > On Wed, Sep 20, 2017 at 05:51:41PM +0100, Russell King - ARM Linux wrote: > > I sort-of agree as far as the time offset information goes, but there's > > a complication that we only open the RTC to set the time at the > > point in > > Hi Russell, > > What do you think of this untested approach in the below patch? > > Upon more careful inspection I think I found a way to make the > rounding in rtc_set_ntp_time compatible with a wide range of rtc > devices, so the subsecond capable ops I suggested do not seem > necessary. Looks like a possibility, but... > +/* True if now.tv_nsec is close enough to time_sec_nsec that we can call the > + * drivers's set_time_ns(). Arbitrarily choose 1ms as the allowable error > + * margin. > + * > + * This also rounds tv_sec to the second that covers the tv_nsec tick we are > + * targeting. > + */ > +#define TIME_SET_NSEC_FUZZ (1000*1000) > +static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, > + struct timespec64 *now) > +{ > + long diff; > + > + diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; > + if (diff < TIME_SET_NSEC_FUZZ) { > + if (rtc->time_set_nsec + diff >= NSEC_PER_SEC) { > + now->tv_sec -= 1; > + now->tv_nsec = NSEC_PER_SEC-1; > + } > + return true; > + } > + > + diff = (rtc->time_set_nsec - now->tv_nsec) % NSEC_PER_SEC; > + if (diff < TIME_SET_NSEC_FUZZ) { > + if (now->tv_nsec + diff >= NSEC_PER_SEC) { > + now->tv_sec += 1; > + now->tv_nsec = 0; > + } > + return true; > + } I don't think this is correct - and I think it's overly expensive. I threw this into a test program to prove the point, and it confirmed my suspicions - the above will always return true. Here's the test program output: now=0.990000000, offset= 0: diff= 990000000 true 2: 0.990000000 now=0.999000000, offset= 0: diff= 999000000 true 2: 0.999000000 now=0.999900000, offset= 0: diff= 999900000 true 2: 0.999900000 now=1.000000000, offset= 0: diff= 0 true 1: 1.000000000 now=1.000100000, offset= 0: diff= 100000 true 1: 1.000100000 now=1.001000000, offset= 0: diff= 1000000 true 2: 1.001000000 now=1.010000000, offset= 0: diff= 10000000 true 2: 1.010000000 now=0.990000000, offset= 470000000: diff= 520000000 true 2: 0.990000000 now=0.999000000, offset= 470000000: diff= 529000000 true 2: 0.999000000 now=0.999900000, offset= 470000000: diff= 529900000 true 2: 0.999900000 now=1.000000000, offset= 470000000: diff=-470000000 true 1: 1.000000000 now=1.000100000, offset= 470000000: diff=-469900000 true 1: 1.000100000 now=1.001000000, offset= 470000000: diff=-469000000 true 1: 1.001000000 now=1.010000000, offset= 470000000: diff=-460000000 true 1: 1.010000000 now=0.990000000, offset=1000000000: diff= -10000000 true 1: 0.990000000 now=0.999000000, offset=1000000000: diff= -1000000 true 1: 0.999000000 now=0.999900000, offset=1000000000: diff= -100000 true 1: 0.999900000 now=1.000000000, offset=1000000000: diff= 0 true 1: 0.999999999 now=1.000100000, offset=1000000000: diff=-999900000 true 1: 1.000100000 now=1.001000000, offset=1000000000: diff=-999000000 true 1: 1.001000000 now=1.010000000, offset=1000000000: diff=-990000000 true 1: 1.010000000 now=0.990000000, offset=1470000000: diff=-480000000 true 1: 0.990000000 now=0.999000000, offset=1470000000: diff=-471000000 true 1: 0.999000000 now=0.999900000, offset=1470000000: diff=-470100000 true 1: 0.999900000 now=1.000000000, offset=1470000000: diff=-470000000 true 1: 0.999999999 now=1.000100000, offset=1470000000: diff=-469900000 true 1: 0.999999999 now=1.001000000, offset=1470000000: diff=-469000000 true 1: 0.999999999 now=1.010000000, offset=1470000000: diff=-460000000 true 1: 0.999999999 which should be self explanatory. Another point to note is that the computation of the time to be set is also incorrect - for example, in the case of offset=0, we can see if we're slightly early, we set tv_sec=0 rather than tv_sec=1. In the offset=1sec case, it also isn't working as we'd expect - this case should be providing tv_sec for the preceding second, but it doesn't. I don't yet have a proposal to fix this...
On Thu, Sep 21, 2017 at 08:59:07AM +0100, Russell King - ARM Linux wrote: > On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > > On Wed, Sep 20, 2017 at 05:51:41PM +0100, Russell King - ARM Linux wrote: > > > I sort-of agree as far as the time offset information goes, but there's > > > a complication that we only open the RTC to set the time at the > > > point in > > > > Hi Russell, > > > > What do you think of this untested approach in the below patch? > > > > Upon more careful inspection I think I found a way to make the > > rounding in rtc_set_ntp_time compatible with a wide range of rtc > > devices, so the subsecond capable ops I suggested do not seem > > necessary. > > Looks like a possibility, but... > > > +/* True if now.tv_nsec is close enough to time_sec_nsec that we can call the > > + * drivers's set_time_ns(). Arbitrarily choose 1ms as the allowable error > > + * margin. > > + * > > + * This also rounds tv_sec to the second that covers the tv_nsec tick we are > > + * targeting. > > + */ > > +#define TIME_SET_NSEC_FUZZ (1000*1000) > > +static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, > > + struct timespec64 *now) > > +{ > > + long diff; > > + > > + diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; > > + if (diff < TIME_SET_NSEC_FUZZ) { > > + if (rtc->time_set_nsec + diff >= NSEC_PER_SEC) { > > + now->tv_sec -= 1; > > + now->tv_nsec = NSEC_PER_SEC-1; > > + } > > + return true; > > + } > > + > > + diff = (rtc->time_set_nsec - now->tv_nsec) % NSEC_PER_SEC; > > + if (diff < TIME_SET_NSEC_FUZZ) { > > + if (now->tv_nsec + diff >= NSEC_PER_SEC) { > > + now->tv_sec += 1; > > + now->tv_nsec = 0; > > + } > > + return true; > > + } > > I don't think this is correct - and I think it's overly expensive. > I threw this into a test program to prove the point, and it confirmed > my suspicions - the above will always return true. > > Here's the test program output: > > now=0.990000000, offset= 0: diff= 990000000 true 2: 0.990000000 > now=0.999000000, offset= 0: diff= 999000000 true 2: 0.999000000 > now=0.999900000, offset= 0: diff= 999900000 true 2: 0.999900000 > now=1.000000000, offset= 0: diff= 0 true 1: 1.000000000 > now=1.000100000, offset= 0: diff= 100000 true 1: 1.000100000 > now=1.001000000, offset= 0: diff= 1000000 true 2: 1.001000000 > now=1.010000000, offset= 0: diff= 10000000 true 2: 1.010000000 > now=0.990000000, offset= 470000000: diff= 520000000 true 2: 0.990000000 > now=0.999000000, offset= 470000000: diff= 529000000 true 2: 0.999000000 > now=0.999900000, offset= 470000000: diff= 529900000 true 2: 0.999900000 > now=1.000000000, offset= 470000000: diff=-470000000 true 1: 1.000000000 > now=1.000100000, offset= 470000000: diff=-469900000 true 1: 1.000100000 > now=1.001000000, offset= 470000000: diff=-469000000 true 1: 1.001000000 > now=1.010000000, offset= 470000000: diff=-460000000 true 1: 1.010000000 > now=0.990000000, offset=1000000000: diff= -10000000 true 1: 0.990000000 > now=0.999000000, offset=1000000000: diff= -1000000 true 1: 0.999000000 > now=0.999900000, offset=1000000000: diff= -100000 true 1: 0.999900000 > now=1.000000000, offset=1000000000: diff= 0 true 1: 0.999999999 > now=1.000100000, offset=1000000000: diff=-999900000 true 1: 1.000100000 > now=1.001000000, offset=1000000000: diff=-999000000 true 1: 1.001000000 > now=1.010000000, offset=1000000000: diff=-990000000 true 1: 1.010000000 > now=0.990000000, offset=1470000000: diff=-480000000 true 1: 0.990000000 > now=0.999000000, offset=1470000000: diff=-471000000 true 1: 0.999000000 > now=0.999900000, offset=1470000000: diff=-470100000 true 1: 0.999900000 > now=1.000000000, offset=1470000000: diff=-470000000 true 1: 0.999999999 > now=1.000100000, offset=1470000000: diff=-469900000 true 1: 0.999999999 > now=1.001000000, offset=1470000000: diff=-469000000 true 1: 0.999999999 > now=1.010000000, offset=1470000000: diff=-460000000 true 1: 0.999999999 > > which should be self explanatory. Another point to note is that > the computation of the time to be set is also incorrect - for > example, in the case of offset=0, we can see if we're slightly > early, we set tv_sec=0 rather than tv_sec=1. > > In the offset=1sec case, it also isn't working as we'd expect - > this case should be providing tv_sec for the preceding second, but > it doesn't. > > I don't yet have a proposal to fix this... How about this: static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, struct timespec64 *now) { long diff; diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; /* diff must be within [-NSEC_PER_SEC/2..NSEC_PER_SEC/2) */ if (diff >= NSEC_PER_SEC / 2) diff -= NSEC_PER_SEC; else if (diff < -NSEC_PER_SEC / 2) diff += NSEC_PER_SEC; if (abs(diff) < TIME_SET_NSEC_FUZZ) { /* Correct the time for the rtc->time_set_nsec and difference */ diff += rtc->time_set_nsec; if (diff > 0) timespec64_sub_ns(now, diff); else if (diff < 0) timespec64_add_ns(now, -diff); return true; } return false; } This always returns now->tv_nsec = 0, but with now->tv_sec appropriately adjusted. The return value is true if we're within 1msec of the required nanoseconds, false otherwise. Here's the results of my test program, which looks a lot better to me: now=0.469000000, offset=-1000000000: diff= 469000000 false now=0.469000001, offset=-1000000000: diff= 469000001 false now=0.470000000, offset=-1000000000: diff= 470000000 false now=0.470999999, offset=-1000000000: diff= 470999999 false now=0.471000000, offset=-1000000000: diff= 471000000 false now=0.499999999, offset=-1000000000: diff= 499999999 false now=0.500000000, offset=-1000000000: diff=-500000000 false now=0.990000000, offset=-1000000000: diff= -10000000 false now=0.999000000, offset=-1000000000: diff= -1000000 false now=0.999900000, offset=-1000000000: diff= -100000 true: 2.000000000 now=1.000000000, offset=-1000000000: diff= 0 true: 2.000000000 now=1.000100000, offset=-1000000000: diff= 100000 true: 2.000000000 now=1.001000000, offset=-1000000000: diff= 1000000 false now=1.010000000, offset=-1000000000: diff= 10000000 false now=1.469000000, offset=-1000000000: diff= 469000000 false now=1.469000001, offset=-1000000000: diff= 469000001 false now=1.470000000, offset=-1000000000: diff= 470000000 false now=1.470999999, offset=-1000000000: diff= 470999999 false now=1.471000000, offset=-1000000000: diff= 471000000 false now=1.499999999, offset=-1000000000: diff= 499999999 false now=1.500000000, offset=-1000000000: diff=-500000000 false now=0.469000000, offset= -530000000: diff= -1000000 false now=0.469000001, offset= -530000000: diff= -999999 true: 1.000000000 now=0.470000000, offset= -530000000: diff= 0 true: 1.000000000 now=0.470999999, offset= -530000000: diff= 999999 true: 1.000000000 now=0.471000000, offset= -530000000: diff= 1000000 false now=0.499999999, offset= -530000000: diff= 29999999 false now=0.500000000, offset= -530000000: diff= 30000000 false now=0.990000000, offset= -530000000: diff=-480000000 false now=0.999000000, offset= -530000000: diff=-471000000 false now=0.999900000, offset= -530000000: diff=-470100000 false now=1.000000000, offset= -530000000: diff=-470000000 false now=1.000100000, offset= -530000000: diff=-469900000 false now=1.001000000, offset= -530000000: diff=-469000000 false now=1.010000000, offset= -530000000: diff=-460000000 false now=1.469000000, offset= -530000000: diff= -1000000 false now=1.469000001, offset= -530000000: diff= -999999 true: 2.000000000 now=1.470000000, offset= -530000000: diff= 0 true: 2.000000000 now=1.470999999, offset= -530000000: diff= 999999 true: 2.000000000 now=1.471000000, offset= -530000000: diff= 1000000 false now=1.499999999, offset= -530000000: diff= 29999999 false now=1.500000000, offset= -530000000: diff= 30000000 false now=0.469000000, offset= 0: diff= 469000000 false now=0.469000001, offset= 0: diff= 469000001 false now=0.470000000, offset= 0: diff= 470000000 false now=0.470999999, offset= 0: diff= 470999999 false now=0.471000000, offset= 0: diff= 471000000 false now=0.499999999, offset= 0: diff= 499999999 false now=0.500000000, offset= 0: diff=-500000000 false now=0.990000000, offset= 0: diff= -10000000 false now=0.999000000, offset= 0: diff= -1000000 false now=0.999900000, offset= 0: diff= -100000 true: 1.000000000 now=1.000000000, offset= 0: diff= 0 true: 1.000000000 now=1.000100000, offset= 0: diff= 100000 true: 1.000000000 now=1.001000000, offset= 0: diff= 1000000 false now=1.010000000, offset= 0: diff= 10000000 false now=1.469000000, offset= 0: diff= 469000000 false now=1.469000001, offset= 0: diff= 469000001 false now=1.470000000, offset= 0: diff= 470000000 false now=1.470999999, offset= 0: diff= 470999999 false now=1.471000000, offset= 0: diff= 471000000 false now=1.499999999, offset= 0: diff= 499999999 false now=1.500000000, offset= 0: diff=-500000000 false now=0.469000000, offset= 470000000: diff= -1000000 false now=0.469000001, offset= 470000000: diff= -999999 true: 0.000000000 now=0.470000000, offset= 470000000: diff= 0 true: 0.000000000 now=0.470999999, offset= 470000000: diff= 999999 true: 0.000000000 now=0.471000000, offset= 470000000: diff= 1000000 false now=0.499999999, offset= 470000000: diff= 29999999 false now=0.500000000, offset= 470000000: diff= 30000000 false now=0.990000000, offset= 470000000: diff=-480000000 false now=0.999000000, offset= 470000000: diff=-471000000 false now=0.999900000, offset= 470000000: diff=-470100000 false now=1.000000000, offset= 470000000: diff=-470000000 false now=1.000100000, offset= 470000000: diff=-469900000 false now=1.001000000, offset= 470000000: diff=-469000000 false now=1.010000000, offset= 470000000: diff=-460000000 false now=1.469000000, offset= 470000000: diff= -1000000 false now=1.469000001, offset= 470000000: diff= -999999 true: 1.000000000 now=1.470000000, offset= 470000000: diff= 0 true: 1.000000000 now=1.470999999, offset= 470000000: diff= 999999 true: 1.000000000 now=1.471000000, offset= 470000000: diff= 1000000 false now=1.499999999, offset= 470000000: diff= 29999999 false now=1.500000000, offset= 470000000: diff= 30000000 false now=0.469000000, offset= 1000000000: diff= 469000000 false now=0.469000001, offset= 1000000000: diff= 469000001 false now=0.470000000, offset= 1000000000: diff= 470000000 false now=0.470999999, offset= 1000000000: diff= 470999999 false now=0.471000000, offset= 1000000000: diff= 471000000 false now=0.499999999, offset= 1000000000: diff= 499999999 false now=0.500000000, offset= 1000000000: diff=-500000000 false now=0.990000000, offset= 1000000000: diff= -10000000 false now=0.999000000, offset= 1000000000: diff= -1000000 false now=0.999900000, offset= 1000000000: diff= -100000 true: 0.000000000 now=1.000000000, offset= 1000000000: diff= 0 true: 0.000000000 now=1.000100000, offset= 1000000000: diff= 100000 true: 0.000000000 now=1.001000000, offset= 1000000000: diff= 1000000 false now=1.010000000, offset= 1000000000: diff= 10000000 false now=1.469000000, offset= 1000000000: diff= 469000000 false now=1.469000001, offset= 1000000000: diff= 469000001 false now=1.470000000, offset= 1000000000: diff= 470000000 false now=1.470999999, offset= 1000000000: diff= 470999999 false now=1.471000000, offset= 1000000000: diff= 471000000 false now=1.499999999, offset= 1000000000: diff= 499999999 false now=1.500000000, offset= 1000000000: diff=-500000000 false now=0.469000000, offset= 1470000000: diff= -1000000 false now=0.469000001, offset= 1470000000: diff= -999999 true: -1.000000000 now=0.470000000, offset= 1470000000: diff= 0 true: -1.000000000 now=0.470999999, offset= 1470000000: diff= 999999 true: -1.000000000 now=0.471000000, offset= 1470000000: diff= 1000000 false now=0.499999999, offset= 1470000000: diff= 29999999 false now=0.500000000, offset= 1470000000: diff= 30000000 false now=0.990000000, offset= 1470000000: diff=-480000000 false now=0.999000000, offset= 1470000000: diff=-471000000 false now=0.999900000, offset= 1470000000: diff=-470100000 false now=1.000000000, offset= 1470000000: diff=-470000000 false now=1.000100000, offset= 1470000000: diff=-469900000 false now=1.001000000, offset= 1470000000: diff=-469000000 false now=1.010000000, offset= 1470000000: diff=-460000000 false now=1.469000000, offset= 1470000000: diff= -1000000 false now=1.469000001, offset= 1470000000: diff= -999999 true: 0.000000000 now=1.470000000, offset= 1470000000: diff= 0 true: 0.000000000 now=1.470999999, offset= 1470000000: diff= 999999 true: 0.000000000 now=1.471000000, offset= 1470000000: diff= 1000000 false now=1.499999999, offset= 1470000000: diff= 29999999 false now=1.500000000, offset= 1470000000: diff= 30000000 false
On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > /** > * rtc_set_ntp_time - Save NTP synchronized time to the RTC > * @now: Current time of day > + * @target_nsec: Output value indicating what now->tv_nsec I'd suggest: * @target_nsec: pointer for desired now->tv_nsec value > * > * Replacement for the NTP platform function update_persistent_clock64 > * that stores time for later retrieval by rtc_hctosys. > @@ -20,28 +54,35 @@ > * > * If temporary failure is indicated the caller should try again 'soon' > */ > -int rtc_set_ntp_time(struct timespec64 now) > +int rtc_set_ntp_time(struct timespec64 now, long *target_nsec) > { > struct rtc_device *rtc; > struct rtc_time tm; > int err = -ENODEV; > > - if (now.tv_nsec < (NSEC_PER_SEC >> 1)) > - rtc_time64_to_tm(now.tv_sec, &tm); > - else > - rtc_time64_to_tm(now.tv_sec + 1, &tm); > - > rtc = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE); > - if (rtc) { > + if (!rtc) > + goto out_err; > + > + /* The ntp code must call this with the correct value in tv_nsec, if > + * it does not we update target_nsec and return EPROTO to make the ntp > + * code try again later. > + */ > + *target_nsec = rtc->time_set_nsec; Here, we want just the positive nanoseconds part of the desired offset (iow, the value we want to see in now.tv_nsec): nsec = rtc->time_set_nsec % NSEC_PER_SEC; if (nsec < 0) nsec += NSEC_PER_SEC; *target_nsec = nsec;
On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > + if (!fail || fail == -ENODEV) > + timespec64_add_ns(&next, 659 * NSEC_PER_SEC); > + else > + /* Update failed, try again in about 10 seconds */ > + timespec64_add_ns(&next, 10 * NSEC_PER_SEC); Trying to build this, the compiler complains about integer overflow here. You need to cast these to 64-bit values.
On Thu, Sep 21, 2017 at 10:32:19AM +0100, Russell King - ARM Linux wrote: > On Thu, Sep 21, 2017 at 08:59:07AM +0100, Russell King - ARM Linux wrote: > > On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > > > On Wed, Sep 20, 2017 at 05:51:41PM +0100, Russell King - ARM Linux wrote: > > > > I sort-of agree as far as the time offset information goes, but there's > > > > a complication that we only open the RTC to set the time at the > > > > point in > > > > > > Hi Russell, > > > > > > What do you think of this untested approach in the below patch? > > > > > > Upon more careful inspection I think I found a way to make the > > > rounding in rtc_set_ntp_time compatible with a wide range of rtc > > > devices, so the subsecond capable ops I suggested do not seem > > > necessary. > > > > Looks like a possibility, but... > > > > > +/* True if now.tv_nsec is close enough to time_sec_nsec that we can call the > > > + * drivers's set_time_ns(). Arbitrarily choose 1ms as the allowable error > > > + * margin. > > > + * > > > + * This also rounds tv_sec to the second that covers the tv_nsec tick we are > > > + * targeting. > > > + */ > > > +#define TIME_SET_NSEC_FUZZ (1000*1000) > > > +static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, > > > + struct timespec64 *now) > > > +{ > > > + long diff; > > > + > > > + diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; > > > + if (diff < TIME_SET_NSEC_FUZZ) { > > > + if (rtc->time_set_nsec + diff >= NSEC_PER_SEC) { > > > + now->tv_sec -= 1; > > > + now->tv_nsec = NSEC_PER_SEC-1; > > > + } > > > + return true; > > > + } > > > + > > > + diff = (rtc->time_set_nsec - now->tv_nsec) % NSEC_PER_SEC; > > > + if (diff < TIME_SET_NSEC_FUZZ) { > > > + if (now->tv_nsec + diff >= NSEC_PER_SEC) { > > > + now->tv_sec += 1; > > > + now->tv_nsec = 0; > > > + } > > > + return true; > > > + } > > > > I don't think this is correct - and I think it's overly expensive. > > I threw this into a test program to prove the point, and it confirmed > > my suspicions - the above will always return true. > > > > Here's the test program output: > > > > now=0.990000000, offset= 0: diff= 990000000 true 2: 0.990000000 > > now=0.999000000, offset= 0: diff= 999000000 true 2: 0.999000000 > > now=0.999900000, offset= 0: diff= 999900000 true 2: 0.999900000 > > now=1.000000000, offset= 0: diff= 0 true 1: 1.000000000 > > now=1.000100000, offset= 0: diff= 100000 true 1: 1.000100000 > > now=1.001000000, offset= 0: diff= 1000000 true 2: 1.001000000 > > now=1.010000000, offset= 0: diff= 10000000 true 2: 1.010000000 > > now=0.990000000, offset= 470000000: diff= 520000000 true 2: 0.990000000 > > now=0.999000000, offset= 470000000: diff= 529000000 true 2: 0.999000000 > > now=0.999900000, offset= 470000000: diff= 529900000 true 2: 0.999900000 > > now=1.000000000, offset= 470000000: diff=-470000000 true 1: 1.000000000 > > now=1.000100000, offset= 470000000: diff=-469900000 true 1: 1.000100000 > > now=1.001000000, offset= 470000000: diff=-469000000 true 1: 1.001000000 > > now=1.010000000, offset= 470000000: diff=-460000000 true 1: 1.010000000 > > now=0.990000000, offset=1000000000: diff= -10000000 true 1: 0.990000000 > > now=0.999000000, offset=1000000000: diff= -1000000 true 1: 0.999000000 > > now=0.999900000, offset=1000000000: diff= -100000 true 1: 0.999900000 > > now=1.000000000, offset=1000000000: diff= 0 true 1: 0.999999999 > > now=1.000100000, offset=1000000000: diff=-999900000 true 1: 1.000100000 > > now=1.001000000, offset=1000000000: diff=-999000000 true 1: 1.001000000 > > now=1.010000000, offset=1000000000: diff=-990000000 true 1: 1.010000000 > > now=0.990000000, offset=1470000000: diff=-480000000 true 1: 0.990000000 > > now=0.999000000, offset=1470000000: diff=-471000000 true 1: 0.999000000 > > now=0.999900000, offset=1470000000: diff=-470100000 true 1: 0.999900000 > > now=1.000000000, offset=1470000000: diff=-470000000 true 1: 0.999999999 > > now=1.000100000, offset=1470000000: diff=-469900000 true 1: 0.999999999 > > now=1.001000000, offset=1470000000: diff=-469000000 true 1: 0.999999999 > > now=1.010000000, offset=1470000000: diff=-460000000 true 1: 0.999999999 > > > > which should be self explanatory. Another point to note is that > > the computation of the time to be set is also incorrect - for > > example, in the case of offset=0, we can see if we're slightly > > early, we set tv_sec=0 rather than tv_sec=1. > > > > In the offset=1sec case, it also isn't working as we'd expect - > > this case should be providing tv_sec for the preceding second, but > > it doesn't. > > > > I don't yet have a proposal to fix this... > > How about this: > > static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, > struct timespec64 *now) > { > long diff; > > diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; > /* diff must be within [-NSEC_PER_SEC/2..NSEC_PER_SEC/2) */ > if (diff >= NSEC_PER_SEC / 2) > diff -= NSEC_PER_SEC; > else if (diff < -NSEC_PER_SEC / 2) > diff += NSEC_PER_SEC; > > if (abs(diff) < TIME_SET_NSEC_FUZZ) { > /* Correct the time for the rtc->time_set_nsec and difference */ > diff += rtc->time_set_nsec; > if (diff > 0) > timespec64_sub_ns(now, diff); The only issue here is that we don't have this function... > else if (diff < 0) > timespec64_add_ns(now, -diff); > return true; > } > return false; > } > > This always returns now->tv_nsec = 0, but with now->tv_sec appropriately > adjusted. The return value is true if we're within 1msec of the required > nanoseconds, false otherwise. > > Here's the results of my test program, which looks a lot better to me: > > now=0.469000000, offset=-1000000000: diff= 469000000 false > now=0.469000001, offset=-1000000000: diff= 469000001 false > now=0.470000000, offset=-1000000000: diff= 470000000 false > now=0.470999999, offset=-1000000000: diff= 470999999 false > now=0.471000000, offset=-1000000000: diff= 471000000 false > now=0.499999999, offset=-1000000000: diff= 499999999 false > now=0.500000000, offset=-1000000000: diff=-500000000 false > now=0.990000000, offset=-1000000000: diff= -10000000 false > now=0.999000000, offset=-1000000000: diff= -1000000 false > now=0.999900000, offset=-1000000000: diff= -100000 true: 2.000000000 > now=1.000000000, offset=-1000000000: diff= 0 true: 2.000000000 > now=1.000100000, offset=-1000000000: diff= 100000 true: 2.000000000 > now=1.001000000, offset=-1000000000: diff= 1000000 false > now=1.010000000, offset=-1000000000: diff= 10000000 false > now=1.469000000, offset=-1000000000: diff= 469000000 false > now=1.469000001, offset=-1000000000: diff= 469000001 false > now=1.470000000, offset=-1000000000: diff= 470000000 false > now=1.470999999, offset=-1000000000: diff= 470999999 false > now=1.471000000, offset=-1000000000: diff= 471000000 false > now=1.499999999, offset=-1000000000: diff= 499999999 false > now=1.500000000, offset=-1000000000: diff=-500000000 false > > now=0.469000000, offset= -530000000: diff= -1000000 false > now=0.469000001, offset= -530000000: diff= -999999 true: 1.000000000 > now=0.470000000, offset= -530000000: diff= 0 true: 1.000000000 > now=0.470999999, offset= -530000000: diff= 999999 true: 1.000000000 > now=0.471000000, offset= -530000000: diff= 1000000 false > now=0.499999999, offset= -530000000: diff= 29999999 false > now=0.500000000, offset= -530000000: diff= 30000000 false > now=0.990000000, offset= -530000000: diff=-480000000 false > now=0.999000000, offset= -530000000: diff=-471000000 false > now=0.999900000, offset= -530000000: diff=-470100000 false > now=1.000000000, offset= -530000000: diff=-470000000 false > now=1.000100000, offset= -530000000: diff=-469900000 false > now=1.001000000, offset= -530000000: diff=-469000000 false > now=1.010000000, offset= -530000000: diff=-460000000 false > now=1.469000000, offset= -530000000: diff= -1000000 false > now=1.469000001, offset= -530000000: diff= -999999 true: 2.000000000 > now=1.470000000, offset= -530000000: diff= 0 true: 2.000000000 > now=1.470999999, offset= -530000000: diff= 999999 true: 2.000000000 > now=1.471000000, offset= -530000000: diff= 1000000 false > now=1.499999999, offset= -530000000: diff= 29999999 false > now=1.500000000, offset= -530000000: diff= 30000000 false > > now=0.469000000, offset= 0: diff= 469000000 false > now=0.469000001, offset= 0: diff= 469000001 false > now=0.470000000, offset= 0: diff= 470000000 false > now=0.470999999, offset= 0: diff= 470999999 false > now=0.471000000, offset= 0: diff= 471000000 false > now=0.499999999, offset= 0: diff= 499999999 false > now=0.500000000, offset= 0: diff=-500000000 false > now=0.990000000, offset= 0: diff= -10000000 false > now=0.999000000, offset= 0: diff= -1000000 false > now=0.999900000, offset= 0: diff= -100000 true: 1.000000000 > now=1.000000000, offset= 0: diff= 0 true: 1.000000000 > now=1.000100000, offset= 0: diff= 100000 true: 1.000000000 > now=1.001000000, offset= 0: diff= 1000000 false > now=1.010000000, offset= 0: diff= 10000000 false > now=1.469000000, offset= 0: diff= 469000000 false > now=1.469000001, offset= 0: diff= 469000001 false > now=1.470000000, offset= 0: diff= 470000000 false > now=1.470999999, offset= 0: diff= 470999999 false > now=1.471000000, offset= 0: diff= 471000000 false > now=1.499999999, offset= 0: diff= 499999999 false > now=1.500000000, offset= 0: diff=-500000000 false > > now=0.469000000, offset= 470000000: diff= -1000000 false > now=0.469000001, offset= 470000000: diff= -999999 true: 0.000000000 > now=0.470000000, offset= 470000000: diff= 0 true: 0.000000000 > now=0.470999999, offset= 470000000: diff= 999999 true: 0.000000000 > now=0.471000000, offset= 470000000: diff= 1000000 false > now=0.499999999, offset= 470000000: diff= 29999999 false > now=0.500000000, offset= 470000000: diff= 30000000 false > now=0.990000000, offset= 470000000: diff=-480000000 false > now=0.999000000, offset= 470000000: diff=-471000000 false > now=0.999900000, offset= 470000000: diff=-470100000 false > now=1.000000000, offset= 470000000: diff=-470000000 false > now=1.000100000, offset= 470000000: diff=-469900000 false > now=1.001000000, offset= 470000000: diff=-469000000 false > now=1.010000000, offset= 470000000: diff=-460000000 false > now=1.469000000, offset= 470000000: diff= -1000000 false > now=1.469000001, offset= 470000000: diff= -999999 true: 1.000000000 > now=1.470000000, offset= 470000000: diff= 0 true: 1.000000000 > now=1.470999999, offset= 470000000: diff= 999999 true: 1.000000000 > now=1.471000000, offset= 470000000: diff= 1000000 false > now=1.499999999, offset= 470000000: diff= 29999999 false > now=1.500000000, offset= 470000000: diff= 30000000 false > > now=0.469000000, offset= 1000000000: diff= 469000000 false > now=0.469000001, offset= 1000000000: diff= 469000001 false > now=0.470000000, offset= 1000000000: diff= 470000000 false > now=0.470999999, offset= 1000000000: diff= 470999999 false > now=0.471000000, offset= 1000000000: diff= 471000000 false > now=0.499999999, offset= 1000000000: diff= 499999999 false > now=0.500000000, offset= 1000000000: diff=-500000000 false > now=0.990000000, offset= 1000000000: diff= -10000000 false > now=0.999000000, offset= 1000000000: diff= -1000000 false > now=0.999900000, offset= 1000000000: diff= -100000 true: 0.000000000 > now=1.000000000, offset= 1000000000: diff= 0 true: 0.000000000 > now=1.000100000, offset= 1000000000: diff= 100000 true: 0.000000000 > now=1.001000000, offset= 1000000000: diff= 1000000 false > now=1.010000000, offset= 1000000000: diff= 10000000 false > now=1.469000000, offset= 1000000000: diff= 469000000 false > now=1.469000001, offset= 1000000000: diff= 469000001 false > now=1.470000000, offset= 1000000000: diff= 470000000 false > now=1.470999999, offset= 1000000000: diff= 470999999 false > now=1.471000000, offset= 1000000000: diff= 471000000 false > now=1.499999999, offset= 1000000000: diff= 499999999 false > now=1.500000000, offset= 1000000000: diff=-500000000 false > > now=0.469000000, offset= 1470000000: diff= -1000000 false > now=0.469000001, offset= 1470000000: diff= -999999 true: -1.000000000 > now=0.470000000, offset= 1470000000: diff= 0 true: -1.000000000 > now=0.470999999, offset= 1470000000: diff= 999999 true: -1.000000000 > now=0.471000000, offset= 1470000000: diff= 1000000 false > now=0.499999999, offset= 1470000000: diff= 29999999 false > now=0.500000000, offset= 1470000000: diff= 30000000 false > now=0.990000000, offset= 1470000000: diff=-480000000 false > now=0.999000000, offset= 1470000000: diff=-471000000 false > now=0.999900000, offset= 1470000000: diff=-470100000 false > now=1.000000000, offset= 1470000000: diff=-470000000 false > now=1.000100000, offset= 1470000000: diff=-469900000 false > now=1.001000000, offset= 1470000000: diff=-469000000 false > now=1.010000000, offset= 1470000000: diff=-460000000 false > now=1.469000000, offset= 1470000000: diff= -1000000 false > now=1.469000001, offset= 1470000000: diff= -999999 true: 0.000000000 > now=1.470000000, offset= 1470000000: diff= 0 true: 0.000000000 > now=1.470999999, offset= 1470000000: diff= 999999 true: 0.000000000 > now=1.471000000, offset= 1470000000: diff= 1000000 false > now=1.499999999, offset= 1470000000: diff= 29999999 false > now=1.500000000, offset= 1470000000: diff= 30000000 false > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up > According to speedtest.net: 8.21Mbps down 510kbps up > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote:
> What do you think of this untested approach in the below patch?
There's another issue.
Most drivers use rtc_device_register() or devm_rtc_device_register()
rather than rtc_allocate_device() (which is static.) This does not
give RTC drivers a chance to set rtc->time_set_nsec before the
RTC is registered with the kernel.
Setting it after the device has been registered is racy. So, having
this member in struct rtc_device and assuming that drivers will override
the value doesn't work.
It doesn't make sense to put it in rtc_class_ops as it isn't an
operation, but we could add a callback in there which is used during
initialisation but before registration which could be used to set this
member.
Thoughts?
On 21/09/2017 at 13:28:53 +0100, Russell King - ARM Linux wrote: > On Wed, Sep 20, 2017 at 04:45:22PM -0600, Jason Gunthorpe wrote: > > What do you think of this untested approach in the below patch? > > There's another issue. > > Most drivers use rtc_device_register() or devm_rtc_device_register() > rather than rtc_allocate_device() (which is static.) This does not It is static on purpose, drivers must use devm_rtc_allocate_device() if they need to allocate the rtc before registering it. I'm in the process of converting most drivers, including rtc_cmos as this solve other race issues. > give RTC drivers a chance to set rtc->time_set_nsec before the > RTC is registered with the kernel. > > Setting it after the device has been registered is racy. So, having > this member in struct rtc_device and assuming that drivers will override > the value doesn't work. > > It doesn't make sense to put it in rtc_class_ops as it isn't an > operation, but we could add a callback in there which is used during > initialisation but before registration which could be used to set this > member. > I'd prefer having drivers that need to set the value to something other than 0 to convert to devm_rtc_allocate_device.
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index 2ed970d61da140..ed1a4e0f8742ba 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -161,6 +161,12 @@ static struct rtc_device *rtc_allocate_device(void) device_initialize(&rtc->dev); + /* Drivers can revise this default after allocating the device. It + * should be the value of wallclock tv_nsec that the driver needs in + * order to synchronize the second tick over during set. + */ + rtc->time_set_nsec = NSEC_PER_SEC / 2; + rtc->irq_freq = 1; rtc->max_user_freq = 64; rtc->dev.class = rtc_class; diff --git a/drivers/rtc/systohc.c b/drivers/rtc/systohc.c index b4a68ffcd06bb8..f756dc1804829b 100644 --- a/drivers/rtc/systohc.c +++ b/drivers/rtc/systohc.c @@ -7,9 +7,43 @@ #include <linux/rtc.h> #include <linux/time.h> +/* True if now.tv_nsec is close enough to time_sec_nsec that we can call the + * drivers's set_time_ns(). Arbitrarily choose 1ms as the allowable error + * margin. + * + * This also rounds tv_sec to the second that covers the tv_nsec tick we are + * targeting. + */ +#define TIME_SET_NSEC_FUZZ (1000*1000) +static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, + struct timespec64 *now) +{ + long diff; + + diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; + if (diff < TIME_SET_NSEC_FUZZ) { + if (rtc->time_set_nsec + diff >= NSEC_PER_SEC) { + now->tv_sec -= 1; + now->tv_nsec = NSEC_PER_SEC-1; + } + return true; + } + + diff = (rtc->time_set_nsec - now->tv_nsec) % NSEC_PER_SEC; + if (diff < TIME_SET_NSEC_FUZZ) { + if (now->tv_nsec + diff >= NSEC_PER_SEC) { + now->tv_sec += 1; + now->tv_nsec = 0; + } + return true; + } + return false; +} + /** * rtc_set_ntp_time - Save NTP synchronized time to the RTC * @now: Current time of day + * @target_nsec: Output value indicating what now->tv_nsec * * Replacement for the NTP platform function update_persistent_clock64 * that stores time for later retrieval by rtc_hctosys. @@ -20,28 +54,35 @@ * * If temporary failure is indicated the caller should try again 'soon' */ -int rtc_set_ntp_time(struct timespec64 now) +int rtc_set_ntp_time(struct timespec64 now, long *target_nsec) { struct rtc_device *rtc; struct rtc_time tm; int err = -ENODEV; - if (now.tv_nsec < (NSEC_PER_SEC >> 1)) - rtc_time64_to_tm(now.tv_sec, &tm); - else - rtc_time64_to_tm(now.tv_sec + 1, &tm); - rtc = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE); - if (rtc) { + if (!rtc) + goto out_err; + + /* The ntp code must call this with the correct value in tv_nsec, if + * it does not we update target_nsec and return EPROTO to make the ntp + * code try again later. + */ + *target_nsec = rtc->time_set_nsec; + if (!rtc_tv_nsec_ok(rtc, &now)) { + err = -EPROTO; + goto out_close; + } + + if (rtc->ops && (rtc->ops->set_time || rtc->ops->set_mmss64 || + rtc->ops->set_mmss)) { /* rtc_hctosys exclusively uses UTC, so we call set_time here, * not set_mmss. */ - if (rtc->ops && - (rtc->ops->set_time || - rtc->ops->set_mmss64 || - rtc->ops->set_mmss)) - err = rtc_set_time(rtc, &tm); - rtc_class_close(rtc); + err = rtc_set_time(rtc, &tm); } +out_close: + rtc_class_close(rtc); +out_err: return err; } diff --git a/include/linux/rtc.h b/include/linux/rtc.h index 0a0f0d14a5fba5..7f9858ef03111a 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -137,6 +137,8 @@ struct rtc_device { /* Some hardware can't support UIE mode */ int uie_unsupported; + long time_set_nsec; + bool registered; struct nvmem_config *nvmem_config; @@ -174,7 +176,7 @@ extern void devm_rtc_device_unregister(struct device *dev, extern int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm); extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm); -extern int rtc_set_ntp_time(struct timespec64 now); +extern int rtc_set_ntp_time(struct timespec64 now, long *target_nsec); int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm); extern int rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alrm); diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index edf19cc5314043..61fdf758dcb754 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -514,17 +514,19 @@ static DECLARE_DELAYED_WORK(sync_cmos_work, sync_cmos_clock); static void sync_cmos_clock(struct work_struct *work) { - struct timespec64 now; - struct timespec64 next; + struct timespec64 now, next, delta; int fail = 1; + long target_nsec = NSEC_PER_SEC / 2; /* - * If we have an externally synchronized Linux clock, then update - * CMOS clock accordingly every ~11 minutes. Set_rtc_mmss() has to be - * called as close as possible to 500 ms before the new second starts. - * This code is run on a timer. If the clock is set, that timer - * may not expire at the correct time. Thus, we adjust... - * We want the clock to be within a couple of ticks from the target. + * If we have an externally synchronized Linux clock, then update CMOS + * clock accordingly every ~11 minutes. Histiocally Set_rtc_mmss() + * has to be called as close as possible to 500 ms (target_nsec) + * before the new second starts, but new RTC drivers can select a + * different value. This code is run on a timer. If the clock is + * set, that timer may not expire at the correct time. Thus, we + * adjust... We want the clock to be within a couple of ticks from + * the target. */ if (!ntp_synced()) { /* @@ -547,25 +549,41 @@ static void sync_cmos_clock(struct work_struct *work) #ifdef CONFIG_RTC_SYSTOHC if (fail == -ENODEV) - fail = rtc_set_ntp_time(adjust); + fail = rtc_set_ntp_time(adjust, &target_nsec); #endif } - next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec - (TICK_NSEC / 2); - if (next.tv_nsec <= 0) - next.tv_nsec += NSEC_PER_SEC; + do { + /* + * Compute the next wall clock time to try and set the + * clock + */ + next = now; + if (!fail || fail == -ENODEV) + timespec64_add_ns(&next, 659 * NSEC_PER_SEC); + else + /* Update failed, try again in about 10 seconds */ + timespec64_add_ns(&next, 10 * NSEC_PER_SEC); - if (!fail || fail == -ENODEV) - next.tv_sec = 659; - else - next.tv_sec = 0; + /* + * The next call to sync_cmos_clock needs to have have a wall + * clock tv_nsec value equal to target_nsec. + */ + if (next.tv_nsec > target_nsec) + next.tv_sec++; + next.tv_nsec = target_nsec; - if (next.tv_nsec >= NSEC_PER_SEC) { - next.tv_sec++; - next.tv_nsec -= NSEC_PER_SEC; - } - queue_delayed_work(system_power_efficient_wq, - &sync_cmos_work, timespec64_to_jiffies(&next)); + /* + * Convert to a relative delay. If time set took a really long + * time, or the wall clock was changed, this might become + * negative, so try again. + */ + getnstimeofday64(&now); + delta = timespec64_sub(next, now); + } while (delta.tv_sec <= 0); + + queue_delayed_work(system_power_efficient_wq, &sync_cmos_work, + timespec64_to_jiffies(&delta)); } void ntp_notify_cmos_timer(void)