Re: x86: improve default idle

Re: x86: improve default idle

[Andrew Morton]

On Fri, 18 Apr 2008 17:37:02 GMT
Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=13af4836b3914b23946f6a8982934e2c828c183f
> Commit:     13af4836b3914b23946f6a8982934e2c828c183f
> Parent:     f5149a49f994e5c469ac398af7cdeb8eb612d3a4
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Wed Apr 2 13:23:22 2008 +0200
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Thu Apr 17 17:41:34 2008 +0200
> 
>     x86: improve default idle
>     
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/process_32.c |    8 --------
>  arch/x86/kernel/process_64.c |    8 --------
>  2 files changed, 0 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 08c41ed..3903a8f 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -113,16 +113,8 @@ void default_idle(void)
>  
>  		local_irq_disable();
>  		if (!need_resched()) {
> -			ktime_t t0, t1;
> -			u64 t0n, t1n;
> -
> -			t0 = ktime_get();
> -			t0n = ktime_to_ns(t0);
>  			safe_halt();	/* enables interrupts racelessly */
>  			local_irq_disable();
> -			t1 = ktime_get();
> -			t1n = ktime_to_ns(t1);
> -			sched_clock_idle_wakeup_event(t1n - t0n);
>  		}
>  		local_irq_enable();
>  		current_thread_info()->status |= TS_POLLING;
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 4f40272..e75ccc8 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -107,16 +107,8 @@ void default_idle(void)
>  	smp_mb();
>  	local_irq_disable();
>  	if (!need_resched()) {
> -		ktime_t t0, t1;
> -		u64 t0n, t1n;
> -
> -		t0 = ktime_get();
> -		t0n = ktime_to_ns(t0);
>  		safe_halt();	/* enables interrupts racelessly */
>  		local_irq_disable();
> -		t1 = ktime_get();
> -		t1n = ktime_to_ns(t1);
> -		sched_clock_idle_wakeup_event(t1n - t0n);
>  	}
>  	local_irq_enable();
>  	current_thread_info()->status |= TS_POLLING;

Ingo, what's going on?  There is no way that this patch is so obvious that
it doesn't even need a changelog.

Was this ever sent to a mailing list for review?  I can't find it, so my
last remaining means of understanding the change does not work.

(And if it _did_ have a changelog, I could at least google for the
changelog's text, but even that tool is frustrated here).

Re: x86: improve default idle

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 18 Apr 2008 17:37:02 GMT
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> 
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=13af4836b3914b23946f6a8982934e2c828c183f
> > Commit:     13af4836b3914b23946f6a8982934e2c828c183f
> > Parent:     f5149a49f994e5c469ac398af7cdeb8eb612d3a4
> > Author:     Ingo Molnar <mingo@elte.hu>
> > AuthorDate: Wed Apr 2 13:23:22 2008 +0200
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Thu Apr 17 17:41:34 2008 +0200
> > 
> >     x86: improve default idle
> >     
> >     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > ---
> >  arch/x86/kernel/process_32.c |    8 --------
> >  arch/x86/kernel/process_64.c |    8 --------
> >  2 files changed, 0 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> > index 08c41ed..3903a8f 100644
> > --- a/arch/x86/kernel/process_32.c
> > +++ b/arch/x86/kernel/process_32.c
> > @@ -113,16 +113,8 @@ void default_idle(void)
> >  
> >  		local_irq_disable();
> >  		if (!need_resched()) {
> > -			ktime_t t0, t1;
> > -			u64 t0n, t1n;
> > -
> > -			t0 = ktime_get();
> > -			t0n = ktime_to_ns(t0);
> >  			safe_halt();	/* enables interrupts racelessly */
> >  			local_irq_disable();
> > -			t1 = ktime_get();
> > -			t1n = ktime_to_ns(t1);
> > -			sched_clock_idle_wakeup_event(t1n - t0n);
> >  		}
> >  		local_irq_enable();
> >  		current_thread_info()->status |= TS_POLLING;
> > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> > index 4f40272..e75ccc8 100644
> > --- a/arch/x86/kernel/process_64.c
> > +++ b/arch/x86/kernel/process_64.c
> > @@ -107,16 +107,8 @@ void default_idle(void)
> >  	smp_mb();
> >  	local_irq_disable();
> >  	if (!need_resched()) {
> > -		ktime_t t0, t1;
> > -		u64 t0n, t1n;
> > -
> > -		t0 = ktime_get();
> > -		t0n = ktime_to_ns(t0);
> >  		safe_halt();	/* enables interrupts racelessly */
> >  		local_irq_disable();
> > -		t1 = ktime_get();
> > -		t1n = ktime_to_ns(t1);
> > -		sched_clock_idle_wakeup_event(t1n - t0n);
> >  	}
> >  	local_irq_enable();
> >  	current_thread_info()->status |= TS_POLLING;
> 
> Ingo, what's going on?  There is no way that this patch is so obvious 
> that it doesn't even need a changelog.

sorry, the changelog should be: if a system uses acpi_pm clocksource but 
default idle method then the overhead of entry/exit measurement is quite 
noticeable in profiles. The measurement is redundant anyway, as in HLT 
sched_clock() is still supposed to work fine. (the worst i know of is a 
slight skew on certain CPUs in HLT)

	Ingo

Re: x86: improve default idle

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:
>
> sorry, the changelog should be: if a system uses acpi_pm clocksource but 
> default idle method then the overhead of entry/exit measurement is quite 
> noticeable in profiles. The measurement is redundant anyway, as in HLT 
> sched_clock() is still supposed to work fine. (the worst i know of is a 
> slight skew on certain CPUs in HLT)

sched_clock checks for unstable tsc so on all systems which mark
the TSC unstable at boot that will mean fall back to jiffies.

I'm not sure that will really work for them? It will be very imprecise.
Basically most of the time you'll have 0 delta and occasionally
every tick a huge jump.

For 64bit the idea of relying on TSC here is reasonable, but to make the code 
behaviour really match your description you would first need another 
sched_clock which ignores the normal tsc unstable state (the rewritten 
sched_clock for which I posted links a few times recently did that) 

On 32bit it would also need to take care of the few older CPUs
(like VIA C6) where TSC doesn't tick in C1.

-Andi