Skip to contentSkip to conversation

kernel/timer.c: xtime lock missing

PATCH In review Benjamin LaHaise 4 participants 5 messages Oct 21, 2004 – Oct 26, 2004

lore
Share & tools
lore

Benjamin LaHaise

author
#0 lore raw

PATCH 1/1 kernel/timer.c: xtime lock missing

Hello all,

While looking at the time keeping code for related work, I came across the following bug. During 2.5 development, the smptimers patch removed a lock from update_times() that is actually needed over the xtime update, since the second overflow is not an atomic operation. This patch fixes that by doing a write_seqlock() over the course of the update.

-ben

Signature · 19 lines
"Time is what keeps everything from happening all at once." -- John Wheeler

===== timer.c 1.93 vs edited =====
--- 1.93/kernel/timer.c	2004-09-17 03:07:06 -04:00
+++ edited/timer.c	2004-10-21 14:51:53 -04:00
@@ -940,11 +940,14 @@
 {
 	unsigned long ticks;
 
+	/* interrupts are disabled */
+	write_seqlock(&xtime_lock);
 	ticks = jiffies - wall_jiffies;
 	if (ticks) {
 		wall_jiffies += ticks;
 		update_wall_time(ticks);
 	}
+	write_sequnlock(&xtime_lock);
 	calc_load(ticks);
 }
#1 lore raw
Quoted 8 lines · Benjamin LaHaise

Hello all,

While looking at the time keeping code for related work, I came across the following bug. During 2.5 development, the smptimers patch removed a lock from update_times() that is actually needed over the xtime update, since the second overflow is not an atomic operation. This patch fixes that by doing a write_seqlock() over the course of the update.

Errrr...

Looking at the comment above that function, the xtime_lock should already be held when executing that code. timer_interrupt() should be the function which grabs the lock and calls do_timer_interrupt() then do_timer() then update_times().

Or am I missing something?

thanks
-john

author
#2 lore raw
Quoted 6 lines · john stultz

Looking at the comment above that function, the xtime_lock should already be held when executing that code. timer_interrupt() should be the function which grabs the lock and calls do_timer_interrupt() then do_timer() then update_times().

Or am I missing something?

No, you're right; I'm blind. That is a very distant chain between where the lock is acquired and where it matters, perhaps a few more comments are in order.

-ben

Signature · 1 line
"Time is what keeps everything from happening all at once." -- John Wheeler
#3 lore raw
Quoted 11 lines · Benjamin LaHaise
Quoted 6 lines · john stultz

Looking at the comment above that function, the xtime_lock should already be held when executing that code. timer_interrupt() should be the function which grabs the lock and calls do_timer_interrupt() then do_timer() then update_times().

Or am I missing something?

No, you're right; I'm blind. That is a very distant chain between where the lock is acquired and where it matters, perhaps a few more comments are in order.

If memory serves, there is a problem here in that the lock is taken in arch code and not all archs are taking it. I think a check of the several arch time.c callers might be in order.

Signature · 2 lines
George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
#4 lore raw
Quoted 3 lines · George Anzinger

If memory serves, there is a problem here in that the lock is taken in arch code and not all archs are taking it. I think a check of the several arch time.c callers might be in order.

Certainly absolutely none of the ARM timer implementations were taking the lock. They do now because its rather necessary to ensure working gettimeofday().

Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core