Rationalize time keeping

Signed-off-by Richard Cochran<[email protected]>

---
  kernel/time/ntp.c |    6 ++----
  1 files changed, 2 insertions(+), 4 deletions(-)

kernel/time/ntp.c +2 -4

index f03fd83..d0a2183 100644
@@ -473,8 +473,6 @@ int second_overflow(unsigned long secs)
  							<<  NTP_SCALE_SHIFT;
  	time_adjust = 0;
 
-
-
  out:
  	spin_unlock_irqrestore(&ntp_lock, flags);
 

@@ -559,10 +557,10 @@ static inline void process_adj_status(struct timex *txc, struct timespec *ts)
  	/* only set allowed bits */
  	time_status&= STA_RONLY;
  	time_status |= txc->status&  ~STA_RONLY;
-
  }
+
  /*
- * Called with the xtime lock held, so we can access and modify
+ * Called with ntp_lock held, so we can access and modify
   * all the global NTP state:
   */
  static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts)

Signed-off-by Richard Cochran<[email protected]>

This patch adds a new internal interface to be used by the NTP code in order to set the next leap second event. Also, it adds a kernel command line option that can be used to dial the TAI - UTC offset at boot.

Signed-off-by Richard Cochran<[email protected]>

---
  kernel/time/leap-seconds.h |   23 ++++++
  kernel/time/timekeeping.c  |  175 ++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 198 insertions(+), 0 deletions(-)

create mode 100644 kernel/time/leap-seconds.h

kernel/time/timekeeping.c +0 -0

  	/*
  	 * wall_to_monotonic is what we need to add to xtime (or xtime corrected
  	 * for sub jiffie times) to get to monotonic time.  Monotonic is pegged

kernel/time/timekeeping.c struct timekeeper { · line 57

Quoted 2 lines · Richard Cochran

kernel/time/timekeeping.c +2 -0

+	/* The Kernel Time Scale (KTS) value of the next leap second. */
+	time_t next_leapsecond;

I'm not a big fan of this new Kernel Time Scale. I don't think we really need a new time domain, and it only muddles things. Why not have next_leapsecond be specified in CLOCK_MONOTONIC time?

kernel/time/timekeeping.c struct timekeeper { · line 61

Quoted 4 lines

kernel/time/timekeeping.c +4 -0

+	/* The current difference KTS - UTC. */
+	int kts_utc_offset;
+	/* The current difference TAI - KTS. */
+	int tai_kts_offset;

Again, get rid of KTS and we can simplify these as:
CLOCK_TAI = CLOCK_MONOTONIC + mono_to_tai; UTC/CLOCK_REALTIME = CLOCK_TAI + utc_offset;

This basically requires changing the timekeeping core from keeping track of CLOCK_REALTIME as its time base, and instead having it use CLOCK_MONOTONIC. This actually is something I proposed a long time ago, but was deferred because folks were concerned that the extra addition would slow gettimeofday() down. However, that was back when everything internally used jiffies. These days ktime_get() is probably called internally more frequently, and so optimizing for CLOCK_MONOTONIC makes sense.

kernel/time/timekeeping.c struct timekeeper { · line 65

Quoted 4 lines

kernel/time/timekeeping.c +4 -0

+#ifdef CONFIG_DELETE_LEAP_SECONDS
+	/* Remembers whether to insert or to delete. */
+	int insert_leapsecond;
+#endif

I'm not a big fan of this additional config option. The maintenance burden for the extra condition is probably not worth the code size trade-off. Or it needs way more justification.

kernel/time/timekeeping.c __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock); · line 109

Quoted 7 lines

kernel/time/timekeeping.c +7 -0

+static int __init tai_offset_setup(char *str)
+{
+	get_option(&str,&timekeeper.kts_utc_offset);
+	return 1;
+}
+__setup("tai_offset=", tai_offset_setup);
+

Oooof.. Yuck. I really don't like the boot time tai_offset argument. What sysadmin wants to change their grub settings after a leapsecond so that the next reboot its set properly?

I'd suggest tai_offset be set to zero until userland updates it at boot time. CLOCK_TAI is not CLOCK_MONOTONIC, and it can jump around if the user calls settimeofday(), so I don't see a major reason for it to be initialized via a boot argument.

Its true that right now there's no userland infrastructure to set it (other then ntp, but I've never verified it actually gets set), but there also aren't any users consuming it either.

