hrtimer: drop active hrtimer checks after adding it

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

First patch adds a WARN_ON_ONCE() to __hrtimer_start_range_ns() to make sure hrtimers are always enqueued from it. Next 6 patches update several parts of kernel to drop calls to hrtimer_active() after starting a hrtimer.

Rebased over 3.16-rc4 and pushed here:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git hrtimer/drop-hrtimer-active-calls

Viresh Kumar (7):
hrtimer: Warn if hrtimer_start*() failed to enqueue hrtimer
hrtimer: don't check for active hrtimer after adding it
tick: don't check for active hrtimer after adding it
sched: don't check for active hrtimer after adding it
futex: don't check for active hrtimer after adding it
rtmutex: don't check for active hrtimer after adding it
net: don't check for active hrtimer after adding it

 kernel/futex.c           |  5 +----
 kernel/hrtimer.c         |  6 ++----
 kernel/locking/rtmutex.c |  5 +----
 kernel/sched/core.c      | 20 +++++++++-----------
 kernel/sched/deadline.c  |  2 +-
 kernel/time/tick-sched.c | 45 ++++++++++++++++++---------------------------
 net/core/pktgen.c        |  2 --
 7 files changed, 32 insertions(+), 53 deletions(-)

2.0.0.rc2

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in kernel we check if hrtimer is enqueued properly with hrtimer_active(). This isn't required and can be dropped.

Before fixing that, lets make sure hrtimer is always enqueued properly by adding

	WARN_ON_ONCE(!hrtimer_active(timer));

towards the end of __hrtimer_start_range_ns().

---
 kernel/hrtimer.c | 2 ++
 1 file changed, 2 insertions(+)

index 3ab2899..cf40209 100644
@@ -1037,6 +1037,8 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 
 	unlock_hrtimer_base(timer, &flags);
 
+	/* Make sure timer is enqueued */
+	WARN_ON_ONCE(!hrtimer_active(timer));
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__hrtimer_start_range_ns);

2.0.0.rc2

Haha, I didn't even go down to this patch after reading 0/N. I knew right there that it's going to be pointless shite.

But now that you point me to it, it's even worse. It's not only pointless shite it's actively wrong and outright stupid for someone who tries to "work" on this code for a couple of month now.

Viresh, I'm really tired of this. Stop touching code you do not understand. I warned you more than once and now you really reached the level of complete incompetence. Welcome to my killfile.

Thanks,

tglx

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

This patch updates hrtimer core to get this fixed.

---
 kernel/hrtimer.c | 4 ----
 1 file changed, 4 deletions(-)

index cf40209..a76f962 100644
@@ -1555,8 +1555,6 @@ static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mod
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		hrtimer_start_expires(&t->timer, mode);
-		if (!hrtimer_active(&t->timer))
-			t->task = NULL;
 
 		if (likely(t->task))
 			freezable_schedule();

@@ -1837,8 +1835,6 @@ schedule_hrtimeout_range_clock(ktime_t *expires, unsigned long delta,
 	hrtimer_init_sleeper(&t, current);
 
 	hrtimer_start_expires(&t.timer, mode);
-	if (!hrtimer_active(&t.timer))
-		t.task = NULL;
 
 	if (likely(t.task))
 		schedule();

2.0.0.rc2

Hi Viresh,

Included following diff to this patch:

index a76f962..ae7b5cf 100644
@@ -1556,8 +1556,7 @@ static int __sched do_nanosleep(struct

hrtimer_sleeper *t, enum hrtimer_mode mod

                set_current_state(TASK_INTERRUPTIBLE);
                hrtimer_start_expires(&t->timer, mode);

- if (likely(t->task))
- freezable_schedule();
+ freezable_schedule();

                hrtimer_cancel(&t->timer);
                mode = HRTIMER_MODE_ABS;

@@ -1836,8 +1835,7 @@ schedule_hrtimeout_range_clock(ktime_t *expires, unsigned long delta,

        hrtimer_start_expires(&t.timer, mode);

- if (likely(t.task))
- schedule();
+ schedule();

        hrtimer_cancel(&t.timer);
        destroy_hrtimer_on_stack(&t.timer);

Latest changes are pushed to my branch in case someone is looking to test them.

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

This is done in while loop at several places, which isn't required anymore.

This patch updates tick core to get this fixed. This also removes the confusing comment present over usage of hrtimer_active().

- /* Check, if the timer was already in the past */

This comment says that hrtimer_active() would check if timer was already in the past, but hrtimer_active() can't check it. It just checks if timer is in INACTIVE state or not.

---
 kernel/time/tick-sched.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

index 6558b7a..66ca5ab 100644
@@ -658,9 +658,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
 		if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
 			hrtimer_start(&ts->sched_timer, expires,
 				      HRTIMER_MODE_ABS_PINNED);
-			/* Check, if the timer was already in the past */
-			if (hrtimer_active(&ts->sched_timer))
-				goto out;
+			goto out;
 		} else if (!tick_program_event(expires, 0))
 				goto out;
 		/*

@@ -844,24 +842,25 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
 	hrtimer_cancel(&ts->sched_timer);
 	hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
 
-	while (1) {
-		/* Forward the time to expire in the future */
-		hrtimer_forward(&ts->sched_timer, now, tick_period);
+	/* Forward the time to expire in the future */
+	hrtimer_forward(&ts->sched_timer, now, tick_period);
 
-		if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
-			hrtimer_start_expires(&ts->sched_timer,
-					      HRTIMER_MODE_ABS_PINNED);
-			/* Check, if the timer was already in the past */
-			if (hrtimer_active(&ts->sched_timer))
-				break;
-		} else {
-			if (!tick_program_event(
-				hrtimer_get_expires(&ts->sched_timer), 0))
-				break;
-		}
+	if (ts->nohz_mode == NOHZ_MODE_HIGHRES) {
+		hrtimer_start_expires(&ts->sched_timer,
+				      HRTIMER_MODE_ABS_PINNED);
+		return;
+	}
+
+	while (1) {
+		if (!tick_program_event(hrtimer_get_expires(&ts->sched_timer),
+					0))
+			break;
 		/* Reread time and update jiffies */
 		now = ktime_get();
 		tick_do_update_jiffies64(now);
+
+		/* Forward the time to expire in the future */
+		hrtimer_forward(&ts->sched_timer, now, tick_period);
 	}
 }
 

