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);
}
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
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
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 [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
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