On Mon, Mar 30, 2015 at 05:32:16PM +0530, Viresh Kumar wrote:
On 29 March 2015 at 15:54, Peter Zijlstra peterz@infradead.org wrote:
What I didn't say, but had thought of is that __run_timer() should skip any timer that has RUNNING set -- for obvious reasons :-)
Below is copied from your first reply, and so you probably already said that ? :)
Also, once you have tbase_running, we can take base->running_timer out altogether.
No, I means something else with that. We can remove the tvec_base::running_timer field. Everything that uses that can use tbase_running() AFAICT.
I wanted to clarify if I understood it correctly..
Are you saying that:
Case 2.) we keep retrying for it, until the time the other handler finishes?
That.
If we remove it from the list before we call ->fn. Therefore, even if migrate happens, it will not see a RUNNING timer entry, seeing how its not actually on any lists.
The only way to get on a list while running is if ->fn() requeues itself _on_another_base_. When that happens, we need to wait for it to complete running.
Case 2.) We kept waiting for the first handler to finish ..
- cpuY may waste some cycles as it kept waiting for handler to finish on cpuX ..
True, rather silly to requeue a timer on the same jiffy as its already running through, but yes, an unlikely possibility.
You can run another timer while we wait -- if there is another of course.
- We may need to perform base unlock/lock on cpuY, so that cpuX can take cpuY's
lock to reset tbase_running. And that might be racy, not sure.
Drop yes, racy not so much I think.
diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 2d3f5c504939..1394f9540348 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -1189,12 +1189,39 @@ static inline void __run_timers(struct tvec_base *base) cascade(base, &base->tv5, INDEX(3)); ++base->timer_jiffies; list_replace_init(base->tv1.vec + index, head); + +again: while (!list_empty(head)) { void (*fn)(unsigned long); unsigned long data; bool irqsafe;
- timer = list_first_entry(head, struct timer_list,entry); + timer = list_first_entry(head, struct timer_list, entry); + if (unlikely(tbase_running(timer))) { + /* Only one timer on the list, force wait. */ + if (unlikely(head->next == head->prev)) { + spin_unlock(&base->lock); + + /* + * The only way to get here is if the + * handler requeued itself on another + * base, this guarantees the timer will + * not go away. + */ + while (tbase_running(timer)) + cpu_relax(); + + spin_lock(&base->lock); + } else { + /* + * Otherwise, rotate the list and try + * someone else. + */ + list_move_tail(&timer->entry, head); + } + goto again; + } + fn = timer->function; data = timer->data; irqsafe = tbase_get_irqsafe(timer->base);