[PATCH] timer race fixes

From: Ingo Molnar <mingo@elte.hu>

It unifies the functionality of add_timer() and mod_timer(), and makes any
combination of the timer API calls completely SMP-safe.  del_timer() is still
not using the timer lock.

this patch fixes the only timer bug in 2.6 i'm aware of: the del_timer_sync()
+ add_timer() combination in kernel/itimer.c is buggy.  This was correct code
in 2.4, because there it was safe to do an add_timer() from the timer handler
itself, parallel to a del_timer_sync().

If we want to make this safe in 2.6 too (which i think we want to) then we
have to make add_timer() almost equivalent to mod_timer(), locking-wise.  And
once we are at this point i think it's much cleaner to actually make
add_timer() a variant of mod_timer().  (There's no locking cost for
add_timer(), only the cost of an extra branch.  And we've removed another
commonly used function from the icache.)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index d98c9a948aa726113876ab6888cd44d7310a07ee..e8f79ea49c74aaabf8d844d898f13ead1f32161c 100644 (file)
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -60,11 +60,30 @@ static inline int timer_pending(const struct timer_list * timer)
        return timer->base != NULL;
 }
 
-extern void add_timer(struct timer_list * timer);
 extern void add_timer_on(struct timer_list *timer, int cpu);
 extern int del_timer(struct timer_list * timer);
+extern int __mod_timer(struct timer_list *timer, unsigned long expires);
 extern int mod_timer(struct timer_list *timer, unsigned long expires);
-  
+
+/***
+ * add_timer - start a timer
+ * @timer: the timer to be added
+ *
+ * The kernel will do a ->function(->data) callback from the
+ * timer interrupt at the ->expired point in the future. The
+ * current time is 'jiffies'.
+ *
+ * The timer's ->expired, ->function (and if the handler uses it, ->data)
+ * fields must be set prior calling this function.
+ *
+ * Timers with an ->expired field in the past will be executed in the next
+ * timer tick.
+ */
+static inline void add_timer(struct timer_list * timer)
+{
+       __mod_timer(timer, timer->expires);
+}
+
 #ifdef CONFIG_SMP
   extern int del_timer_sync(struct timer_list * timer);
 #else

diff --git a/kernel/ksyms.c b/kernel/ksyms.c
index 5fd68e235ff7c53b47f51593addce853226c7ab3..ab951f967218922cef1fdb2d3302a9aeb849d136 100644 (file)
--- a/kernel/ksyms.c
+++ b/kernel/ksyms.c
@@ -405,8 +405,6 @@ EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
 EXPORT_SYMBOL(proc_doulongvec_minmax);
 
 /* interrupt handling */
-EXPORT_SYMBOL(add_timer);
-EXPORT_SYMBOL(del_timer);
 EXPORT_SYMBOL(request_irq);
 EXPORT_SYMBOL(free_irq);
 
@@ -433,7 +431,10 @@ EXPORT_SYMBOL(probe_irq_off);
 #ifdef CONFIG_SMP
 EXPORT_SYMBOL(del_timer_sync);
 #endif
+EXPORT_SYMBOL(add_timer);
+EXPORT_SYMBOL(del_timer);
 EXPORT_SYMBOL(mod_timer);
+EXPORT_SYMBOL(__mod_timer);
 
 #ifdef HAVE_DISABLE_HLT
 EXPORT_SYMBOL(disable_hlt);

diff --git a/kernel/timer.c b/kernel/timer.c
index ad4543e5a8714b1ee81168a1b16a1d102db151d7..81bb979fb7e7b362ed943fa5cc941371fd54cf26 100644 (file)
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -144,34 +144,62 @@ static void internal_add_timer(tvec_base_t *base, struct timer_list *timer)
        list_add_tail(&timer->entry, vec);
 }
 
