Messages in this thread |  | | Date | Fri, 17 Jul 2009 15:26:29 +0200 | From | Oliver Hartkopp <> | Subject | Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq |
| |
Peter Zijlstra wrote: > 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?
Would be fine to me.
It reduces the duplicated code as well as private structs for hrtimers & tasklets. And finally your suggestion preserves the proper separation of the hrtimers and the tasklets that are used as underlying concepts.
Regards, Oliver (who wrote net/can/bcm.c)
> > --- > 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 netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
|  |