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/
This archive was generated by hypermail 2b29 : Thu Jul 31 2003 - 22:00:44 EST