* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-18 13:55 ` Ben Hutchings
@ 2012-06-18 16:28 ` Richard Cochran
2012-06-19 11:54 ` Ben Hutchings
2012-06-18 18:20 ` John Stultz
2012-07-01 1:28 ` Ben Hutchings
2 siblings, 1 reply; 15+ messages in thread
From: Richard Cochran @ 2012-06-18 16:28 UTC (permalink / raw)
To: Ben Hutchings
Cc: Jonathan Nieder, John Stultz, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
On Mon, Jun 18, 2012 at 02:55:11PM +0100, Ben Hutchings wrote:
> On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
> > 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.
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-18 16:28 ` Richard Cochran
@ 2012-06-19 11:54 ` Ben Hutchings
2012-06-19 17:26 ` John Stultz
0 siblings, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2012-06-19 11:54 UTC (permalink / raw)
To: Richard Cochran
Cc: Jonathan Nieder, John Stultz, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]
On Mon, 2012-06-18 at 18:28 +0200, Richard Cochran wrote:
> On Mon, Jun 18, 2012 at 02:55:11PM +0100, Ben Hutchings wrote:
> > On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
> > > 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.
>
> > 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...
> Here is a little test program I have been using:
>
> https://github.com/richardcochran/leap
Thanks, that runs without incident but does show the incorrect offset
during OOP.
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-19 11:54 ` Ben Hutchings
@ 2012-06-19 17:26 ` John Stultz
2012-06-20 16:25 ` Richard Cochran
0 siblings, 1 reply; 15+ messages in thread
From: John Stultz @ 2012-06-19 17:26 UTC (permalink / raw)
To: Ben Hutchings
Cc: Richard Cochran, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
On 06/19/2012 04:54 AM, Ben Hutchings wrote:
> On Mon, 2012-06-18 at 18:28 +0200, Richard Cochran wrote:
>> On Mon, Jun 18, 2012 at 02:55:11PM +0100, Ben Hutchings wrote:
>>> On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
>>>> 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.
>>
>>> 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...
>
>> Here is a little test program I have been using:
>>
>> https://github.com/richardcochran/leap
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-19 17:26 ` John Stultz
@ 2012-06-20 16:25 ` Richard Cochran
2012-06-20 16:42 ` John Stultz
0 siblings, 1 reply; 15+ messages in thread
From: Richard Cochran @ 2012-06-20 16:25 UTC (permalink / raw)
To: John Stultz
Cc: Ben Hutchings, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
On Tue, Jun 19, 2012 at 10:26:49AM -0700, John Stultz wrote:
>
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-20 16:25 ` Richard Cochran
@ 2012-06-20 16:42 ` John Stultz
0 siblings, 0 replies; 15+ messages in thread
From: John Stultz @ 2012-06-20 16:42 UTC (permalink / raw)
To: Richard Cochran
Cc: Ben Hutchings, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
On 06/20/2012 09:25 AM, Richard Cochran wrote:
> On Tue, Jun 19, 2012 at 10:26:49AM -0700, John Stultz wrote:
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-18 13:55 ` Ben Hutchings
2012-06-18 16:28 ` Richard Cochran
@ 2012-06-18 18:20 ` John Stultz
2012-06-19 11:57 ` Ben Hutchings
2012-07-01 1:28 ` Ben Hutchings
2 siblings, 1 reply; 15+ messages in thread
From: John Stultz @ 2012-06-18 18:20 UTC (permalink / raw)
To: Ben Hutchings
Cc: Richard Cochran, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]
On 06/18/2012 06:55 AM, Ben Hutchings wrote:
> On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
>> On Sun, Jun 17, 2012 at 11:47:51AM -0500, Jonathan Nieder wrote:
>>> Ben Hutchings wrote:
>>>
>>> 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).
> 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.
Yes, I think Ben is right that before the ntp_lock split the potential
deadlock couldn't happen.
>>> 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.
> [...]
>
> 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).
>
> 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
[-- Attachment #2: leaptest.c --]
[-- Type: text/x-csrc, Size: 2076 bytes --]
/* Leap second test
* by: john stultz (johnstul@us.ibm.com)
* (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;
}
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-18 18:20 ` John Stultz
@ 2012-06-19 11:57 ` Ben Hutchings
0 siblings, 0 replies; 15+ messages in thread
From: Ben Hutchings @ 2012-06-19 11:57 UTC (permalink / raw)
To: John Stultz
Cc: Richard Cochran, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]
On Mon, 2012-06-18 at 11:20 -0700, John Stultz wrote:
> On 06/18/2012 06:55 AM, Ben Hutchings wrote:
[...]
> >>> 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.
> > [...]
> >
> > 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).
>
> >
> > 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.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-06-18 13:55 ` Ben Hutchings
2012-06-18 16:28 ` Richard Cochran
2012-06-18 18:20 ` John Stultz
@ 2012-07-01 1:28 ` Ben Hutchings
2012-07-01 5:27 ` John Stultz
2 siblings, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2012-07-01 1:28 UTC (permalink / raw)
To: Richard Cochran
Cc: Jonathan Nieder, John Stultz, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]
On Mon, 2012-06-18 at 14:55 +0100, Ben Hutchings wrote:
> On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
> > On Sun, Jun 17, 2012 at 11:47:51AM -0500, Jonathan Nieder wrote:
> > > Ben Hutchings wrote:
> > > > On Fri, 2012-06-15 at 11:56 -0700, John Stultz wrote:
> > >
> > > >> commit dd48d708ff3e917f6d6b6c2b696c3f18c019feed upstream.
> > > [...]
> > > > 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).
>
> 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.
[...]
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.
--
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH -stable] ntp: Correct TAI offset during leap second
2012-07-01 1:28 ` Ben Hutchings
@ 2012-07-01 5:27 ` John Stultz
0 siblings, 0 replies; 15+ messages in thread
From: John Stultz @ 2012-07-01 5:27 UTC (permalink / raw)
To: Ben Hutchings
Cc: Richard Cochran, Jonathan Nieder, stable, Sasha Levin,
Thomas Gleixner, Dave Jones, lkml
On 06/30/2012 06:28 PM, Ben Hutchings wrote:
> On Mon, 2012-06-18 at 14:55 +0100, Ben Hutchings wrote:
>> On Sun, 2012-06-17 at 19:34 +0200, Richard Cochran wrote:
>>> On Sun, Jun 17, 2012 at 11:47:51AM -0500, Jonathan Nieder wrote:
>>>> Ben Hutchings wrote:
>>>>> On Fri, 2012-06-15 at 11:56 -0700, John Stultz wrote:
>>>>>> commit dd48d708ff3e917f6d6b6c2b696c3f18c019feed upstream.
>>>> [...]
>>>>> 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).
>> 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.
> [...]
>
> 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.
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread