Re: [PATCH] fix __mod_timer vs __run_timers deadlock.

Maneesh Soni
2005-05-02T04:10:36Z

On Sun, May 01, 2005 at 11:55:33AM +0400, Oleg Nesterov wrote:
> The bug was identified by Maneesh Soni.
>

Thanks, to Sharyathi too as he is the first to see the problem and
provided kdump to us. I just did initial kdump analysis.

Sharyathi, it would be great if you can test the following patch in your
setup.

Thanks
Maneesh

> When __mod_timer() changes timer's base it waits for the completion
> of timer->function. It is just stupid: the caller of __mod_timer()
> can held locks which would prevent completion of the timer's handler.
>
> Solution: do not change the base of the currently running timer.
>
> Side effect: __mod_timer() doesn't garantees anymore that timer will
> run on the local cpu.
>
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
> --- rc2-mm3/kernel/timer.c~ 2005-04-30 18:43:56.000000000 +0400
> +++ rc2-mm3/kernel/timer.c 2005-05-01 14:29:02.000000000 +0400
> @@ -212,41 +212,39 @@ int __mod_timer(struct timer_list *timer
> timer_base_t *base;
> tvec_base_t *new_base;
> unsigned long flags;
> - int ret = -1;
> + int ret;
>
> BUG_ON(!timer->function);
> check_timer(timer);
> -
> - do {
> - base = lock_timer_base(timer, &flags);
> - new_base = &__get_cpu_var(tvec_bases);
> -
> - /* Ensure the timer is serialized. */
> - if (base != &new_base->t_base
> - && base->running_timer == timer)
> - goto unlock;
> -
> - ret = 0;
> - if (timer_pending(timer)) {
> - detach_timer(timer, 0);
> - ret = 1;
> - }
> -
> - if (base != &new_base->t_base) {
> - timer->base = NULL;
> - /* Safe: the timer can't be seen via ->entry,
> - * and lock_timer_base checks ->base != 0. */
> - spin_unlock(&base->lock);
> - base = &new_base->t_base;
> - spin_lock(&base->lock);
> - timer->base = base;
> - }
> -
> - timer->expires = expires;
> - internal_add_timer(new_base, timer);
> -unlock:
> - spin_unlock_irqrestore(&base->lock, flags);
> - } while (ret < 0);
> +
> + base = lock_timer_base(timer, &flags);
> +
> + ret = 0;
> + if (timer_pending(timer)) {
> + detach_timer(timer, 0);
> + ret = 1;
> + }
> +
> + new_base = &__get_cpu_var(tvec_bases);
> +
> + if (base != &new_base->t_base) {
> + if (unlikely(base->running_timer == timer))
> + /* Don't change timer's base while it is running.
> + * Needed for serialization of timer wrt itself. */
> + new_base = container_of(base, tvec_base_t, t_base);
> + else {
> + timer->base = NULL;
> + /* Safe: the timer can't be seen via ->entry,
> + * and lock_timer_base checks ->base != 0. */
> + spin_unlock(&base->lock);
> + spin_lock(&new_base->t_base.lock);
> + timer->base = &new_base->t_base;
> + }
> + }
> +
> + timer->expires = expires;
> + internal_add_timer(new_base, timer);
> + spin_unlock_irqrestore(&new_base->t_base.lock, flags);
>
> return ret;
> }

--
Maneesh Soni
Linux Technology Center,
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990