Messages in this thread |  | | Date | Sun, 31 May 2015 18:05:39 +0200 | From | Richard Cochran <> | Subject | Re: [RFC][PATCH 4/4] time: Do leapsecond adjustment in gettime fastpaths |
| |
On Fri, May 29, 2015 at 01:24:28PM -0700, John Stultz wrote: > Apologies to Richard Cochran, who pushed for such a change > years ago, which I resisted due to the concerns about the > performance overhead.
For the record, I got the idea from Michel Hack of IBM.
> While I suspect this isn't extremely critical, folks who > care about strict leap-second correctness will likely > want to watch this, and it will likely be a -stable candidate.
I think this is a step in the right direction. If the 'next_leap_sec' is made available to the vdso, then the 1-10 ms time error could also be prevented there.
I have some comments, but, as is, feel free to add my ack.
Acked-by: Richard Cochran <richardcochran@gmail.com>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c > index 472591e..6e15fbb 100644 > --- a/kernel/time/ntp.c > +++ b/kernel/time/ntp.c
> @@ -359,6 +364,33 @@ u64 ntp_tick_length(void) > return tick_length; > } > > +/** > + * get_leap_state - Returns the NTP leap state > + * @next_leap_sec: Next leapsecond in time64_t > + * @next_leap_ktime: Next leapsecond in ktime_t > + * > + * Provides NTP leapsecond state. Returns direction > + * of the leapsecond adjustment as an integer. > + */ > +int get_leap_state(time64_t *next_leap_sec, ktime_t *next_leap_ktime) > +{ > + int dir; > + > + if ((time_state == TIME_INS) && (time_status & STA_INS)) {
This can be reduced to just one test on (time_state == TIME_INS). If user spaces clears STA_INS, then you can immediately cancel the leap second.
> + dir = -1; > + *next_leap_sec = ntp_next_leap_sec; > + *next_leap_ktime = ktime_set(ntp_next_leap_sec, 0); > + } else if ((time_state == TIME_DEL) && (time_status & STA_DEL)) { > + dir = 1; > + *next_leap_sec = ntp_next_leap_sec; > + *next_leap_ktime = ktime_set(ntp_next_leap_sec, 0); > + } else { > + dir = 0; > + *next_leap_sec = TIME64_MAX; > + next_leap_ktime->tv64 = KTIME_MAX; > + } > + return dir; > +} > > /* > * this routine handles the overflow of the microsecond field
> @@ -382,15 +414,21 @@ int second_overflow(unsigned long secs) > */ > switch (time_state) { > case TIME_OK: > - if (time_status & STA_INS) > + if (time_status & STA_INS) {
The user sets STA_INS via adjtimex, but we don't change to TIME_INS until the next tick. Why not change immediately? Then this funtion would only need to check for TIME_INS && (secs % 86400 == 0) and the very unlikey TIME_DEL.
> time_state = TIME_INS; > - else if (time_status & STA_DEL) > + ntp_next_leap_sec = secs + SECS_PER_DAY - > + (secs % SECS_PER_DAY); > + } else if (time_status & STA_DEL) { > time_state = TIME_DEL; > + ntp_next_leap_sec = secs + SECS_PER_DAY - > + ((secs+1) % SECS_PER_DAY); > + } > break; > case TIME_INS: > - if (!(time_status & STA_INS)) > + if (!(time_status & STA_INS)) { > + ntp_next_leap_sec = TIME64_MAX; > time_state = TIME_OK; > - else if (secs % 86400 == 0) { > + } else if (secs % SECS_PER_DAY == 0) { > leap = -1; > time_state = TIME_OOP; > printk_deferred(KERN_NOTICE
> @@ -711,6 +752,24 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai) > if (!(time_status & STA_NANO)) > txc->time.tv_usec /= NSEC_PER_USEC; > > + /* Handle leapsec adjustments */
This block and its commnet rather confused me. What this code actually does is fix up the time value returned to the caller of adjtimex, but only in the 1-10 millisecond window before the leap second tick.
> + if (unlikely(ts->tv_sec >= ntp_next_leap_sec)) { > + if ((time_state == TIME_INS) && (time_status & STA_INS)) { > + result = TIME_OOP; > + txc->tai++; > + txc->time.tv_sec--; > + } > + if ((time_state == TIME_DEL) && (time_status & STA_DEL)) { > + result = TIME_WAIT; > + txc->tai--; > + txc->time.tv_sec++; > + } > + if ((time_state == TIME_OOP) && > + (ts->tv_sec == ntp_next_leap_sec)) { > + result = TIME_WAIT; > + } > + } > + > return result; > }
Thanks, Richard
|  |