Messages in this thread | | | Date | Thu, 27 Aug 2009 19:09:57 -0400 | From | Ashwin Chaugule <> | Subject | Re: [RFC] [PATCH 1/1] hrtimers: Cache next hrtimer |
| |
Thomas Gleixner wrote: > You forgot to describe the application scenario which triggers this. > > I didn't have anything specific running in userspace to trigger this. The sched_timer itself was causing most of the unnecessary reprogramming. I reckon, with more applications running, the timer_stats will show other timers (hrtimer_wakeup, it_real_fn etc.) that cause this effect too. >> @@ -843,16 +851,20 @@ static void __remove_hrtimer(struct hrtimer *timer, >> struct hrtimer_clock_base *base, >> unsigned long newstate, int reprogram) >> { >> - if (timer->state & HRTIMER_STATE_ENQUEUED) { >> + struct hrtimer *next_hrtimer = __get_cpu_var(hrtimer_bases).next_hrtimer; >> + >> + if (hrtimer_is_queued(timer)) { >> /* >> * Remove the timer from the rbtree and replace the >> * first entry pointer if necessary. >> */ >> if (base->first == &timer->node) { >> base->first = rb_next(&timer->node); >> - /* Reprogram the clock event device. if enabled */ >> - if (reprogram && hrtimer_hres_active()) >> - hrtimer_force_reprogram(base->cpu_base); >> + if (next_hrtimer == timer) { >> + /* Reprogram the clock event device. if enabled */ >> + if (reprogram && hrtimer_hres_active()) >> + hrtimer_force_reprogram(base->cpu_base); >> + } >> } >> rb_erase(&timer->node, &base->active); >> > > So if I'm not totally on the wrong track, that's the meat of the > patch. > Yup. > Any reason why we can't solve that problem with checking > cpu_base->expires_next against the timer which is deleted ? > > See the patently untested patch below. > > Another question which arises is whether we should bother with the > reprogramming at all and just let the last programmed event happen > even when the corresponding timer has been removed. > Hm. Interesting approach. See below. > --- > diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c > index 49da79a..91d099c 100644 > --- a/kernel/hrtimer.c > +++ b/kernel/hrtimer.c > @@ -906,19 +906,30 @@ static void __remove_hrtimer(struct hrtimer *timer, > struct hrtimer_clock_base *base, > unsigned long newstate, int reprogram) > { > - if (timer->state & HRTIMER_STATE_ENQUEUED) { > - /* > - * Remove the timer from the rbtree and replace the > - * first entry pointer if necessary. > - */ > - if (base->first == &timer->node) { > - base->first = rb_next(&timer->node); > - /* Reprogram the clock event device. if enabled */ > - if (reprogram && hrtimer_hres_active()) > - hrtimer_force_reprogram(base->cpu_base); > - } > - rb_erase(&timer->node, &base->active); > - } > + ktime_t expires; > + > + if (!(timer->state & HRTIMER_STATE_ENQUEUED)) > + goto out; > + > + /* > + * Remove the timer from the rbtree and replace the first > + * entry pointer if necessary. > + */ > + rb_erase(&timer->node, &base->active); > + > + if (base->first != &timer->node) > + goto out; > + > + base->first = rb_next(&timer->node); > + /* Reprogram the clock event device. if enabled */ > + if (!reprogram || !hrtimer_hres_active()) > + goto out; > + > + expires = ktime_sub(hrtimer_get_expires(timer), base->offset); > + if (base->cpu_base->expires_next.tv64 == expires.tv64) > + hrtimer_force_reprogram(base->cpu_base); > + > +out: > timer->state = newstate; > } > >
So, you suggest checking the ktime of the hrtimer thats about to expire and compare it with expires_next ? I guess, another reason to go with caching the hrtimer is to avoid looping through HRTIMER_MAX_CLOCK_BASES, which may increase to more than 2 (?) for other architectures, and also all the code flow to arm the clock events device. With the caching approach, I also saw a 4% speedup in various application startups too.
Cheers, Ashwin
| |