ntp: Correct TAI offset during leap second
Subsystems (2)
PATCH 1/1 ntp: Correct TAI offset during leap second
Hey Greg,
Forgot to cc stable on this one. Thanks to Dave Jones for noticing!
-john
------------------
From: Richard Cochran<[email protected]>
commit dd48d708ff3e917f6d6b6c2b696c3f18c019feed upstream.
When repeating a UTC time value during a leap second (when the UTC
time should be 23:59:60), the TAI timescale should not stop. The kernel
NTP code increments the TAI offset one second too late. This patch fixes
the issue by incrementing the offset during the leap second itself.
---
kernel/time/ntp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/ntp.c +0 -0
index f03fd83..e8c8671 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -412,6 +412,7 @@ int second_overflow(unsigned long secs)
if (secs % 86400 == 0) {
leap = -1;
time_state = TIME_OOP;
+ time_tai++;
printk(KERN_NOTICE
"Clock: inserting leap second 23:59:60 UTC\n");
}
@@ -426,7 +427,6 @@ int second_overflow(unsigned long secs)
}
break;
case TIME_OOP:
- time_tai++;
time_state = TIME_WAIT;
break;
Signature · 1 line
1.7.9.5
Quoted 3 lines · John Stultz
Hey Greg,
Forgot to cc stable on this one. Thanks to Dave Jones for noticing!
-john
Gah. Thunderbird's preformat wrecked that one. Sorry. Here it is again.
-john
------------------
From: Richard Cochran<[email protected]>
commit dd48d708ff3e917f6d6b6c2b696c3f18c019feed upstream.
When repeating a UTC time value during a leap second (when the UTC time should be 23:59:60), the TAI timescale should not stop. The kernel NTP code increments the TAI offset one second too late. This patch fixes the issue by incrementing the offset during the leap second itself.
--- kernel/time/ntp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/ntp.c +1 -1
@@ -412,6 +412,7 @@ int second_overflow(unsigned long secs)if (secs %86400 ==0 ) { leap =-1 ; time_state =TIME_OOP ; + time_tai++;printk (KERN_NOTICE "Clock: inserting leap second 23:59:60 UTC\n" ); }
@@ -426,7 +427,6 @@ int second_overflow(unsigned long secs) }break ;case TIME_OOP : - time_tai++; time_state =TIME_WAIT ;break ;
Signature · 1 line
1.7.9.5
Quoted 10 lines · John Stultz
Hey Greg,
Forgot to cc stable on this one. Thanks to Dave Jones for noticing!
-john------------------
From: Richard Cochran<[email protected]>
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.
Quoted 7 lines
When repeating a UTC time value during a leap second (when the UTC
time should be 23:59:60), the TAI timescale should not stop. The kernel
NTP code increments the TAI offset one second too late. This patch fixes
the issue by incrementing the offset during the leap second itself.
[...]
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
Ben.
Signature · 2 lines
Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once.
Quoted 3 lines · Ben Hutchings
Quoted 1 line · John Stultz
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.
[...]
Quoted 2 lines
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
6b43ae8a619d (ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends on bd3312681f69 (ntp: Add ntp_lock to replace xtime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends on ea7cf49a7633).
John, is that bug present in 3.2.y and 3.0.y, too? Any hints for fixing it?
Thanks,
Jonathan
Quoted 8 lines · Jonathan Nieder
Quoted 3 lines · Ben Hutchings
Quoted 1 line · John Stultz
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.[...]
Quoted 2 lines
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
Quoted 7 lines
6b43ae8a619d(ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends onbd3312681f69(ntp: Addntp_lockto replacextime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends onea7cf49a7633).John, is that bug present in 3.2.y and 3.0.y, too? Any hints for fixing it?
It looks like incrementing the TAI offset was wrong even before
6b43ae8a ntp: Fix leap-second hrtimer livelock v3.4-rc1~44^2~9
The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
Thanks,
Richard
---
kernel/time/ntp.c +1 -1
@@ -365,6 +365,7 @@ static enum hrtimer_restart ntp_leap_second(struct hrtimer *timer)break ;case TIME_INS :timekeeping_leap_insert (-1 ); + time_tai++; time_state =TIME_OOP ;printk (KERN_NOTICE "Clock: inserting leap second 23:59:60 UTC\n" );
@@ -379,7 +380,6 @@ static enum hrtimer_restart ntp_leap_second(struct hrtimer *timer) "Clock: deleting leap second 23:59:59 UTC\n");break ;case TIME_OOP : - time_tai++; time_state =TIME_WAIT ;/* fall through */ case TIME_WAIT :
Quoted 5 lines · Richard Cochran
Quoted 4 lines · Jonathan Nieder
Quoted 3 lines · Ben Hutchings
Quoted 1 line · John Stultz
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.
Quoted 8 lines
Quoted 8 lines
[...]
Quoted 2 lines
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
6b43ae8a619d(ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends onbd3312681f69(ntp: Addntp_lockto replacextime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends onea7cf49a7633).
If I understand the commit message for 6b43ae8a619d correctly, the livelock results from ntp_lock and xtime_lock being acquired in opposite orders in two threads. Which means it wasn't possible before ntp_lock was introduced in bd3312681f69.
Quoted 9 lines
Quoted 2 lines
John, is that bug present in 3.2.y and 3.0.y, too? Any hints for fixing it?
It looks like incrementing the TAI offset was wrong even before
6b43ae8a ntp: Fix leap-second hrtimer livelock v3.4-rc1~44^2~9The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by adjtimex() during an inserted second; is that right?
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?
Ben.
Signature · 2 lines
Ben Hutchings If more than one person is responsible for a bug, no one is at fault.
Quoted 7 lines · Ben Hutchings
Quoted 2 lines · Richard Cochran
The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by
adjtimex()during an inserted second; is that right?
Right, nothing really terrible will happen. The worst that I can imagine is that ntpd will set the new TAI offset during OOP, and then the kernel will add one to it, resulting in the TAI offset being off by one.
But I really doubt any software makes use of this information.
Quoted 4 lines
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?
Well, if you want to wait all night then that is one way to do it. Here is a little test program I have been using:
https://github.com/richardcochran/leap
Thanks,
Richard
Quoted 14 lines · Richard Cochran
Quoted 7 lines · Ben Hutchings
Quoted 2 lines · Richard Cochran
The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by
adjtimex()during an inserted second; is that right?Right, nothing really terrible will happen. The worst that I can imagine is that ntpd will set the new TAI offset during OOP, and then the kernel will add one to it, resulting in the TAI offset being off by one.
Quoted 8 lines
But I really doubt any software makes use of this information.
Quoted 4 lines
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?Well, if you want to wait all night then that is one way to do it.
I was intending to change the clock too...
Quoted 3 lines
Here is a little test program I have been using:
Thanks, that runs without incident but does show the incorrect offset during OOP.
Ben.
Signature · 2 lines
Ben Hutchings If more than one person is responsible for a bug, no one is at fault.
Quoted 20 lines · Ben Hutchings
Quoted 19 lines · Richard Cochran
Quoted 7 lines · Ben Hutchings
Quoted 2 lines · Richard Cochran
The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by
adjtimex()during an inserted second; is that right?Right, nothing really terrible will happen. The worst that I can imagine is that ntpd will set the new TAI offset during OOP, and then the kernel will add one to it, resulting in the TAI offset being off by one.
But I really doubt any software makes use of this information.
Quoted 4 lines
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?
Quoted 8 lines
Quoted 1 line
Well, if you want to wait all night then that is one way to do it.
I was intending to change the clock too...
Quoted 3 lines
Here is a little test program I have been using:
Thanks, that runs without incident but does show the incorrect offset during OOP.
Yep. That's a long-standing issue, due to the leap-second processing happening at tick time (further complicated since to handle NOHZ, we accumulate in fixed-tick intervals that don't exactly line up with the interrupt edge), so we'll usually get one to two ticks into the second before we apply the leap second.
Richard has recently taken a stab at correcting this.
Richard, are you planning on taking another go at those changes? Or were my last suggestions just too daft? ;)
thanks
-john
Quoted 3 lines · John Stultz
Richard, are you planning on taking another go at those changes? Or were my last suggestions just too daft? ;)
I wonder if it is really worth the effort to fix up adjtimex.
But I am strongly in favor of adding CLOCK_TAI, since that would be easy to add (as you showed), is free from the tick issue, and would be really useful for Test and Measurement style applications
Thanks,
Richard
Quoted 8 lines · Richard Cochran
Quoted 2 lines · John Stultz
Richard, are you planning on taking another go at those changes? Or were my last suggestions just too daft? ;)
I wonder if it is really worth the effort to fix up adjtimex.
But I am strongly in favor of adding
CLOCK_TAI, since that would be easy to add (as you showed), is free from the tick issue, and would be really useful for Test and Measurement style applications
I've got the CLOCK_TAI patches queued for 3.6. I should send them to -tip soon here.
thanks
-john
Quoted 4 lines · Ben Hutchings
Quoted 3 lines · Richard Cochran
Quoted 2 lines · Jonathan Nieder
Ben Hutchings wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
6b43ae8a619d(ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends onbd3312681f69(ntp: Addntp_lockto replacextime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends onea7cf49a7633).If I understand the commit message for
6b43ae8a619dcorrectly, the livelock results fromntp_lockandxtime_lockbeing acquired in opposite orders in two threads. Which means it wasn't possible beforentp_lockwas introduced inbd3312681f69.
Yes, I think Ben is right that before the ntp_lock split the potential deadlock couldn't happen.
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
John, is that bug present in 3.2.y and 3.0.y, too? Any hints for fixing it?
It looks like incrementing the TAI offset was wrong even before
Quoted 8 lines
Quoted 4 lines
6b43ae8a ntp: Fix leap-second hrtimer livelock v3.4-rc1~44^2~9The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by
adjtimex()during an inserted second; is that right?
Yep. It just makes sure the TAI offset is adjusted at the same point that the leapsecond is inserted (as opposed to a second late).
Quoted 5 lines
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?
Attached is a simple leap second test you can play with.
thanks
-john
/* Leap second test
* by: john stultz ([email protected]) * (C) Copyright IBM 2012 * Licensed under the GPL
*/
#include <stdio.h>
#include <time.h>
#include <sys/time.h>
#include <sys/timex.h>
#define CALLS_PER_LOOP 64
#define NSEC_PER_SEC 1000000000ULL
/* returns 1 if a <= b, 0 otherwise */
static inline int in_order(struct timespec a, struct timespec b)
{
if(a.tv_sec < b.tv_sec)
return 1;
if(a.tv_sec > b.tv_sec)
return 0;
if(a.tv_nsec > b.tv_nsec)
return 0;
return 1;
}int main(void)
{
struct timeval tv;
struct timex tx;
struct timespec list[CALLS_PER_LOOP];
int i, inconsistent;
int clock_type = CLOCK_REALTIME;
long now, then;
/* Get the current time */
gettimeofday(&tv, NULL);
/* Calculate the next leap second */
tv.tv_sec += 86400 - tv.tv_sec % 86400;
/* Set the time to be 10 seconds from that time */
tv.tv_sec -= 10;
settimeofday(&tv, NULL);
/* Set the leap second insert flag */
tx.modes = ADJ_STATUS;
tx.status = STA_INS;
adjtimex(&tx);
clock_gettime(clock_type, &list[0]);
now = then = list[0].tv_sec;
while(now - then < 30){
inconsistent = 0;
/* Fill list */
for(i=0; i < CALLS_PER_LOOP; i++)
clock_gettime(clock_type, &list[i]);
/* Check for inconsistencies */
for(i=0; i < CALLS_PER_LOOP-1; i++)
if(!in_order(list[i],list[i+1]))
inconsistent = i;
/* display inconsistency */
if(inconsistent){
unsigned long long delta;
for(i=0; i < CALLS_PER_LOOP; i++){
if(i == inconsistent)
printf("--------------------\n");
printf("%lu:%lu\n",list[i].tv_sec,
list[i].tv_nsec);
if(i == inconsistent + 1 )
printf("--------------------\n");
}
delta = list[inconsistent].tv_sec*NSEC_PER_SEC;
delta += list[inconsistent].tv_nsec;
delta -= list[inconsistent+1].tv_sec*NSEC_PER_SEC;
delta -= list[inconsistent+1].tv_nsec;
printf("Delta: %llu ns\n", delta);
fflush(0);
break;
}
now = list[0].tv_sec;
}
/* clear TIME_WAIT */
tx.modes = ADJ_STATUS;
tx.status = 0;
adjtimex(&tx);
return 0;
}Quoted 1 line · John Stultz
On 06/18/2012 06:55 AM, Ben Hutchings wrote:
[...]
Quoted 17 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
John, is that bug present in 3.2.y and 3.0.y, too? Any hints for fixing it?
It looks like incrementing the TAI offset was wrong even before
Quoted 8 lines
Quoted 4 lines
6b43ae8a ntp: Fix leap-second hrtimer livelock v3.4-rc1~44^2~9The offset should change upon entering state OOP, so something like the following (untested) patch should fix it for 3.2.9.
[...]
It looks like this patch just changes the offset reported by
adjtimex()during an inserted second; is that right?Yep. It just makes sure the TAI offset is adjusted at the same point that the leapsecond is inserted (as opposed to a second late).
Quoted 1 line
Quoted 5 lines
Quoted 4 lines
Other than that, is 3.2.y likely to be OK? Is there a good way to test that in advance; does
<http://codemonkey.org.uk/2012/06/15/testing-leap-code/> look reasonable?Attached is a simple leap second test you can play with.
Thanks. That also detects inconsistency on some runs, but I don't see anything worse. So I don't intend to apply any of the ntp fixes to 3.2.y.
Ben.
Signature · 2 lines
Ben Hutchings If more than one person is responsible for a bug, no one is at fault.
Quoted 15 lines · Ben Hutchings
Quoted 5 lines · Richard Cochran
Quoted 4 lines · Jonathan Nieder
Quoted 3 lines · Ben Hutchings
Quoted 1 line · John Stultz
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.Quoted 8 lines
Quoted 8 lines
[...]
Quoted 2 lines
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
6b43ae8a619d(ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends onbd3312681f69(ntp: Addntp_lockto replacextime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends onea7cf49a7633).
Quoted 4 lines
If I understand the commit message for
6b43ae8a619dcorrectly, the livelock results fromntp_lockandxtime_lockbeing acquired in opposite orders in two threads. Which means it wasn't possible beforentp_lockwas introduced inbd3312681f69.
[...]
Apparently some other livelock was possible, though I was unable to reproduce it myself. Some proportion of systems running 2.6.32 or 3.2 (not sure about the intermediate stable-supported versions) are reported to have locked up, either in adjtimex or during the leap second.
I understand that we might not have so long to wait for the next leap second, so if anyone understands what fixes are still needed in stable updates I would really appreciate that.
Ben.
Signature · 2 lines
Ben Hutchings If you seem to know what you are doing, you'll be given more to do.
Quoted 17 lines · Ben Hutchings
Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Richard Cochran
Quoted 6 lines · Jonathan Nieder
Quoted 2 lines · Ben Hutchings
Quoted 1 line · John Stultz
commit
dd48d708ff3e917f6d6b6c2b696c3f18c019feedupstream.[...]
Quoted 2 lines
This doesn't apply to 3.2.y, unsurprisingly. Let me know if there are any urgent leap second fixes that will be needed there.
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
6b43ae8a619d(ntp: Fix leap-second hrtimer livelock) sounds important, but the patch depends onbd3312681f69(ntp: Addntp_lockto replacextime_locking) which does not have a commit message explaining its purpose (and that patch in turn depends onea7cf49a7633).If I understand the commit message for
6b43ae8a619dcorrectly, the livelock results fromntp_lockandxtime_lockbeing acquired in opposite orders in two threads. Which means it wasn't possible beforentp_lockwas introduced inbd3312681f69.
Quoted 6 lines
[...]
Apparently some other livelock was possible, though I was unable to reproduce it myself. Some proportion of systems running 2.6.32 or 3.2 (not sure about the intermediate stable-supported versions) are reported to have locked up, either in adjtimex or during the leap second.
Ugh. That sucks. Any more details as they come in would be helpful, specifically around the exact kernel versions affected.
Quoted 3 lines
I understand that we might not have so long to wait for the next leap second, so if anyone understands what fixes are still needed in stable updates I would really appreciate that.
Yea. I'm digging now to try to see what could be happening.
thanks
-john