kernel/time/timekeeping.c struct timekeeper { · line 65

Quoted 4 lines · Richard Cochran

kernel/time/timekeeping.c +4 -0

+#ifdef CONFIG_DELETE_LEAP_SECONDS
+	/* Remembers whether to insert or to delete. */
+	int insert_leapsecond;
+#endif

I'm not a big fan of this additional config option. The maintenance burden for the extra condition is probably not worth the code size trade-off. Or it needs way more justification.

Quoted 9 lines · John Stultz

kernel/time/timekeeping.c struct timekeeper { · line 65

Quoted 4 lines · Richard Cochran

kernel/time/timekeeping.c +4 -0

+#ifdef CONFIG_DELETE_LEAP_SECONDS
+	/* Remembers whether to insert or to delete. */
+	int insert_leapsecond;
+#endif

I'm not a big fan of this additional config option. The maintenance burden for the extra condition is probably not worth the code size trade-off. Or it needs way more justification.

Out of curiosity, I looked at ntp-4.2.6p5 to see if they really support deleting leap seconds or not. Even though the code appears to try and support them, I spotted a few bugs. There is a hard coded assumption that the TAI offset is increasing.

This is just the reason why I suggest not supporting deletions (or only conditionally for nit pickers). You can code it up, but it will be in vain, since the code will never be tested or used in practice. Code that is never executed is a true mainenance burden by definition.

This patch converts the core time keeping code to use a continuous timescale called the Kernel Time Scale (KTS). KTS is based on the TAI timescale but can be offset from it by an integral number of seconds. Every function that returns UTC time now coverts the seconds by adding the current KTS - UTC offset.

As a result of this change, the NTP leap second code is considerably simplified and hopefully more robust.

Signed-off-by Richard Cochran <[email protected]>

---
 include/linux/timex.h     |    2 +-
 kernel/time/ntp.c         |   81 ++++++++++----------------------------------
 kernel/time/timekeeping.c |   73 ++++++++++++++++++++++++++++++++--------
 3 files changed, 79 insertions(+), 77 deletions(-)

include/linux/timex.h +1 -1

index 99bc88b..9461e6f 100644
@@ -252,7 +252,7 @@ extern void ntp_clear(void);
 /* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
 extern u64 ntp_tick_length(void);
 
-extern int second_overflow(unsigned long secs);
+void second_overflow(void);
 extern int do_adjtimex(struct timex *);
 extern void hardpps(const struct timespec *, const struct timespec *);
 

kernel/time/ntp.c +8 -57

index d0a2183..91de2f8 100644
@@ -16,6 +16,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 
+#include "leap-seconds.h"
 #include "tick-internal.h"
 
 /*

@@ -24,6 +25,7 @@
 
 DEFINE_SPINLOCK(ntp_lock);
 
+#define STA_LEAP		(STA_INS | STA_DEL)
 
 /* USER_HZ period (usecs): */
 unsigned long			tick_usec = TICK_USEC;

@@ -42,19 +44,9 @@ static u64			tick_length_base;
  * phase-lock loop variables
  */
 
-/*
- * clock synchronization status
- *
- * (TIME_ERROR prevents overwriting the CMOS clock)
- */
-static int			time_state = TIME_OK;
-
 /* clock status bits:							*/
 static int			time_status = STA_UNSYNC;
 
-/* TAI offset (secs):							*/
-static long			time_tai;
-
 /* time adjustment (nsecs):						*/
 static s64			time_offset;
 

@@ -386,57 +378,14 @@ u64 ntp_tick_length(void)
  * They were originally developed for SUN and DEC kernels.
  * All the kudos should go to Dave for this stuff.
  *
- * Also handles leap second processing, and returns leap offset
  */
-int second_overflow(unsigned long secs)
+void second_overflow(void)
 {
 	s64 delta;
-	int leap = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&ntp_lock, flags);
 
-	/*
-	 * Leap second processing. If in leap-insert state at the end of the
-	 * day, the system clock is set back one second; if in leap-delete
-	 * state, the system clock is set ahead one second.
-	 */
-	switch (time_state) {
-	case TIME_OK:
-		if (time_status & STA_INS)
-			time_state = TIME_INS;
-		else if (time_status & STA_DEL)
-			time_state = TIME_DEL;
-		break;
-	case TIME_INS:
-		if (secs % 86400 == 0) {
-			leap = -1;
-			time_state = TIME_OOP;
-			printk(KERN_NOTICE
-				"Clock: inserting leap second 23:59:60 UTC\n");
-		}
-		break;
-	case TIME_DEL:
-		if ((secs + 1) % 86400 == 0) {
-			leap = 1;
-			time_tai--;
-			time_state = TIME_WAIT;
-			printk(KERN_NOTICE
-				"Clock: deleting leap second 23:59:59 UTC\n");
-		}
-		break;
-	case TIME_OOP:
-		time_tai++;
-		time_state = TIME_WAIT;
-		break;
-
-	case TIME_WAIT:
-		if (!(time_status & (STA_INS | STA_DEL)))
-			time_state = TIME_OK;
-		break;
-	}
-
-
 	/* Bump the maxerror field */
 	time_maxerror += MAXFREQ / NSEC_PER_USEC;
 	if (time_maxerror > NTP_PHASE_LIMIT) {

@@ -475,8 +424,6 @@ int second_overflow(unsigned long secs)
 
 out:
 	spin_unlock_irqrestore(&ntp_lock, flags);
-
-	return leap;
 }
 
 #ifdef CONFIG_GENERIC_CMOS_UPDATE

@@ -541,7 +488,6 @@ static inline void notify_cmos_timer(void) { }
 static inline void process_adj_status(struct timex *txc, struct timespec *ts)
 {
 	if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
-		time_state = TIME_OK;
 		time_status = STA_UNSYNC;
 		/* restart PPS frequency calibration */
 		pps_reset_freq_interval();

@@ -554,6 +500,18 @@ static inline void process_adj_status(struct timex *txc, struct timespec *ts)
 	if (!(time_status & STA_PLL) && (txc->status & STA_PLL))
 		time_reftime = get_seconds();
 
+	/*
+	 * Check for new leap second commands.
+	 */
+	if (!(time_status & STA_INS) && (txc->status & STA_INS))
+		timekeeper_insert_leap_second();

Just in time for this year's leap second, this patch series presents a solution for the UTC leap second mess.

Of course, the POSIX UTC system is broken by design, and the Linux kernel cannot fix that. However, what we can do is correctly execute leap seconds and always report the time variables (UTC time, TAI offset, and leap second status) with consistency.

The basic idea is to keep the internal time using a continuous timescale and to convert to UTC by testing the time value against the current threshold and adding the appropriate offset. Since the UTC time and the leap second status is provided on demand, this eliminates the need to set a timer or to constantly monitor for leap seconds, as was done up until now.

Patches 2 and 3 are just trivial stuff I saw along the way.

* Benefits
- Fixes the buggy, inconsistent time reporting surrounding a leap second event.

- Opens the possibility of offering a rational time source to user space. [ Trivial to offer clock_gettime(CLOCK_TAI) for example. ]

* Performance Impacts
** con
- Small extra cost when reading the time (one integer addition plus one integer test).

** pro
- Removes repetitive, periodic division (secs % 86400 == 0) the whole day long preceding a leap second.
- Cost of maintaining leap second status goes to the user of the NTP adjtimex() interface, if any.

* Todo
- The function __current_kernel_time accesses the time variables without taking the lock. I can't figure that out.

Quoted 3 lines · Richard Cochran

* Benefits
- Fixes the buggy, inconsistent time reporting surrounding a leap second event.

Just to clarify this, so we've got the right scope on the problem, you're trying to address the fact that the leap second is not actually applied until the tick after the leap second, correct?

Where basically you can see small offsets like:

And you're proposing we fix this by changing the leap-second processing from only being done at tick-time (which isn't exactly on the second boundary)to being calculated for each getnstimeofday, correct?

Quoted 2 lines

- Opens the possibility of offering a rational time source to user space. [ Trivial to offer clock_gettime(CLOCK_TAI) for example. ]

CLOCK_TAI is something I'd like to have.

My only concern is how we
manage it along with possible smeared-leap-seconds ala:
http://googleblog.blogspot.com/2011/09/time-technology-and-leaping-seconds.html

( I shudder at the idea of managing two separate frequency corrections for different time domains).

Quoted 4 lines

* Performance Impacts
** con
- Small extra cost when reading the time (one integer addition plus one integer test).

This may not be so small when it comes to folks who are very concerned about the clock_gettime hotpath.

Further, the correction will be needed to be made in the vsyscall paths, which isn't done with your current patchset (causing userland to see different time values then what kernel space calculates).

One possible thing to consider? Since the TIME_OOP flag is only visible via the adjtimex() interface, maybe it alone should have the extra overhead of the conditional?

I'm not excited about the
gettimeofday field returned by adjtimex not matching what gettimeofday actually provides for that single-tick interval, but maybe its a reasonable middle ground?

Quoted 5 lines

** pro
- Removes repetitive, periodic division (secs % 86400 == 0) the whole day long preceding a leap second.
- Cost of maintaining leap second status goes to the user of the NTP adjtimex() interface, if any.

Not sure I follow this last point. How are we pushing this maintenance to adjtimex() users?

Quoted 4 lines

* Todo
- The function __current_kernel_time accesses the time variables without taking the lock. I can't figure that out.

There's a few cases where we want the current second value when we already hold the xtime_lock, or we might possibly hold the xtime_lock. Its an special internal interface for special users (update_vsyscall, for example).

Quoted 4 lines · John Stultz

Quoted 3 lines · Richard Cochran

* Benefits
- Fixes the buggy, inconsistent time reporting surrounding a leap second event.

Quoted 3 lines

Just to clarify this, so we've got the right scope on the problem, you're trying to address the fact that the leap second is not actually applied until the tick after the leap second, correct?

That is one problem, yes.

Quoted 1 line

Where basically you can see small offsets like:

I can synchronize over the network to under 100 nanoseconds, so to me, one second is a large offset.

Quoted 6 lines

My only concern is how we
manage it along with possible smeared-leap-seconds ala:
http://googleblog.blogspot.com/2011/09/time-technology-and-leaping-seconds.html

( I shudder at the idea of managing two separate frequency corrections for different time domains).

Are you planning to implement that? This approach is by no means universally accepted.

In my view, what Google is doing is hack (albeit a sensible for business applications). For test and measurement or scientific applications, it does not make sense to introduce artifical frequency errors in this way.

Another variant of this idea: http://www.cl.cam.ac.uk/~mgk25/time/utc-sls/

Here is a nice quote from that page:

All other objections to UTC-SLS that I heard were not directed against its specific design choices, but against the (very well established) practice of using UTC at all in the applications that this proposal targets:

* Some people argue that operating system interfaces, such as the POSIX "seconds since the epoch" scale used in time_t APIs, should be changed from being an encoding of UTC to being an encoding of the leap-second free TAI timescale.

* Some people want to go even further and abandon UTC and leap seconds entirely, detach all civilian time zones from the rotation of Earth, and redefine them purely based on atomic time.

While these people are usually happy to agree that UTC-SLS is a sensible engineering solution as long as UTC remains the main time basis of distributed computing, they argue that this is just a workaround that will be obsolete once their grand vision of giving up UTC entirely has become true, and that it is therefore just an unwelcome distraction from their ultimate goal.

Until the whole world agrees to this "work around" I think we should stick to current standards. If and when this practice becomes standardized (I'm not holding my breath), then we could simply drop the internal difference between the kernel time scale and UTC, and steer out the leap second synchronously with the rest of the world.

Quoted 6 lines

Quoted 4 lines

* Performance Impacts
** con
- Small extra cost when reading the time (one integer addition plus one integer test).

This may not be so small when it comes to folks who are very concerned about the clock_gettime hotpath.

If you would support the option to only insert leap seconds, then the cost is one integer addition and one integer test.

Also, once we have a rational time interface (like CLOCK_TAI), then time sensitive will want to use that instead anyhow.

Quoted 3 lines

Further, the correction will be needed to be made in the vsyscall paths, which isn't done with your current patchset (causing userland to see different time values then what kernel space calculates).

Do you mean __current_kernel_time? What did I miss?

Quoted 3 lines

One possible thing to consider? Since the TIME_OOP flag is only visible via the adjtimex() interface, maybe it alone should have the extra overhead of the conditional?

This would mean that you would have to do the conditional somehow backwards in order to provide TAI time values. To me, the logical way is to keep a continuous time scale, and then compute UTC from it.

Quoted 4 lines

I'm not excited about the
gettimeofday field returned by adjtimex not matching what gettimeofday actually provides for that single-tick interval, but maybe its a reasonable middle ground?

Not sure what you mean, but to me it is not acceptable to deliver inconsistent time values to userspace!

Quoted 7 lines

Quoted 5 lines

** pro
- Removes repetitive, periodic division (secs % 86400 == 0) the whole day long preceding a leap second.
- Cost of maintaining leap second status goes to the user of the NTP adjtimex() interface, if any.

Not sure I follow this last point. How are we pushing this maintenance to adjtimex() users?

Only adjtimex calls timekeeper_gettod_status, where the leap second is calculated, outside of timekeeper.lock, on the NTP user space's kernel time.

In current Linux, the modulus is done in update_wall_time and logarithmic_accumulation, on kernel time.

Quoted 4 lines

Quoted 4 lines

* Todo
- The function __current_kernel_time accesses the time variables without taking the lock. I can't figure that out.

Quoted 4 lines

There's a few cases where we want the current second value when we already hold the xtime_lock, or we might possibly hold the xtime_lock. Its an special internal interface for special users (update_vsyscall, for example).

What about kdb_summary?

Quoted 2 lines · Richard Cochran

I can synchronize over the network to under 100 nanoseconds, so to me, one second is a large offset.

Well, the leap-offset is a second, but when it arrives is only tick-accurate. :)

True, although even if it is a hack, google *is* using it. My concern is that if CLOCK_REALTIME is smeared to avoid a leap second jump, in that environment we cannot also accurate provide a correct CLOCK_TAI. So far that's not been a problem, because CLOCK_TAI isn't a clockid we yet support. But the expectations bar always rises, so I suspect once we have a CLOCK_TAI, someone will want us to handle smeared-leap seconds without affecting CLOCK_TAI's correctness.

Quoted 6 lines

While these people are usually happy to agree that UTC-SLS is a sensible engineering solution as long as UTC remains the main time basis of distributed computing, they argue that this is just a workaround that will be obsolete once their grand vision of giving up UTC entirely has become true, and that it is therefore just an unwelcome distraction from their ultimate goal.

I think this last point is very telling. Neither of the above options are really viable in my mind, as I don't see any real consensus to giving up UTC. What is in-practice is actually way more important then where folks wish things would go.

Well, I think that Google shows some folks are starting to use workarounds like smeared-leap-seconds/UTC-SLS. So its something we should watch carefully and expect more folks to follow.

Its true
that you don't want to mix UTC-SLS and standard UTC time domains, but its likely this will be a site-specific configuration.

So its a concern when a correct CLOCK_TAI would be incompatible on systems using these hacks/workarounds.

*Any* extra work is a big deal to folks who are sensitive to clock_gettime performance.
That said, I don't see why its more complicated to also handle leap removal?

Well, performance sensitive and correctness sensitive are two different things. :) I think CLOCK_TAI is much cleaner for things, but at the same time, the world "thinks" in UTC, and converting between them isn't always trivial (very similar to the timezone presentation layer, which isn't fun). So I'd temper any hopes of mass conversion. :)

