Re: [RFC] [PATCH 1/1] hrtimers: Cache next hrtimer

[Thomas Gleixner]

On Thu, 27 Aug 2009, Ashwin Chaugule wrote:
>    Cached the hrtimer which causes the expire_next
>    value to change. This is to avoid unnecessary re-programming
>    of the clock events device.

You forgot to describe the application scenario which triggers this.
 
> @@ -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.

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.

Thanks,

	tglx
---
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;
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/