On 27 November 2012 19:17, Steven Rostedt rostedt@goodmis.org wrote:
On Tue, 2012-11-06 at 16:08 +0530, Viresh Kumar wrote:
diff --git a/kernel/timer.c b/kernel/timer.c @@ -729,6 +730,12 @@ __mod_timer(struct timer_list *timer, unsigned long expires,
base = lock_timer_base(timer, &flags);
if (timer->sched_del) {/* Don't schedule it again, as it is getting deleted */ret = -EBUSY;goto out_unlock;}ret = detach_if_pending(timer, base, false); if (!ret && pending_only) goto out_unlock;@@ -746,21 +753,12 @@ __mod_timer(struct timer_list *timer, unsigned long expires, new_base = per_cpu(tvec_bases, cpu);
if (base != new_base) {
/** We are trying to schedule the timer on the local CPU.* However we can't change timer's base while it is running,* otherwise del_timer_sync() can't detect that the timer's* handler yet has not finished. This also guarantees that* the timer is serialized wrt itself.*/if (likely(base->running_timer != timer)) {/* See the comment in lock_timer_base() */timer_set_base(timer, NULL);spin_unlock(&base->lock);base = new_base;spin_lock(&base->lock);timer_set_base(timer, base);}
/* See the comment in lock_timer_base() */timer_set_base(timer, NULL);spin_unlock(&base->lock);base = new_base;spin_lock(&base->lock);timer_set_base(timer, base); }
I don't think this is good enough. For one thing, it doesn't handle try_to_del_timer_sync() or even del_timer_sync() for that matter. As that may return success when the timer happens to be running on another CPU.
We have this:
CPU0 CPU1 ---- ----timerA (running) mod_timer(timerA) [ migrate to CPU2 ] release timer base lock del_timer_sync(timerA) timer->sched_del = true try_to_del_timer_sync(timerA) base(CPU2)->timer != timerA [TRUE!] timerA (finishes)
Fail!
Hi Steven/Thomas,
I came back to this patch after completing some other stuff and posting wq part of this patchset separately.
I got your point and understand how this would fail.
@Thomas: I need your opinion first. Do you like this concept of migrating running timer or not? Or you see some basic problem with this concept?
If no (i.e. i can go ahead with another version), then i have some solution to fix earlier problems reported by Steven:
The problem lies with del_timer_sync() which just checks base->running_timer != timer to check if timer is currently running or not.
What if we add another variable in struct timer_list, that will store if we are running timer callback or not. And so, before we call callback in timer core, we will set this variable and will reset it after finishing callback.
del_timer_sync() will have something like:
if (base->running_timer != timer) remove timer and return; else if (timer->running_callback) go back to its loop...
So, with my existing patch + this change, del_timer_sync() will not return back unless the callback is completed on CPU0.
But what can happen now is base->running_timer == timer can be true for two cpus simultaneously cpu0 (running callback) and cpu2 (running hardware timer). Will that cause any issues?
-- viresh