Since this is done in
different ways for each architecture, you need to export the proper information out via update_vsyscall() and also update the arch-specific vsyscall gettimeofday paths (which is non-trivial, as some arches are implemented in asm, etc - my sympathies here, its a pain).

For users of clock_gettime/gettimeofday, a leapsecond is an inconsistency. Neither interfaces provide a way to detect that the TIME_OOP flag is set and its not 23:59:59 again, but 23:59:60 (which can't be represented by a time_t). Thus even if the behavior was perfect, and the leapsecond landed at exactly the second edge, it is still a time hiccup to most applications anyway.

Thus, most of userland doesn't really care if the hiccup happens up to a tick after the second's edge. They don't expect it anyway. So they really don't want a constant performance drop in order for the hiccup to be more "correct" when it happens. :)

That's why I'm suggesting that you consider starting by modifying the adjtimex() interface. Any application that actually cares about leapseconds should be using adjtimex() since its the only interface that allows you to realize that's whats happening. Its not a performance optimized path, and so its a fine candidate for being slow-but-correct.

My only concern there is that it would cause problems when mixing adjtimex() calls with clock_gettime() calls, because you could have a tick-length of time when they report different time values. But this may be acceptable.

Quoted 3 lines · John Stultz

Quoted 2 lines · Richard Cochran

