Skip to contentSkip to conversation

NTP: remove clock_was_set() call to prevent deadlock

PATCH In review Thomas Gleixner 3 participants 5 messages Jul 3, 2007 – Jul 4, 2007 Timekeeping, clocksource core, NTP, alarmtimer

lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

PATCH 1/1 NTP: remove clock_was_set() call to prevent deadlock

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 <[email protected]>
kernel/time/ntp.c +0 -2
@@ -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");
 		}
Acked-by on v1 1/1
#1 lore raw
Quoted 12 lines · 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.

Quoted 7 lines

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 <[email protected]>

looks good to me.

Acked-by Ingo Molnar <[email protected]>

Ingo

author
#2 lore raw
In reply to Thomas Gleixner
Quoted 6 lines · 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()

Quoted 7 lines

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

#3 lore raw
In reply to Thomas Gleixner

Hi,

Quoted 10 lines · 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.

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

author
#4 lore raw
Quoted 15 lines · Roman Zippel

Hi,

Quoted 10 lines · 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.

Quoted 4 lines

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