lkml.org 
[lkml]   [2003]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch] SMP races in the timer code, timer-fix-2.6.0-test7-A0

On Sat, 11 Oct 2003, Ingo Molnar wrote:

> it would be much cleaner to add another timer->running field, especially
> since this would be the 8th word-sized field in struct timer_list,
> making it a nice round structure size.
>
> btw., there's a third type of timer race we have. If a timer function is
> delayed by more than 1 timer tick [which could happen under eg. UML],
> then it's possible for the timer function to run on another CPU in
> parallel to the already executing timer function.

i've implemented the timer->running field, and it's quite straightforward
and solves all 3 types of races. It also simplifies del_timer_sync() quite
visibly, and it's probably a fair del_timer_sync() speedup as well. I
tested the attached patch on SMP and UP x86 as well, works fine for me.

the timer->running field is now also used to guarantee serialization of
timer->fn execution of the same timer [race #3]. This is a quite
reasonable thing to expect, and a fair portion of timer users do expect
this.

Ingo

--- linux/include/linux/timer.h.orig
+++ linux/include/linux/timer.h
@@ -18,6 +18,7 @@ struct timer_list {
unsigned long data;

struct tvec_t_base_s *base;
+ unsigned long running;
};

#define TIMER_MAGIC 0x4b87ad6e
@@ -29,6 +30,7 @@ struct timer_list {
.base = NULL, \
.magic = TIMER_MAGIC, \
.lock = SPIN_LOCK_UNLOCKED, \
+ .running = 0, \
}

/***
@@ -42,6 +44,7 @@ static inline void init_timer(struct tim
{
timer->base = NULL;
timer->magic = TIMER_MAGIC;
+ timer->running = 0;
spin_lock_init(&timer->lock);
}

--- linux/kernel/timer.c.orig
+++ linux/kernel/timer.c
@@ -57,7 +57,6 @@ typedef struct tvec_root_s {
struct tvec_t_base_s {
spinlock_t lock;
unsigned long timer_jiffies;
- struct timer_list *running_timer;
tvec_root_t tv1;
tvec_t tv2;
tvec_t tv3;
@@ -67,14 +66,6 @@ struct tvec_t_base_s {

typedef struct tvec_t_base_s tvec_base_t;

-static inline void set_running_timer(tvec_base_t *base,
- struct timer_list *timer)
-{
-#ifdef CONFIG_SMP
- base->running_timer = timer;
-#endif
-}
-
/* Fake initialization */
static DEFINE_PER_CPU(tvec_base_t, tvec_bases) = { SPIN_LOCK_UNLOCKED };

@@ -315,35 +306,27 @@ EXPORT_SYMBOL(del_timer);
* the timer it also makes sure the handler has finished executing on other
* CPUs.
*
- * Synchronization rules: callers must prevent restarting of the timer,
- * otherwise this function is meaningless. It must not be called from
- * interrupt contexts. Upon exit the timer is not queued and the handler
- * is not running on any CPU.
+ * Synchronization rules: callers must prevent restarting of the timer
+ * (except restarting the timer from the timer function itself), otherwise
+ * this function is meaningless. It must not be called from interrupt
+ * contexts. Upon exit the timer is not queued and the handler is not
+ * running on any CPU.
*
- * The function returns whether it has deactivated a pending timer or not.
+ * The function returns the number of times it has deactivated a pending
+ * timer.
*/
int del_timer_sync(struct timer_list *timer)
{
- tvec_base_t *base;
- int i, ret = 0;
+ int ret = 0;

check_timer(timer);

del_again:
ret += del_timer(timer);

- for (i = 0; i < NR_CPUS; i++) {
- if (!cpu_online(i))
- continue;
-
- base = &per_cpu(tvec_bases, i);
- if (base->running_timer == timer) {
- while (base->running_timer == timer) {
- cpu_relax();
- preempt_check_resched();
- }
- break;
- }
+ while (unlikely(timer->running)) {
+ cpu_relax();
+ preempt_check_resched();
}
smp_rmb();
if (timer_pending(timer))
@@ -417,16 +400,36 @@ repeat:
fn = timer->function;
data = timer->data;

+#ifdef CONFIG_SMP
+ /*
+ * If the timer is running on another CPU then
+ * wait for it - without holding the base lock.
+ * This is a very rare but possible situation.
+ *
+ * Re-start the whole processing after that, the
+ * timer might have been removed while we dropped
+ * the lock.
+ */
+ if (unlikely(timer->running)) {
+ spin_unlock_irq(&base->lock);
+ while (timer->running)
+ cpu_relax();
+ spin_lock_irq(&base->lock);
+ goto repeat;
+ }
+ timer->running = 1;
+#endif
list_del(&timer->entry);
timer->base = NULL;
- set_running_timer(base, timer);
spin_unlock_irq(&base->lock);
fn(data);
spin_lock_irq(&base->lock);
+#ifdef CONFIG_SMP
+ timer->running = 0;
+#endif
goto repeat;
}
}
- set_running_timer(base, NULL);
spin_unlock_irq(&base->lock);
}

-
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:49    [W:0.062 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site