I can synchronize over the network to under 100 nanoseconds, so to me, one second is a large offset.

Quoted 2 lines

Well, the leap-offset is a second, but when it arrives is only tick-accurate. :)

It would be fine to change the leap second status on the tick, but then you must also change the time then, and only then, as well. I know Linux moved away from this long ago, and the new way is better, but still what the kernel does today is just plain wrong.

But there is a fix. I just offered it.

Quoted 8 lines

True, although even if it is a hack, google *is* using it. My concern is that if CLOCK_REALTIME is smeared to avoid a leap second jump, in that environment we cannot also accurate provide a correct CLOCK_TAI. So far that's not been a problem, because CLOCK_TAI isn't a clockid we yet support. But the expectations bar always rises, so I suspect once we have a CLOCK_TAI, someone will want us to handle smeared-leap seconds without affecting CLOCK_TAI's correctness.

It is either/or, but not both simultaneously.

My proposal does not prevent the smear method in any way. People who want the smear just never schedule a leap second. People who want the frequency constant just use the TAI clock interface for the important work.

We really don't have to support both ways at once.

Quoted 4 lines

*Any* extra work is a big deal to folks who are sensitive to clock_gettime performance.
That said, I don't see why its more complicated to also handle leap removal?

It makes your kernel image larger with no added benefit.