-/***
- * add_timer - start a timer
- * @timer: the timer to be added
- *
- * The kernel will do a ->function(->data) callback from the
- * timer interrupt at the ->expired point in the future. The
- * current time is 'jiffies'.
- *
- * The timer's ->expired, ->function (and if the handler uses it, ->data)
- * fields must be set prior calling this function.
- *
- * Timers with an ->expired field in the past will be executed in the next
- * timer tick. It's illegal to add an already pending timer.
- */
-void add_timer(struct timer_list *timer)
+int __mod_timer(struct timer_list *timer, unsigned long expires)
 {
-       tvec_base_t *base = &get_cpu_var(tvec_bases);
-       unsigned long flags;
-  
-       BUG_ON(timer_pending(timer) || !timer->function);
+       tvec_base_t *old_base, *new_base;
+       unsigned long flags;
+       int ret = 0;
+
+       BUG_ON(!timer->function);
 
        check_timer(timer);
 
-       spin_lock_irqsave(&base->lock, flags);
-       internal_add_timer(base, timer);
-       timer->base = base;
-       spin_unlock_irqrestore(&base->lock, flags);
-       put_cpu_var(tvec_bases);
+       spin_lock_irqsave(&timer->lock, flags);
+       new_base = &__get_cpu_var(tvec_bases);
+repeat:
+       old_base = timer->base;
+
+       /*
+        * Prevent deadlocks via ordering by old_base < new_base.
+        */
+       if (old_base && (new_base != old_base)) {
+               if (old_base < new_base) {
+                       spin_lock(&new_base->lock);
+                       spin_lock(&old_base->lock);
+               } else {
+                       spin_lock(&old_base->lock);
+                       spin_lock(&new_base->lock);
+               }
+               /*
+                * The timer base might have been cancelled while we were
+                * trying to take the lock(s):
+                */
+               if (timer->base != old_base) {
+                       spin_unlock(&new_base->lock);
+                       spin_unlock(&old_base->lock);
+                       goto repeat;
+               }
+       } else
+               spin_lock(&new_base->lock);
+
+       /*
+        * Delete the previous timeout (if there was any), and install
+        * the new one:
+        */
+       if (old_base) {
+               list_del(&timer->entry);
+               ret = 1;
+       }
+       timer->expires = expires;
+       internal_add_timer(new_base, timer);
+       timer->base = new_base;
+
+       if (old_base && (new_base != old_base))
+               spin_unlock(&old_base->lock);
+       spin_unlock(&new_base->lock);
+       spin_unlock_irqrestore(&timer->lock, flags);
+
+       return ret;
 }
 
 /***
@@ -179,7 +207,7 @@ void add_timer(struct timer_list *timer)
  * @timer: the timer to be added
  * @cpu: the CPU to start it on
  *
- * This is not very scalable on SMP.
+ * This is not very scalable on SMP. Double adds are not possible.
  */
 void add_timer_on(struct timer_list *timer, int cpu)
 {
@@ -217,10 +245,6 @@ void add_timer_on(struct timer_list *timer, int cpu)
  */
 int mod_timer(struct timer_list *timer, unsigned long expires)
 {
-       tvec_base_t *old_base, *new_base;
-       unsigned long flags;
-       int ret = 0;
-
        BUG_ON(!timer->function);
 
        check_timer(timer);
@@ -233,52 +257,7 @@ int mod_timer(struct timer_list *timer, unsigned long expires)
        if (timer->expires == expires && timer_pending(timer))
                return 1;
 
-       spin_lock_irqsave(&timer->lock, flags);
-       new_base = &__get_cpu_var(tvec_bases);
-repeat:
-       old_base = timer->base;
-
-       /*
-        * Prevent deadlocks via ordering by old_base < new_base.
-        */
-       if (old_base && (new_base != old_base)) {
-               if (old_base < new_base) {
-                       spin_lock(&new_base->lock);
-                       spin_lock(&old_base->lock);
-               } else {
-                       spin_lock(&old_base->lock);
-                       spin_lock(&new_base->lock);
-               }
-               /*
-                * The timer base might have been cancelled while we were
-                * trying to take the lock(s):
-                */
-               if (timer->base != old_base) {
-                       spin_unlock(&new_base->lock);
-                       spin_unlock(&old_base->lock);
-                       goto repeat;
-               }
-       } else
-               spin_lock(&new_base->lock);
-
-       /*
-        * Delete the previous timeout (if there was any), and install
-        * the new one:
-        */
-       if (old_base) {
-               list_del(&timer->entry);
-               ret = 1;
-       }
-       timer->expires = expires;
-       internal_add_timer(new_base, timer);
-       timer->base = new_base;
-
-       if (old_base && (new_base != old_base))
-               spin_unlock(&old_base->lock);
-       spin_unlock(&new_base->lock);
-       spin_unlock_irqrestore(&timer->lock, flags);
-
-       return ret;
+       return __mod_timer(timer, expires);
 }
 
 /***