kuznet@ms2.inr.ac.ru wrote:
>
> Hello!
Howdy. Replying to two emails here...
>...
> BTW del_timer_sync() (or its proper modification) is planned to be used
> for TCP to get rid of reference counts, which are seen in profiles.
> No global dependencies are allowed in this case.
This should be OK.
> To be honest I still did not find the patch, where new del_timer()
> is defined. 8)
I've attached an in-progress version of the del_timer patch to this email. It assumes that:
1: timer handlers are globally serialised (correct)
2: (*timer) will _not_ be kfree'ed by the handler.
The second assumption is OK because calling del_timer on a timer which can be freed from its handler is a BUG (unless the del_timer is done from the handler itself, which
it copes with).
> Brigde and decnet must be clean as well. If they are not,
> this code has active maintainers, which can be kicked.
OK.
> > inet6_ifa_finish_destroy() does a del_timer() and then kfree's 'ifp'.
>
> All these things use reference counting. When destructor is entered,
> it is the last user and no timer is running on the object.
>
> del_timer() there is pure debugging check.
OK, so all of ipv6 can safely continue to use async del_timer.
> > In reassembly.c, both calls to del_timer() are followed by a
> > frag_kfree_s(fq), but the handler could still be running, and it
> > accesses *fq.
>
> Yes... It was safe in 2.2 (BH only), now it is bug. And del_timer_sync()
> is dangerous to be used here, reassembly function could be called
> from an obscure context... Reference counting.
So I propose that we use del_timer_async in reassembly.c for now.
Who should look at the alleged bug? Yourself? Pedro?
> I agreed! The thing, which I still do not understand is how you managed
> to implement correct del_timer_sync(). 8)
Basically just a global var which points to the currently-running-timer. If it points to the timer being deleted, spin until it doesn't :) Patch attached for
review/comments.
-- -akpm-
--- linux-2.4.0test1-ac2/kernel/timer.c Mon May 15 21:25:15 2000 +++ linux-akpm/kernel/timer.c Sun May 28 00:57:03 2000 @@ -13,6 +13,8 @@ * serialize accesses to xtime/lost_ticks). * Copyright (C) 1998 Andrea Arcangeli * 1999-03-10 Improved NTP compatibility by Ulrich Windl + * 2000-05-27 Make del_timer synchronous, add del_timer_async - Andrew Morton. + * http://kernelnotes.org/lnxlists/linux-kernel/lk_0005_04/msg00672.html */ #include <linux/config.h> @@ -72,6 +74,23 @@ unsigned long prof_len; unsigned long prof_shift; +#ifdef CONFIG_SMP +/* + * A non-zero entry here indicates a currently-running timer on + * the CPU identified by running_timer_cpu. + */ +static struct timer_list *running_timer; +int running_timer_cpu; + +/* FIXME: delete this stuff */ +#ifdef CONFIG_X86 +extern void show_stack(unsigned long* esp); /* Why doesn't this have a prototype? */ +#define x86_show_stack() show_stack(0) +#else +#define x86_show_stack() do { } while (0) +#endif +#endif /* CONFIG_SMP */ + /* * Event timer code */ @@ -201,6 +220,7 @@ return ret; } +#ifndef CONFIG_SMP int del_timer(struct timer_list * timer) { int ret; @@ -212,39 +232,82 @@ spin_unlock_irqrestore(&timerlist_lock, flags); return ret; } +#endif #ifdef CONFIG_SMP -/* - * SMP specific function to delete periodic timer. - * Caller must disable by some means restarting the timer - * for new. Upon exit the timer is not queued and handler is not running - * on any CPU. It returns number of times, which timer was deleted - * (for reference counting). - */ -int del_timer_sync(struct timer_list * timer) +int del_timer_async(struct timer_list * timer) { - int ret = 0; + int ret; + unsigned long flags; - for (;;) { - unsigned long flags; - int running; + spin_lock_irqsave(&timerlist_lock, flags); + ret = detach_timer(timer); + timer->list.next = timer->list.prev = NULL; + spin_unlock_irqrestore(&timerlist_lock, flags); + return ret; +} - spin_lock_irqsave(&timerlist_lock, flags); - ret += detach_timer(timer); - timer->list.next = timer->list.prev = 0; - running = timer->running; +int del_timer(struct timer_list * timer) +{ + int ret; + unsigned long flags; + + spin_lock_irqsave(&timerlist_lock, flags); + ret = detach_timer(timer); + timer->list.next = timer->list.prev = 0; + + if (running_timer == timer) { /* This timer's handler is presently running */ + int j; + struct timer_list * volatile *vtl; + + if (running_timer_cpu == smp_processor_id()) + goto out; /* Handler is deleting timer. Usually pointless. */ + + /* FIXME: temp */ + printk( "del_timer(%p) during handler execution! Called from %p\n" + "See http://www.uow.edu.au/~andrewm/linux/del_timer.html\n", + timer, __builtin_return_address(0)); + x86_show_stack(); +re_del_timer: + /* Unlock the timer list and spin until handler completion. */ spin_unlock_irqrestore(&timerlist_lock, flags); + vtl = (struct timer_list * volatile *)&(running_timer); + for (j = 50 * 1000 * 1000; j != 0; --j) { + if (*vtl != timer) + break; + } + spin_lock_irqsave(&timerlist_lock, flags); - if (!running) - return ret; - timer_synchronize(timer); + if (j == 0) { + printk( "del_timer(%p): deadlock! Called from %p\n", + timer, __builtin_return_address(0)); + printk("See http://www.uow.edu.au/~andrewm/linux/deadlock.html\n"); + x86_show_stack(); + } else if (running_timer == timer) { + /* There's a tiny chance that the same timer handler has started to run again */ + printk("del_timer(%p): the timer started running again!\n", timer); + x86_show_stack(); + goto re_del_timer; + + /* + * OK, we have the lock and the timer handler is definitely not running. But the + * handler may have re-added the timer, and it may have deleted the timer. + * But we assume that the handler has NOT kfree'ed the timer. We can assume this + * because calling del_timer() on a timer which is freed in the handler is a BUG. + */ + if (timer_pending(timer)) { + printk("del_timer(): timer was re-added\n"); + ret += detach_timer(timer); + timer->list.next = timer->list.prev = 0; + } + } } - +out: + spin_unlock_irqrestore(&timerlist_lock, flags); return ret; } -#endif - +#endif /* CONFIG_SMP */ static inline void cascade_timers(struct timer_vec *tv) { @@ -295,10 +358,22 @@ detach_timer(timer); timer->list.next = timer->list.prev = NULL; - timer_set_running(timer); +#ifdef CONFIG_SMP + if (running_timer != 0) { + printk("run_timer_list(%p): the timer is running!\n", timer); + BUG(); + } + running_timer = timer; + running_timer_cpu = smp_processor_id(); +#endif spin_unlock_irq(&timerlist_lock); fn(data); spin_lock_irq(&timerlist_lock); +#ifdef CONFIG_SMP + if (running_timer != timer) + BUG(); + running_timer = 0; +#endif goto repeat; } ++timer_jiffies; --- linux-2.4.0test1-ac2/include/linux/timer.h Mon May 15 21:24:15 2000 +++ linux-akpm/include/linux/timer.h Sun May 28 00:57:10 2000 @@ -52,11 +52,28 @@ unsigned long expires; unsigned long data; void (*function)(unsigned long); - volatile int running; }; extern void add_timer(struct timer_list * timer); extern int del_timer(struct timer_list * timer); +#define del_timer_sync(t) del_timer(t) +#define timer_exit(t) (void)(t) + +#ifdef CONFIG_SMP +extern int del_timer_async(struct timer_list * timer); +#else +#define del_timer_async(t) del_timer(t) +#endif + +#define INIT_TIMER_FND(fn, d) { \ + list: LIST_HEAD_INIT_NULL, \ + expires: 0, \ + data: d, \ + function: fn, \ +} + +#define INIT_TIMER_FN(fn) INIT_TIMER_FND(fn, 0) +#define INIT_TIMER INIT_TIMER_FN(0) /* * mod_timer is a more efficient way to update the expire field of an @@ -70,29 +87,12 @@ static inline void init_timer(struct timer_list * timer) { timer->list.next = timer->list.prev = NULL; -#ifdef CONFIG_SMP - timer->running = 0; -#endif } static inline int timer_pending (const struct timer_list * timer) { return timer->list.next != NULL; } - -#ifdef CONFIG_SMP -#define timer_exit(t) do { (t)->running = 0; mb(); } while (0) -#define timer_set_running(t) do { (t)->running = 1; mb(); } while (0) -#define timer_is_running(t) ((t)->running != 0) -#define timer_synchronize(t) while (timer_is_running(t)) barrier() -extern int del_timer_sync(struct timer_list * timer); -#else -#define timer_exit(t) (void)(t) -#define timer_set_running(t) (void)(t) -#define timer_is_running(t) (0) -#define timer_synchronize(t) do { (void)(t); barrier(); } while(0) -#define del_timer_sync(t) del_timer(t) -#endif /* * These inlines deal with timer wrapping correctly. You are --- linux-2.4.0test1-ac2/include/linux/list.h Wed Apr 26 22:52:03 2000 +++ linux-akpm/include/linux/list.h Sat May 27 22:26:43 2000 @@ -19,6 +19,8 @@ #define LIST_HEAD_INIT(name) { &(name), &(name) } +#define LIST_HEAD_INIT_NULL { 0, 0 } + #define LIST_HEAD(name) \ struct list_head name = LIST_HEAD_INIT(name) --- linux-2.4.0test1-ac2/kernel/ksyms.c Sat May 27 21:34:22 2000 +++ linux-akpm/kernel/ksyms.c Sat May 27 22:21:29 2000 @@ -341,9 +341,14 @@ EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax); EXPORT_SYMBOL(proc_doulongvec_minmax); -/* interrupt handling */ +/* kernel timers */ EXPORT_SYMBOL(add_timer); EXPORT_SYMBOL(del_timer); +#ifdef CONFIG_SMP +EXPORT_SYMBOL(del_timer_async); +#endif + +/* interrupt handling */ EXPORT_SYMBOL(request_irq); EXPORT_SYMBOL(free_irq); @@ -356,9 +361,6 @@ EXPORT_SYMBOL(autoirq_report); #endif -#ifdef CONFIG_SMP -EXPORT_SYMBOL(del_timer_sync); -#endif EXPORT_SYMBOL(mod_timer); EXPORT_SYMBOL(tq_timer); EXPORT_SYMBOL(tq_immediate);
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed May 31 2000 - 21:00:18 EST