lkml.org 
[lkml]   [2003]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: PATCH: Race in 2.6.0-test2 timer code
On Wed, Jul 30, 2003 at 09:40:14AM +0200, Ingo Molnar wrote:
>
> On Wed, 30 Jul 2003, Andrea Arcangeli wrote:
>
> > [...] I'd feel much safer with the whole timer API being smp safe w/o
> > requiring driver developers to add complicated external brainer locking.
> > This will provide a much more friendly abstraction.
>
> i agree with the goal, but your patch does not achieve this. Your patch
> does not make double-add_timer() safe for example. As far as i can see
> your 2.6 patch only introduces additional overhead.

I never did any 2.6 patch, so it maybe a different thing what you've
seen, not what I applied to 2.4. Infact even the 2.4 patch isn't from
me.

This is the fix I applied to my 2.4 tree and it definitely fixes the
problem, including add_timer against add_timer. And it fixes the bug in
practice, it usually took a few hours to crash, not it can run for days
stable and I never heard complains again.

--- linux-2.4.19.SuSE-orig/kernel/timer.c 2003-07-02 13:53:55.000000000 -0700
+++ linux-2.4.19.SuSE-timer/kernel/timer.c 2003-07-02 18:32:08.000000000 -0700
@@ -144,17 +144,22 @@ void add_timer(timer_t *timer)

CHECK_BASE(base);
CHECK_BASE(timer->base);
- spin_lock_irqsave(&base->lock, flags);
- if (timer_pending(timer))
- goto bug;
- internal_add_timer(base, timer);
- timer->base = base;
- spin_unlock_irqrestore(&base->lock, flags);
- return;
-bug:
- spin_unlock_irqrestore(&base->lock, flags);
- printk("bug: kernel timer added twice at %p.\n",
- __builtin_return_address(0));
+
+ local_irq_save(flags);
+ while (unlikely(test_and_set_bit(0, &timer->lock)))
+ while (test_bit(0, &timer->lock));
+
+ spin_lock(&base->lock);
+ if (timer_pending(timer)) {
+ printk("bug: kernel timer added twice at %p.\n",
+ __builtin_return_address(0));
+ } else {
+ internal_add_timer(base, timer);
+ timer->base = base;
+ }
+ spin_unlock(&base->lock);
+ clear_bit(0, &timer->lock);
+ local_irq_restore(flags);
}

static inline int detach_timer(timer_t *timer)
@@ -244,16 +249,24 @@ int del_timer(timer_t * timer)
CHECK_BASE(timer->base);
if (!timer->base)
return 0;
+
+ local_irq_save(flags);
+ while (unlikely(test_and_set_bit(0, &timer->lock)))
+ while (test_bit(0, &timer->lock));
+
repeat:
base = timer->base;
- spin_lock_irqsave(&base->lock, flags);
+ spin_lock(&base->lock);
if (base != timer->base) {
- spin_unlock_irqrestore(&base->lock, flags);
+ spin_unlock(&base->lock);
goto repeat;
}
ret = detach_timer(timer);
timer->list.next = timer->list.prev = NULL;
- spin_unlock_irqrestore(&base->lock, flags);
+ spin_unlock(&base->lock);
+
+ clear_bit(0, &timer->lock);
+ local_irq_restore(flags);

return ret;
}
@@ -274,34 +287,48 @@ void sync_timers(void)

int del_timer_sync(timer_t * timer)
{
+ unsigned long flags;
tvec_base_t * base;
int ret = 0;

CHECK_BASE(timer->base);
if (!timer->base)
return 0;
+
+ local_irq_save(flags);
+ while (unlikely(test_and_set_bit(0, &timer->lock)))
+ while (test_bit(0, &timer->lock));
+
for (;;) {
- unsigned long flags;
int running;
-
repeat:
base = timer->base;
- spin_lock_irqsave(&base->lock, flags);
+ spin_lock(&base->lock);
if (base != timer->base) {
- spin_unlock_irqrestore(&base->lock, flags);
+ spin_unlock(&base->lock);
goto repeat;
}
ret += detach_timer(timer);
timer->list.next = timer->list.prev = 0;
running = timer_is_running(base, timer);
- spin_unlock_irqrestore(&base->lock, flags);
+ spin_unlock(&base->lock);

if (!running)
break;
+
+ clear_bit(0, &timer->lock);
+ local_irq_restore(flags);

timer_synchronize(base, timer);
+
+ local_irq_save(flags);
+ while (unlikely(test_and_set_bit(0, &timer->lock)))
+ while (test_bit(0, &timer->lock));
}

+ clear_bit(0, &timer->lock);
+ local_irq_restore(flags);
+
return ret;
}
#endif
Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:47    [W:0.108 / U:1.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site