@@ -1104,7 +1103,6 @@ early_param("skew_tick", skew_tick);
 void tick_setup_sched_timer(void)
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
-	ktime_t now = ktime_get();
 
 	/*
 	 * Emulate tick processing via per-CPU hrtimers:

@@ -1123,15 +1121,8 @@ void tick_setup_sched_timer(void)
 		hrtimer_add_expires_ns(&ts->sched_timer, offset);
 	}
 
-	for (;;) {
-		hrtimer_forward(&ts->sched_timer, now, tick_period);
-		hrtimer_start_expires(&ts->sched_timer,
-				      HRTIMER_MODE_ABS_PINNED);
-		/* Check, if the timer was already in the past */
-		if (hrtimer_active(&ts->sched_timer))
-			break;
-		now = ktime_get();
-	}
+	hrtimer_forward(&ts->sched_timer, ktime_get(), tick_period);
+	hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
 
 #ifdef CONFIG_NO_HZ_COMMON
 	if (tick_nohz_enabled) {

2.0.0.rc2

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

Also this is done in while loop at several places, which isn't required anymore.

This patch updates sched core to get this fixed.

---
 kernel/sched/core.c     | 20 +++++++++-----------
 kernel/sched/deadline.c |  2 +-
 2 files changed, 10 insertions(+), 12 deletions(-)

index 3bdf01b..4a6ef8d 100644
@@ -111,19 +111,17 @@ void start_bandwidth_timer(struct hrtimer *period_timer, ktime_t period)
 	unsigned long delta;
 	ktime_t soft, hard, now;
 
-	for (;;) {
-		if (hrtimer_active(period_timer))
-			break;
+	if (hrtimer_active(period_timer))
+		return;
 
-		now = hrtimer_cb_get_time(period_timer);
-		hrtimer_forward(period_timer, now, period);
+	now = hrtimer_cb_get_time(period_timer);
+	hrtimer_forward(period_timer, now, period);
 
-		soft = hrtimer_get_softexpires(period_timer);
-		hard = hrtimer_get_expires(period_timer);
-		delta = ktime_to_ns(ktime_sub(hard, soft));
-		__hrtimer_start_range_ns(period_timer, soft, delta,
-					 HRTIMER_MODE_ABS_PINNED, 0);
-	}
+	soft = hrtimer_get_softexpires(period_timer);
+	hard = hrtimer_get_expires(period_timer);
+	delta = ktime_to_ns(ktime_sub(hard, soft));
+	__hrtimer_start_range_ns(period_timer, soft, delta,
+				 HRTIMER_MODE_ABS_PINNED, 0);
 }
 
 DEFINE_MUTEX(sched_domains_mutex);

index fc4f98b1..eeb6786 100644
@@ -484,7 +484,7 @@ static int start_dl_timer(struct sched_dl_entity *dl_se, bool boosted)
 	__hrtimer_start_range_ns(&dl_se->dl_timer, soft,
 				 range, HRTIMER_MODE_ABS, 0);
 
-	return hrtimer_active(&dl_se->dl_timer);
+	return 1;
 }
 
 /*

2.0.0.rc2

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

This patch updates futex core to get this fixed.

---
 kernel/futex.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index b632b5f..4847efc 100644
@@ -2083,11 +2083,8 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
 	queue_me(q, hb);
 
 	/* Arm the timer */
-	if (timeout) {
+	if (timeout)
 		hrtimer_start_expires(&timeout->timer, HRTIMER_MODE_ABS);
-		if (!hrtimer_active(&timeout->timer))
-			timeout->task = NULL;
-	}
 
 	/*
 	 * If we have been removed from the hash list, then another task

2.0.0.rc2

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

This patch updates rtmutex core to get this fixed.

---
 kernel/locking/rtmutex.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index fc60594..9ea8830 100644
@@ -922,11 +922,8 @@ rt_mutex_slowlock(struct rt_mutex *lock, int state,
 	set_current_state(state);
 
 	/* Setup the timer, when timeout != NULL */
