[PATCH] kernel/timer.c: xtime lock missing

[PATCH] kernel/timer.c: xtime lock missing

[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.

		-ben
-- 
"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);
 }
   

Re: [PATCH] kernel/timer.c: xtime lock missing

[john stultz]

On Thu, 2004-10-21 at 12:03, Benjamin LaHaise wrote:
> 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

Re: [PATCH] kernel/timer.c: xtime lock missing

[Benjamin LaHaise]

On Thu, Oct 21, 2004 at 01:23:32PM -0700, john stultz wrote:
> 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
-- 
"Time is what keeps everything from happening all at once." -- John Wheeler

Re: [PATCH] kernel/timer.c: xtime lock missing

[George Anzinger]

Benjamin LaHaise wrote:
> On Thu, Oct 21, 2004 at 01:23:32PM -0700, john stultz wrote:
> 
>>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.

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] kernel/timer.c: xtime lock missing

[Russell King]

On Mon, Oct 25, 2004 at 04:09:51PM -0700, George Anzinger wrote:
> 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().

-- 
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