Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq

[Peter Zijlstra]

On Tue, 2009-07-14 at 09:42 -0700, Linus Torvalds wrote:
> 
> On Tue, 14 Jul 2009, Peter Zijlstra wrote:
> > 
> > Linus really hated the softirq mode, which is what prompted me to change
> > that.
> > 
> > Now, it might be he only hated the particular interface and the
> > resulting code, but I think to remember he simply thought the whole
> > thing daft.
> 
> Yes. And I hated the bugs it had. 
> 
> Don't make something as core as timers any more complicated. Don't take 
> locks in timers and then complain about deadlocks. If your locking is 
> broken, don't make the core timers be idiotically broken.
> 
> Because it was. The code was a total mess to follow, and had bugs.

How would something like the below work for people?

---
 include/linux/hrtimer.h |   22 ++++++++++++++++++++--
 kernel/hrtimer.c        |   23 ++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 4759917..e7559fe 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -22,6 +22,7 @@
 #include <linux/wait.h>
 #include <linux/percpu.h>
 #include <linux/timer.h>
+#include <linux/interrupt.h>
 
 
 struct hrtimer_clock_base;
@@ -91,7 +92,6 @@ enum hrtimer_restart {
  * @function:	timer expiry callback function
  * @base:	pointer to the timer base (per cpu and per clock)
  * @state:	state information (See bit values above)
- * @cb_entry:	list head to enqueue an expired timer into the callback list
  * @start_site:	timer statistics field to store the site where the timer
  *		was started
  * @start_comm: timer statistics field to store the name of the process which
@@ -108,7 +108,6 @@ struct hrtimer {
 	enum hrtimer_restart		(*function)(struct hrtimer *);
 	struct hrtimer_clock_base	*base;
 	unsigned long			state;
-	struct list_head		cb_entry;
 #ifdef CONFIG_TIMER_STATS
 	int				start_pid;
 	void				*start_site;
@@ -116,6 +115,12 @@ struct hrtimer {
 #endif
 };
 
+struct hrtimer_softirq {
+	struct hrtimer		timer;
+	struct tasklet_struct	tasklet;
+	enum hrtimer_restart	(*function)(struct hrtimer *);
+};
+
 /**
  * struct hrtimer_sleeper - simple sleeper structure
  * @timer:	embedded timer structure
@@ -335,6 +340,19 @@ static inline void hrtimer_init_on_stack(struct hrtimer *timer,
 static inline void destroy_hrtimer_on_stack(struct hrtimer *timer) { }
 #endif
 
+enum hrtimer_restart __hrtimer_softirq_trampoline(struct hrtimer *timer);
+void __hrtimer_tasklet_trampoline(unsigned long data);
+
+static inline void hrtimer_softirq_init(struct hrtimer_softirq *stimer,
+		enum hrtimer_restart (*func)(struct hrtimer *),
+		clockid_t which_clock, enum hrtimer_mode mode)
+{
+	hrtimer_init(&stimer->timer, which_clock, mode);
+	stimer->timer.function = __hrtimer_softirq_trampoline;
+	tasklet_init(&stimer->tasklet, __hrtimer_tasklet_trampoline, stimer);
+	stimer->function = func;
+}
+
 /* Basic timer operations: */
 extern int hrtimer_start(struct hrtimer *timer, ktime_t tim,
 			 const enum hrtimer_mode mode);
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ab5eb70..dae063c 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1098,7 +1098,6 @@ static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
 		clock_id = CLOCK_MONOTONIC;
 
 	timer->base = &cpu_base->clock_base[clock_id];
-	INIT_LIST_HEAD(&timer->cb_entry);
 	hrtimer_init_timer_hres(timer);
 
 #ifdef CONFIG_TIMER_STATS
@@ -1141,6 +1140,28 @@ int hrtimer_get_res(const clockid_t which_clock, struct timespec *tp)
 }
 EXPORT_SYMBOL_GPL(hrtimer_get_res);
 
+enum hrtimer_restart __hrtimer_softirq_trampoline(struct hrtimer *timer)
+{
+	struct hrtimer_softirq *stimer =
+		container_of(timer, struct hrtimer_softirq, timer);
+
+	tasklet_hi_schedule(&timer->tasklet);
+
+	return HRTIMER_NORESTART;
+}
+EXPORT_SYMBOL_GPL(__hrtimer_softirq_trampoline);
+
+void __hrtimer_tasklet_trampoline(unsigned long data)
+{
+	struct hrtimer_softirq *stimer = (void *)data;
+	enum hrtimer_restart restart;
+
+	restart = stimer->function(&stimer->timer);
+	if (restart != HRTIMER_NORESTART)
+		hrtimer_restart(&stimer->timer);
+}
+EXPORT_SYMBOL_GPL(__hrtimer_tasklet_trampoline);
+
 static void __run_hrtimer(struct hrtimer *timer)
 {
 	struct hrtimer_clock_base *base = timer->base;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/