Re: PATCH: Race in 2.6.0-test2 timer code

[linas@austin.ibm.com]

Hi Mingo,

On Wed, Jul 30, 2003 at 07:57:32AM +0200, Ingo Molnar wrote:
> 
> On Tue, 29 Jul 2003, Andrew Morton wrote:
> > Andrea says that we need to take the per-timer lock in add_timer() and
> > del_timer(), but I haven't yet got around to working out why.
> 
> this makes no sense - in 2.6 (and in 2.4) there's no safe add_timer() /
> del_timer() use without using external SMP synchronization. (There's one
> special timer use variant involving del_timer_sync() that was safe in 2.4
> but is unsafe in 2.6, see below.)

I don't understand why you don't like this, since your patch seems to 
acheive the same results as Andrea's patch (he uses timer->lock to 
serialize add_timer() and del_timer(), where as your patch modifies
add_timer so that it grabs locks on old_base as well as new_base;
either approach should fix the add_timer/del_timer race.)

> What i'd propose is the attached (tested, against 2.6.0-test2) patch
> instead. It unifies the functionality of add_timer() and mod_timer(), and
> makes any combination of the timer API calls completely SMP-safe.  
> del_timer() is still not using the timer lock.
> 
> this patch fixes the only timer bug in 2.6 i'm aware of: the
> del_timer_sync() + add_timer() combination in kernel/itimer.c is buggy.
> This was correct code in 2.4, because there it was safe to do an
> add_timer() from the timer handler itself, parallel to a del_timer_sync().  
> If we want to make this safe in 2.6 too (which i think we want to) then we
> have to make add_timer() almost equivalent to mod_timer(), locking-wise.
> And once we are at this point i think it's much cleaner to actually make
> add_timer() a variant of mod_timer(). (There's no locking cost for
> add_timer(), only the cost of an extra branch. And we've removed another
> commonly used function from the icache.)

Well, I'm confused by this a bit too, now.  I saw this bug in 2.4 and I 
thought that Andrea was implying that it couldn't happen in 2.6. 
He seemed to be saying that the del_timer_sync() + add_timer() race
can happen only in 2.4, where add_timer() is running on the 'wrong' cpu 
bacuase it got there through the evil run_all_timers()/TIMER_BH.  Since 
there's no run_all_timers() in 2.6, he seemed to imply that the race 
'couldn't happen'.  Is he right?  

So, to add to my 'stupid question' quota of the day: the only way that
a timer could run on a CPU other than what it was added on would be for 
a softirq to somehow get moved to a different cpu, and that is impossible,
right?

> Linas, could you please give this patch a go, does it make a difference to
> your timer list corruption problem? I've booted it on SMP and UP as well.

Still trying ... but after reading it, it looks like it will fix my 2.4 bug.  

--linas