Skip to contentSkip to conversation

fix mod_timer() race

PATCH In review Andrew Morton 1 participant 1 message Nov 5, 2002 High-resolution timers, timer wheel, clockevents

lore
Share & tools
lore
#0 lore raw

PATCH 1/4 fix mod_timer() race

If two CPUs run mod_timer against the same not-pending timer then they have no locking relationship. They can both see the timer as not-pending and they both add the timer to their cpu-local list. The CPU which gets there second corrupts the first CPU's lists.

This was causing Dave Hansen's 8-way to oops after a couple of minutes of specweb testing.

I believe that to fix this we need locking which is associated with the timer itself. The easy fix is hashed spinlocking based on the timer's address. The hard fix is a lock inside the timer itself.

It is hard because init_timer() becomes compulsory, to initialise that spinlock. An unknown number of code paths in the kernel just wipe the timer to all-zeroes and start using it.

I chose the hard way - it is cleaner and more idiomatic. The patch also adds a "magic number" to the timer so we can detect when a timer was not correctly initialised. A warning and stack backtrace is generated and the timer is fixed up. After 16 such warnings the warning mechanism shuts itself up until a reboot.

It took six patches to my kernel to stop the warnings from coming out. The uninitialised timers are extremely easy to find and fix. But it will take some time to weed them all out. Maybe we should go for the hashed locking...

Note that the new timer->lock means that we can clean up some awkward "oh we raced, let's try again" code in timer.c. But to do that we'd also need to take timer->lock in the commonly-called del_timer(), so I left it as-is.

The lock is not needed in add_timer() because concurrent add_timer()/add_timer() and concurrent add_timer()/mod_timer() are illegal.

 include/linux/timer.h |    8 ++++++++
 kernel/timer.c        |   42 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 3 deletions(-)
include/linux/timer.h renamed from include/linux/timer.h~mod_timer-race +8 -0
@@ -3,6 +3,7 @@
 
 #include <linux/config.h>
 #include <linux/list.h>
+#include <linux/spinlock.h>
 
 struct tvec_t_base_s;
 
@@ -10,12 +11,17 @@ struct timer_list {
 	struct list_head entry;
 	unsigned long expires;
 
+	spinlock_t lock;
+	unsigned long magic;
+
 	void (*function)(unsigned long);
 	unsigned long data;
 
 	struct tvec_t_base_s *base;
 };
 
+#define TIMER_MAGIC	0x4b87ad6e
+
 /***
  * init_timer - initialize a timer.
  * @timer: the timer to be initialized
@@ -26,6 +32,8 @@ struct timer_list {
 static inline void init_timer(struct timer_list * timer)
 {
 	timer->base = NULL;
+	timer->magic = TIMER_MAGIC;
+	spin_lock_init(&timer->lock);
 }
 
 /***
kernel/timer.c renamed from kernel/timer.c~mod_timer-race +39 -3

96 lines not on this page · raw message

.