-	if (unlikely(timeout)) {
+	if (unlikely(timeout))
 		hrtimer_start_expires(&timeout->timer, HRTIMER_MODE_ABS);
-		if (!hrtimer_active(&timeout->timer))
-			timeout->task = NULL;
-	}
 
 	ret = task_blocks_on_rt_mutex(lock, &waiter, current, detect_deadlock);
 

2.0.0.rc2

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The only special case is when the hrtimer was in past. If it is getting enqueued to local CPUs's clock-base, we raise a softirq and exit, else we handle that on next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().

This patch updates net core to get this fixed.

---
 net/core/pktgen.c | 2 --
 1 file changed, 2 deletions(-)

index fc17a9d..f911acd 100644
@@ -2186,8 +2186,6 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 		do {
 			set_current_state(TASK_INTERRUPTIBLE);
 			hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
-			if (!hrtimer_active(&t.timer))
-				t.task = NULL;
 
 			if (likely(t.task))
 				schedule();

2.0.0.rc2

Hi Viresh,

--Chris

Hi Chris,

Yeah, it looks like we can get rid of this. Also,

        } while (t.task && pkt_dev->running && !signal_pending(current));

is present in the closing "}" of do-while loop and probably we don't need to check t.task here as well.

And this review comment applies to patch 2/7 as well:
hrtimer: don't check for active hrtimer after adding it

I would still wait for somebody to prove us wrong :), and will resend it next week only.

Thanks.

Yeah, no worries. I just happened to read it and not knowing any of the APIs had to look up what is going on.

BTW, I *will* get back to you about that broadcast stuff when I get back to it myself. Other priorities at the moment again.

--Chris

Actually No. t.task is modified from hrtimer handler and so this check would stay:

Diff I have added to this patch:

index f911acd..cc2694e 100644
@@ -2187,8 +2187,7 @@ static void spin(struct pktgen_dev *pkt_dev,

ktime_t spin_until)

                        set_current_state(TASK_INTERRUPTIBLE);
                        hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);

- if (likely(t.task))
- schedule();
+ schedule();

                        hrtimer_cancel(&t.timer);
                } while (t.task && pkt_dev->running &&

!signal_pending(current));

On Wed, 9 Jul 2014, Viresh Kumar wrote:

So your patch series drops active hrtimer checks after adding it, according to your subject line.

Quite useeul to drop something after adding it, right?

Wrong as usual.

It's a common pattern that short timeouts are given which lead to immediate expiry so the extra round through schedule is even more pointless than the extra check.

Aside of that it's a long discussed issue that we really should tell the caller right away that the timer was setup in the past and not enqueued at all.

That requires to fixup a few call sites, but that'd far more valuable than removing a few assumed to be pointless checks.

Thnaks,

tglx

It may be a common pattern but it's not obvious at all as is in the code except for timers gurus.

It looks like error handling while it's actually an optimization.

Also what about this pattern when it's used in interrupt or interrupt-disabled code? In this case the handler is not going to fire right away, unless it's enqueued on another CPU for unpinned timers.

For example this code in tick_nohz_stop_sched_tick():

          hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
          /* Check, if the timer was already in the past */
          if (hrtimer_active(&ts->sched_timer))
                 goto out;

It's not clear what this is handling. Concurrent immediate callback expiration from another CPU? But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active() check...

Hi Thomas,

I meant "hrtimer" by "it". Will fix it in case this patchset is still required.

I cross-checked this with Frederic and Preeti before reaching out to you, to make sure its not 'obviously stupid'. And still couldn't get it right. :(

Just wanted to confirm it again, you are talking about CPU being interrupted by clockevent device's interrupt right after hrtimer_start*() returns and before calling hrtimer_active()?

Actually I was concerned about other cases as well.

- Timeouts

I do agree that an extra check is better than an extra round of schedule(). But this is already achieved without calling hrtimer_active(), isn't it?

All these timeout hrtimers have hrtimer_wakeup() as there handler (as these are initialized with: hrtimer_init_sleeper()).

And on expiration hrtimer_wakeup() does this: t->task = NULL;

So would this extra call to hrtimer_active() make any difference?

- Process-context: sched changes

I am not sure if scheduler routines: start_bandwidth_timer() and start_dl_timer() would get called *only* with interrupts disabled.

But, it doesn't look obvious that the optimization Thomas mentioned earlier is relevant here as well. These might be added here for error checking.

I might be wrong here as I don't have any understanding of this code and so sorry in advance.

Note: My tree is monitored by kbuild-bot and these changes are under testing for over a week now. And I haven't received any reports of the WARN() firing in __hrtimer_start_range_ns().. Probably these short timeouts aren't getting hit at all by bot's tests.

--
viresh