disable irq's and check need_resched before safe_halt

While sending interrupts to a cpu to repeatedly wake a thread, on occasion that thread will take a full timer tick cycle (4002 usec in my case) to wakeup.

The problem concerns a race condition in the code around the safe_halt() call in the default_idle() routine. Setting 'nohalt' on the kernel command line causes the long wakeups to disappear.

void
default_idle (void)

{
        local_irq_enable();
        while (!need_resched()) {

--> if (can_do_pal_halt)
--> safe_halt();
else

A timer tick could arrive between the check for !need_resched and the actual call to safe_halt() (which does a pal call to PAL_HALT_LIGHT). By the time the timer tick completes, a thread that might now need to run could get held up for as long as a timer tick waiting for the halted cpu.

I'm proposing that we disable irq's and check need_resched again before calling safe_halt(). Does anyone see any problem with this approach?

Index: linux/arch/ia64/kernel/process.c
=================================--- linux.orig/arch/ia64/kernel/process.c 2007-08-02 15:05:56.427236082 -0500
+++ linux/arch/ia64/kernel/process.c 2007-08-06 19:42:20.147944967 -0500
@@ -198,9 +198,13 @@ default_idle (void)
{
local_irq_enable();
while (!need_resched()) {
- if (can_do_pal_halt)
- safe_halt();
- else
+ if (can_do_pal_halt) {
+ local_irq_disable();
+ if (!need_resched()) {
+ safe_halt();
+ }
+ local_irq_enable();
+ } else
cpu_relax();
}
}

This looks like it re-introduces code that Ken Chen backed out about two years ago. Here's Ken's commit that explains what broke last time we made the idle loop look like this. Now that code was wider ranging ... messing with TIF bits too, so maybe this is different this time?

-Tony

commit 1e185b97b4364063f1135604b87f8d8469944233

Author: Chen, Kenneth W <[email protected]>
Date:   Tue Nov 15 14:37:05 2005 -0800

[PATCH] ia64: cpu_idle performance bug fix

Our performance validation on 2.6.15-rc1 caught a disastrous performance regression on ia64 with netperf (-98%) and volanomark (-58%) compares to previous kernel version 2.6.14-git7. See the following chart (result group 1 & 2).

http://kernel-perf.sourceforge.net/results.machine_id&.html

We have root caused it to commit 64c7c8f88559624abdbe12b5da6502e8879f8d28

This changeset broke the ia64 task resched notification. In sched.c:resched_task(), a reschedule IPI is conditioned upon TIF_POLLING_NRFLAG. However, the above changeset unconditionally set the polling thread flag for idle tasks regardless whether pal_halt_light is in use or not. As a result, resched IPI is not sent from resched_task(). And since the default behavior on ia64 is to use pal_halt_light, we end up delaying the rescheduling task until next timer tick, and thus cause the performance regression.

This fixes the performance bug. I'm glad our performance suite is turning up bad performance bug like this in time.

index e92ea64..4305d2b 100644
@@ -202,12 +202,9 @@ default_idle (void)
 {
 	local_irq_enable();
 	while (!need_resched()) {
-		if (can_do_pal_halt) {
-			local_irq_disable();
-			if (!need_resched())
-				safe_halt();
-			local_irq_enable();
-		} else
+		if (can_do_pal_halt)
+			safe_halt();
+		else
 			cpu_relax();
 	}
 }

@@ -272,10 +269,14 @@ cpu_idle (void)
 {
 	void (*mark_idle)(int) = ia64_mark_idle;
   	int cpu = smp_processor_id();
-	set_thread_flag(TIF_POLLING_NRFLAG);
 
 	/* endless idle loop with no priority at all */
 	while (1) {
+		if (can_do_pal_halt)
+			clear_thread_flag(TIF_POLLING_NRFLAG);
+		else
+			set_thread_flag(TIF_POLLING_NRFLAG);
+
 		if (!need_resched()) {
 			void (*idle)(void);
 #ifdef CONFIG_SMP

Yeah, I think the TIF flag was the key in fixing the resched IPI notification. The change in default_idle() is an optimization.

I'm horrified to see the same code coming back: doing interrupt enable/disable in the most inner while loop. Disable interrupt is just crude, but I suppose that's the only way to resolve the race condition? Looking at other arch like x86_64, it is also doing the same thing.

- Ken

If anyone can suggest a better alternative to fix this race condition, I'd certainly consider it.

I suppose one alternative might be to move the local_irq_enable() down into the default_idle loop so that we don't have to enable and disable irq's the first time through if pal_halt will be called.

..
..

And the problem with the former hunk is that it reintroduces the race between checking !need_resched() and receiving a timer interrupt before safe_halt(). You could have a thread needing execution at the time the cpu enters LIGHT HALT. If irq's are disabled, LIGHT HALT will return relatively quickly with the pending timer interrupt, rather than having to wait for the next one.

Yup. x86 does pretty much the same thing.

I had the perf. team run the problem benchmarks (netperf and volanomark) with this patch applied ... and they saw no performance difference.

The patch is bundled to go in with the next batch that I send to Linus.

-Tony