lkml.org 
[lkml]   [2009]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2009-08-28 01:13    [W:0.602 / U:0.692 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site