kuznet@xxxxxxxxxxxxx wrote:
>
> Hello!
>
> > Why does the handler have to call timer_exit() at all?
> >
> > Could we not clear timer->running in run_timer_list()? That would
> > certainly protect us from the problem you identify...
>
> Timers are self-destructable as rule. See? Normal usage
> for timer is to have it allocated inside an object and
> timer event detroys the object together with timer.
> In this case we have to use refcounts external to timer
> to avoid races.
>
> Actually, existing *_timer primitives are very inconvenient.
> And I did not find any good way to improve them. Essentially,
> del_timer_sync(), timer->running and mod_timer() returning
> value are all that I was able to do.
>
I think there's a race in the timer code at present:
int del_timer_sync(struct timer_list * timer)
{
int ret = 0;
for (;;) {
unsigned long flags;
int running;
spin_lock_irqsave(&timerlist_lock, flags);
** The timer handler could be running now. It can delete the
timer and kfree it, or reuse its memory for something else,
or turn it into a semantically different timer **
ret += detach_timer(timer);
timer->list.next = timer->list.prev = 0;
** uh-oh **
Still, my immediate concern is this:
I'll be spending the next <however long it takes> working through the
old net drivers. One very common theme/bug in these is the pattern:
xxx_close()
{
...
del_timer();
release(some_resources);
...
}
xxx_timer()
{
use(some_resources);
}
These need to be turned into del_timer_sync()'s [1]. This means I have
to add timer_exit() calls as well. I'd like you to confirm that we
cannot move the timer_exit() funtionality into run_timer_list():
static inline void run_timer_list(void)
{
...
timer->list.next = timer->list.prev = NULL;
timer_set_running(timer);
spin_unlock_irq(&timerlist_lock);
fn(data);
+ timer->running = 0;
spin_lock_irq(&timerlist_lock);
goto repeat;
Are you saying that we can't do this because we should not touch the
timer after its handler has executed? (Even though del_timer_sync can
do this - see above :))
[1]: del_timer_sync() is only needed if the driver is to support SMP.
With a lot of the old ISA drivers this is a lost cause. The probability
of breaking the driver for UP is sufficiently high, and the usefulness
of making it SMP-aware is sufficiently low that we should simply say
#ifdef CONFIG_SMP
#error This driver does not support SMP
#endif
I'll be doing this if the SMP fixes look risky.
--
-akpm-
|