Quoted 11 lines

For users of clock_gettime/gettimeofday, a leapsecond is an inconsistency. Neither interfaces provide a way to detect that the TIME_OOP flag is set and its not 23:59:59 again, but 23:59:60 (which can't be represented by a time_t). Thus even if the behavior was perfect, and the leapsecond landed at exactly the second edge, it is still a time hiccup to most applications anyway.

Thus, most of userland doesn't really care if the hiccup happens up to a tick after the second's edge. They don't expect it anyway. So they really don't want a constant performance drop in order for the hiccup to be more "correct" when it happens. :)

I don't buy that argument. Repeating a time_t value leads to ambiguous UTC times, put it is posixly correct. The values are usable together with difftime(3). Having the time_t go forward and then back again is certainly worse.

If we leave everything as is, then the user is left with two choices for data collection applications.

1. Turn off your data system on the night of a leap second.

2. Record data even during a leap second, but post process the files to fix up all the uglies.

Either way, the kernel has failed us.

Quoted 7 lines

That's why I'm suggesting that you consider starting by modifying the adjtimex() interface. Any application that actually cares about leapseconds should be using adjtimex() since its the only interface that allows you to realize that's whats happening. Its not a performance optimized path, and so its a fine candidate for being slow-but-correct.

Quoted 4 lines

