Re: 2.6.12-rc2-mm3

Andrew Morton
2005-05-03T06:30:51Z

Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> Benjamin Herrenschmidt wrote:
> >
> > I'll have a look at the timer patch next week, they might have some
> > subtle race caused by a lack of memory barrier. I've had to debug some
> > of those in early timer code, and those are really nasty, they usually
> > only trigger under some subtle conditions, like ... heavy networking.
>
> Before this timer patch del_timer(pending_timer) worked as
> a memory barrier for the caller, now it does not.
>
> For example, sk_stop_timer() does:
>
> if (del_timer(timer))
> // no more wmb() here
> atomic_dec(&sk->sk_refcnt);
>
> I think that this particular case is ok, but may be there is
> some user of timers which lacks the memory barrier?
>
> ...
> --- 2.6.12-rc2+timer_patches/kernel/timer.c~ Sun Apr 24 11:59:31 2005
> +++ 2.6.12-rc2+timer_patches/kernel/timer.c Sun Apr 24 13:35:01 2005
> @@ -341,6 +341,9 @@ int del_timer(struct timer_list *timer)
> spin_unlock_irqrestore(&base->lock, flags);
> }
>
> + if (ret)
> + smp_wmb();
> +
> return ret;
> }
>
> @@ -387,6 +390,10 @@ unlock:
> spin_unlock_irqrestore(&base->lock, flags);
> } while (ret < 0);
>
> + if (ret)
> + smp_wmb();
> + smp_rmb();
> +
> return ret;
> }
>
> @@ -457,6 +464,7 @@ repeat:
>
> set_running_timer(base, timer);
> detach_timer(timer, 1);
> + smp_wmb();
> spin_unlock_irq(&base->t_base.lock);
> {
> u32 preempt_count = preempt_count();

I wonder if we should still do this? It would seem to be a safer approach.

(This barrier stuff continues to give me the creeps. Imagine being
dependent upon accidental barriers in the add/del/mod_timer code. Ugh).