Re: [PATCH] idle using PNI monitor/mwait (take 2)

[Jamie Lokier]

Nakajima, Jun wrote:
> We are doing this as defensive programming (because of bogus device
> drivers, for example), like the other idle routines (default_idle, and
> poll_idle) always do. 
> 
> BTW, I'm not sure that local_irq_disable() is really required below (as
> you know, "sti" is hiding in safe_halt()).
> 
> void default_idle(void)
> {
> 	if (!hlt_counter && current_cpu_data.hlt_works_ok) {
> =>		local_irq_disable();
> 		if (!need_resched())
> 			safe_halt();
> 		else
> 			local_irq_enable();
> 	}
> }


It isn't defensive.  The local_irq_disable() is needed to avoid a race
condition.  It was added to fix high latency spikes.

(A comment to that effect in default_idle would be nice).

Without it, after checking need_resched, that flag can be set by an
interrupt before we do "hlt".

The result is a halted CPU even though need_resched is set, and the
idle loop isn't broken until the next interrupt, often a timer tick.

The "sti" in safe_halt() is immediately before the "hlt", which means
it doesn't enable interrupts until after the "hlt" instruction.  This
means that any interrupt will wake the CPU.

local_irq_disable() isn't required in the monitor/mwait loop, because
you check need_resched between the monitor and mwait.  (If Intel had
implemented monitor+mwait as a single instruction, then you'd need it).

So you can remove it from your loop.

Enjoy :)
-- Jamie

-
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/