My only concern there is that it would cause problems when mixing adjtimex() calls with clock_gettime() calls, because you could have a tick-length of time when they report different time values. But this may be acceptable.

(Introduce yet another kernel bug? No, thanks ;)

Quoted 2 lines · Richard Cochran

Quoted 1 line · John Stultz

Well, the leap-offset is a second, but when it arrives is only

Quoted 1 line

Quoted 1 line

tick-accurate. :)

Quoted 6 lines

It would be fine to change the leap second status on the tick, but then you must also change the time then, and only then, as well. I know Linux moved away from this long ago, and the new way is better, but still what the kernel does today is just plain wrong.

But there is a fix. I just offered it.

Maybe could you clarify a bit more here about how the kernel is plain wrong? Try a clear problem description including examples (before and after you changes)? I'm worried we're talking about different problems.

Quoted 9 lines

If we leave everything as is, then the user is left with two choices for data collection applications.

1. Turn off your data system on the night of a leap second.

2. Record data even during a leap second, but post process the files to fix up all the uglies.

Either way, the kernel has failed us.

3. Use adjtimex() and interpret the timespec and time_state return together to interpret 23:59:60 properly?

4. Use adjtimex(), and use the timespec + the time_tai offset value to calculate TAI time?

I dunno. Again, I suspect we're thinking about different issues that sound very similar. :)

