Re: [PATCH] x86_64 add missing enter_idle() calls

[Stephane Eranian <eranian@hpl.hp.com>]

Andi,

On Mon, Oct 16, 2006 at 04:36:52PM +0200, Andi Kleen wrote:
> > With the original code, the number of callbacks you see for IDLE_START and
> > IDLE_STOP is not too obvious.
> > 
> > On an idle system Opteron 250 with HZ=250, one would expect to see for a 10s duration:
> > 	- for CPU0      : IDLE_START = IDLE_STOP = about 5000 calls
> > 	- for other CPUs: IDLE_START = IDLE_STOP = about 2500  calls
> 
> Yes.
> 
> > With the original code, you get the following number of calls:
> > 
> > CPU0.IDLE_START = 44 (enter_idle)
> > CPU0.IDLE_STOP  = 5206 (exit_idle)
> > 
> > CPU1.IDLE_START = 27 (enter_idle)
> > CPU1.IDLE_STOP  = 2528 (exit_idle)
> > 
> 
> Hmm, the last time I fixed this when you complained (post .18) i added a counter for 
> entry/exit and verified that it was balanced. I haven't rechecked since then.
> I don't know why your numbers are off. You're using the latest git tree, right?

As I reported earlier, going to the Git kernel did not really change the
number of invocations of the idle notifier. I was very puzzled by this, so I
chased it some more.

I finally found the culprit for this. The current code is wrong for the
simple reason that the cpu_idle() function is NOT always the lowest level
idle loop function. For enter_idle()/__exit_idle() to work correctly they
must be placed in the lowest-level idle loop. The cpu_idle() eventually ends
up in the idle() function, but this one may have a loop in it! This is the
case when idle()=cpu_default_idle() and idle()=poll_idle(), for instance. 

The reason why the idle notifier was called so few times, even though we had
the right number of interrupts, is simply because we were not getting out of
the idle() function. So I can, indeed, confirm that an interrupt in HLT
instruction gets you out, but HLT is in a loop from which you do not get out
unless you need to reschedule. By moving enter_idle()/__exit_idle() to
cpu_default_idle() I got the right number of calls for the idle notifier.

I see two solutions to this:
	- move enter_idle()/__exit_idle() to the actual lowest-level loop,
	  in cpu_default_idle() and not in cpu_idle(). We would also have
	  to do something similar to poll_idle(), or any similar idle function
	  which contains a loop.

	- add exit_idle() to all the local interrupt handlers, like my
	  initial patch was doing and leave the ente_idle()/__exit_idle()
	  where they are today.

-- 
-Stephane