[PATCH] NTP: remove clock_was_set() call to prevent deadlock

[PATCH] NTP: remove clock_was_set() call to prevent deadlock

[Thomas Gleixner]

The clock_was_set() call in seconds_overflow() which happens only when
leap seconds are inserted / deleted is wrong in two aspects:

1. it results in a call to on_each_cpu() with interrupts disabled
2. it is potential deadlock source vs. call_lock in smp_call_function()

The only possible side effect of the removal might be, that an absolute
CLOCK_REALTIME timer fires 1 second too late, in the rare case of leap
second deletion and an absolute CLOCK_REALTIME timer which expires in
the affected time frame. It will never fire too early.

This was probably observed by the reporter of a June 30th -> July 1st
hang: http://lkml.org/lkml/2007/7/3/

A similar problem was observed by Dave Jones, who provided a screen shot
with a lockdep back trace, which allowed to analyse the problem.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -122,7 +122,6 @@ void second_overflow(void)
 			 */
 			time_interpolator_update(-NSEC_PER_SEC);
 			time_state = TIME_OOP;
-			clock_was_set();
 			printk(KERN_NOTICE "Clock: inserting leap second "
 					"23:59:60 UTC\n");
 		}
@@ -137,7 +136,6 @@ void second_overflow(void)
 			 */
 			time_interpolator_update(NSEC_PER_SEC);
 			time_state = TIME_WAIT;
-			clock_was_set();
 			printk(KERN_NOTICE "Clock: deleting leap second "
 					"23:59:59 UTC\n");
 		}

Re: [PATCH] NTP: remove clock_was_set() call to prevent deadlock

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> The clock_was_set() call in seconds_overflow() which happens only when 
> leap seconds are inserted / deleted is wrong in two aspects:
> 
> 1. it results in a call to on_each_cpu() with interrupts disabled
> 2. it is potential deadlock source vs. call_lock in smp_call_function()
> 
> The only possible side effect of the removal might be, that an 
> absolute CLOCK_REALTIME timer fires 1 second too late, in the rare 
> case of leap second deletion and an absolute CLOCK_REALTIME timer 
> which expires in the affected time frame. It will never fire too 
> early.
> 
> This was probably observed by the reporter of a June 30th -> July 1st 
> hang: http://lkml.org/lkml/2007/7/3/
> 
> A similar problem was observed by Dave Jones, who provided a screen 
> shot with a lockdep back trace, which allowed to analyse the problem.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

looks good to me.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] NTP: remove clock_was_set() call to prevent deadlock

[Thomas Gleixner]

On Tue, 2007-07-03 at 20:05 +0200, Thomas Gleixner wrote:
> The clock_was_set() call in seconds_overflow() which happens only when
> leap seconds are inserted / deleted is wrong in two aspects:
> 
> 1. it results in a call to on_each_cpu() with interrupts disabled
> 2. it is potential deadlock source vs. call_lock in smp_call_function()
> 
> The only possible side effect of the removal might be, that an absolute
> CLOCK_REALTIME timer fires 1 second too late, in the rare case of leap
> second deletion and an absolute CLOCK_REALTIME timer which expires in
> the affected time frame. It will never fire too early.
> 
> This was probably observed by the reporter of a June 30th -> July 1st
> hang: http://lkml.org/lkml/2007/7/3/

Ooops, missed the full reference:

http://lkml.org/lkml/2007/7/3/103

	tglx

Re: [PATCH] NTP: remove clock_was_set() call to prevent deadlock

[Roman Zippel]

Hi,

On Tuesday 03 July 2007, Thomas Gleixner wrote:

> The clock_was_set() call in seconds_overflow() which happens only when
> leap seconds are inserted / deleted is wrong in two aspects:
>
> 1. it results in a call to on_each_cpu() with interrupts disabled
> 2. it is potential deadlock source vs. call_lock in smp_call_function()
>
> The only possible side effect of the removal might be, that an absolute
> CLOCK_REALTIME timer fires 1 second too late, in the rare case of leap
> second deletion and an absolute CLOCK_REALTIME timer which expires in
> the affected time frame. It will never fire too early.

That's a bit of an easy solution and doesn't fix the real problem. The 
clock_was_set() calls were correct, what's broken is the locking. Why wasn't 
that fixed instead?
I would at least like to see a comment there, why these calls were removed.

bye, Roman

Re: [PATCH] NTP: remove clock_was_set() call to prevent deadlock

[Thomas Gleixner]

On Wed, 2007-07-04 at 13:06 +0200, Roman Zippel wrote:
> Hi,
> 
> On Tuesday 03 July 2007, Thomas Gleixner wrote:
> 
> > The clock_was_set() call in seconds_overflow() which happens only when
> > leap seconds are inserted / deleted is wrong in two aspects:
> >
> > 1. it results in a call to on_each_cpu() with interrupts disabled
> > 2. it is potential deadlock source vs. call_lock in smp_call_function()
> >
> > The only possible side effect of the removal might be, that an absolute
> > CLOCK_REALTIME timer fires 1 second too late, in the rare case of leap
> > second deletion and an absolute CLOCK_REALTIME timer which expires in
> > the affected time frame. It will never fire too early.
> 
> That's a bit of an easy solution and doesn't fix the real problem. The 
> clock_was_set() calls were correct, what's broken is the locking. Why wasn't 
> that fixed instead?
> I would at least like to see a comment there, why these calls were removed.

We can not fix the locking that late in the cycle. I doubt that we can
fix it at all, as smp_call_function _must_ run with interrupts enabled,
therefor it can not run in interrupt context. It needs to run from
thread context only.

	tglx