You did just suggest we allow for CLOCK_TAI to be broken for folks want to use smeared-leap-seconds. That sounds like an eventual bug too. :) Regardless, the point of my suggestion is that you're likely going to be adding logic to a very hot path, and the resulting benefit has not been clearly stated.

Expect push back
there (not just from me), so have a strong argument and show overhead impact with numbers to help your case. Alternatively look for other solutions that don't affect the hot-path (like what I suggested w/ adjtimex() above - although that may have downsides too).

Do forgive me for prodding you here. Assuming I understand your goals (adding CLOCK_TAI, reworking timekeeping core to keep construct time in a more sane way, and improved leapsecond logic), I very much want to see them come to be. I appreciate your focus on solving some of the complex and unloved issues here.

I look forward to your next revision!

No. So, on architectures that support vsyscalls/vdso (x86_64, powerpc, ia64, and maybe a few others) getnstimeofday() is really only an internal interface for in-kernel access. Userland uses the vsyscall/vdso interface to be able to read the time completely from userland context (with no syscall overhead). Since this is done in different ways for each architecture, you need to export the proper information out via update_vsyscall() and also update the arch-specific vsyscall gettimeofday paths (which is non-trivial, as some arches are implemented in asm, etc - my sympathies here, its a pain).

Quoted 10 lines · John Stultz

No. So, on architectures that support vsyscalls/vdso (x86_64, powerpc, ia64, and maybe a few others) getnstimeofday() is really only an internal interface for in-kernel access. Userland uses the vsyscall/vdso interface to be able to read the time completely from userland context (with no syscall overhead). Since this is done in different ways for each architecture, you need to export the proper information out via update_vsyscall() and also update the arch-specific vsyscall gettimeofday paths (which is non-trivial, as some arches are implemented in asm, etc - my sympathies here, its a pain).

Okay, so now I understand the vDSO page thingy. Help me please to understand exactly which architectures would need changes for my proposal.

The only archs exporting time variables/functions through vDSO are those which define CONFIG_GENERIC_TIME_VSYSCALL=y, namely:

- ia64
- powerpc 64 and 32 bit
- s390 64 and 32 bit
- x86 64 bit only **

** But 32 guest running in a 64 host also has time in the vDSO?!?

Did I get that right?

Quoted 4 lines · Richard Cochran

* Performance Impacts
** con
- Small extra cost when reading the time (one integer addition plus one integer test).

This may not be so small when it comes to folks who are very concerned about the clock_gettime hotpath.
Further, the correction will be needed to be made in the vsyscall paths, which isn't done with your current patchset (causing userland

to see different time values then what kernel space calculates).

Quoted 9 lines · John Stultz

Quoted 4 lines · Richard Cochran

* Performance Impacts
** con
- Small extra cost when reading the time (one integer addition plus one integer test).

This may not be so small when it comes to folks who are very concerned about the clock_gettime hotpath.
Further, the correction will be needed to be made in the vsyscall paths, which isn't done with your current patchset (causing userland

          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

to see different time values then what kernel space calculates).

John, now that you clarified the vDSO thing, I am very confused about this statement of yours. It appears that the vDSO data are updated when timekeeping_update() in timekeeper.c calls update_vsyscall().

I think the hunk from patch #5, below, does in fact adjust the time value correctly before it gets handed off to the arch-specific update_vsyscall() to be copied into the vDSO page. So I'll make the claim that:

1. We don't have to touch the vsyscall paths for this.
2. This change does not affect vDSO performance at all.

But the changes you make to getnstimeofday() still needs to happen in the vDSO code. The vDSO code basically implements getnstimeofday() in userland.

If you're code is trying to make it so that the leap-second is properly handled at the second boundary instead of the tick boundary, there must me some change needed to the vDSO, since the vDSO code is updated only each tick. Otherwise how can you enforce the leap after the second boundary but before the tick?

Quoted 9 lines · John Stultz

But the changes you make to getnstimeofday() still needs to happen in the vDSO code. The vDSO code basically implements getnstimeofday() in userland.

If you're code is trying to make it so that the leap-second is properly handled at the second boundary instead of the tick boundary, there must me some change needed to the vDSO, since the vDSO code is updated only each tick. Otherwise how can you enforce the leap after the second boundary but before the tick?

Yeah, so the vDSO does the sub-tick interpolation, and this can easily miss an inserted leap second for a while (just like the current code).

So, this patch series as it stands improves the users of the traditional syscalls without hurting the superduper vDSO performance at all. The vDSO leap second time errors are not fixed, but they are also no worse than today, either.

I am try to say that, even if there is resistance to adding code in the vDSO path for reasons of performance, that doesn't necessarily mean that we cannot fix the leap second for the tradition syscall case.

Just in time for this year's leap second, this patch series presents a solution for the UTC leap second mess.

Of course, the POSIX UTC system is broken by design, and the Linux kernel cannot fix that. However, what we can do is correctly execute leap seconds and always report the time variables (UTC time, TAI offset, and leap second status) with consistency.

The basic idea is to keep the internal time using a continuous timescale and to convert to UTC by testing the time value against the current threshold and adding the appropriate offset. Since the UTC time and the leap second status is provided on demand, this eliminates the need to set a timer or to constantly monitor for leap seconds, as was done up until now.

Quoted 7 lines · Richard Cochran

The basic idea is to keep the internal time using a continuous timescale and to convert to UTC by testing the time value against the current threshold and adding the appropriate offset. Since the UTC time and the leap second status is provided on demand, this eliminates the need to set a timer or to constantly monitor for leap seconds, as was done up until now.

So as I mentioned in my earlier review of this patch set, I'm not as excited about the meta-layer you added in utc-tai.h.

http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/clktai

The untested patch set there basically pushes TAI time management into the timekeeping core, and then exports a CLOCK_TAI clockid.

This patch set *does not* address the tick-granularity delayed leap-second processing issue that your patch also addresses. But I feel like the basic handling of tai is a little simpler.

Take a look at it and let me know what you think.

Quoted 7 lines · John Stultz

Quoted 6 lines · Richard Cochran

The basic idea is to keep the internal time using a continuous timescale and to convert to UTC by testing the time value against the current threshold and adding the appropriate offset. Since the UTC time and the leap second status is provided on demand, this eliminates the need to set a timer or to constantly monitor for leap seconds, as was done up until now.

Quoted 2 lines

So as I mentioned in my earlier review of this patch set, I'm not as excited about the meta-layer you added in utc-tai.h.

It really isn't a layer. It is just a pair of static inline helper functions. The code could just as well be internal to timekeeper.c, but I put it in its own file so that I could include it into a unit test program.

Quoted 8 lines

http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/clktai

The untested patch set there basically pushes TAI time management into the timekeeping core, and then exports a CLOCK_TAI clockid.

This patch set *does not* address the tick-granularity delayed leap-second processing issue that your patch also addresses. But I feel like the basic handling of tai is a little simpler.

So, is this a "won't fix" issue, in your view?

(If so, I'll drop it.)

Quoted 1 line

Take a look at it and let me know what you think.

It looks okay to me, as far as it goes.

At least you offer a clock that doesn't jump around. With your patches (and the TAI offset fix before), user space programs sensitive to the leap second uglies would have a reasonable option. Data collection programs can read and store TAI time stamps, and these can always be converted into UTC using a table method.

But do you think it will be possible to implement the following in the same way?

- clock_settime(CLOCK_TAI)
- clock_adjtime(CLOCK_TAI)

- timer_create(CLOCK_TAI)