[PATCH] x86: limit mwait_idle to Intel CPUs

[PATCH] x86: limit mwait_idle to Intel CPUs

[Andreas Herrmann]

Commit 991528d7348667924176f3e29addea0675298944
introduced mwait_idle which is supposed to work
for Intel CPUs starting with Core Duo.

AMD Fam10 processors won't enter C1 on mwait.
This patch will enable default_idle for non-Intel
CPUs even if mwait is supported.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/i386/kernel/process.c   |    4 +++-
 arch/x86_64/kernel/process.c |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/i386/kernel/process.c b/arch/i386/kernel/process.c
index 393a67d..e3067e4 100644
--- a/arch/i386/kernel/process.c
+++ b/arch/i386/kernel/process.c
@@ -259,7 +259,9 @@ static void mwait_idle(void)
 
 void __devinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
-	if (cpu_has(c, X86_FEATURE_MWAIT)) {
+	if (cpu_has(c, X86_FEATURE_MWAIT) &&
+	    (c->x86_vendor == X86_VENDOR_INTEL)) {
+
 		printk("monitor/mwait feature present.\n");
 		/*
 		 * Skip, if setup has overridden idle.
diff --git a/arch/x86_64/kernel/process.c b/arch/x86_64/kernel/process.c
index d8d5ccc..fed830c 100644
--- a/arch/x86_64/kernel/process.c
+++ b/arch/x86_64/kernel/process.c
@@ -271,7 +271,8 @@ static void mwait_idle(void)
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
 	static int printed;
-	if (cpu_has(c, X86_FEATURE_MWAIT)) {
+	if (cpu_has(c, X86_FEATURE_MWAIT) &&
+	    (c->x86_vendor == X86_VENDOR_INTEL)) {
 		/*
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
-- 
1.5.0.6

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Andi Kleen]

On Thursday 05 April 2007 16:00:45 Andreas Herrmann wrote:
> 
> Commit 991528d7348667924176f3e29addea0675298944
> introduced mwait_idle which is supposed to work
> for Intel CPUs starting with Core Duo.
> 
> AMD Fam10 processors won't enter C1 on mwait.

Unfortunate. Will this be fixed?

> This patch will enable default_idle for non-Intel
> CPUs even if mwait is supported.

It would be better to clear MONITOR/MWAIT in the AMD specific
CPU initialize code than add workarounds everywhere else.

-Andi

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Andreas Herrmann]

On Thu, Apr 05, 2007 at 04:24:45PM +0200, Andi Kleen wrote:
> On Thursday 05 April 2007 16:00:45 Andreas Herrmann wrote:
> > 
> > Commit 991528d7348667924176f3e29addea0675298944
> > introduced mwait_idle which is supposed to work
> > for Intel CPUs starting with Core Duo.
> > 
> > AMD Fam10 processors won't enter C1 on mwait.
> 
> Unfortunate. Will this be fixed?

No, it is not planned to change this behavior.

In fact mwait does certain power savings but the
core won't enter the C1 state. And power savings from
entering C1 are greater than power savings caused
by mwait (on AMD Fam10).

> 
> > This patch will enable default_idle for non-Intel
> > CPUs even if mwait is supported.
> 
> It would be better to clear MONITOR/MWAIT in the AMD specific
> CPU initialize code than add workarounds everywhere else.

Why is that?
MONITOR/MWAIT is usable. And I think this should
be indicated by cpuinfo.
It's just inappropriate to use it in pm_idle.


Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

RE: [PATCH] x86: limit mwait_idle to Intel CPUs

[Langsdorf, Mark]

> > Commit 991528d7348667924176f3e29addea0675298944
> > introduced mwait_idle which is supposed to work
> > for Intel CPUs starting with Core Duo.
> > 
> > AMD Fam10 processors won't enter C1 on mwait.
> 
> Unfortunate. Will this be fixed?

Fam10 processors were not designed to enter C1 on mwait.
That feature will not be added for Fam10 processors.
 
-Mark Langsdorf
Operating Systems Research Center
AMD, Inc.

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Andi Kleen]

> > > This patch will enable default_idle for non-Intel
> > > CPUs even if mwait is supported.
> > 
> > It would be better to clear MONITOR/MWAIT in the AMD specific
> > CPU initialize code than add workarounds everywhere else.
> 
> Why is that?
> MONITOR/MWAIT is usable.

If it doesn't save power it's not usable imho.

> And I think this should 
> be indicated by cpuinfo.
> It's just inappropriate to use it in pm_idle.

There are no other users anyways and user space can't use it.
Ok in theory you could add a X86_FEATURE_MWAIT_DOESNT_SAVE_POWER
and check that, but just clearing it seems simpler and equivalent.

What would perhaps make sense is to add a idle=mwait command
line option for this though. So that the benchmarkers who currently
use idle=poll could migrate to idle=mwait. That option would need
to check the real cpuid bit of course again, but that should be
easy enough.

-Andi

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Andreas Herrmann]

On Thu, Apr 05, 2007 at 05:37:17PM +0200, Andi Kleen wrote:
> 
> > > > This patch will enable default_idle for non-Intel
> > > > CPUs even if mwait is supported.
> > > 
> > > It would be better to clear MONITOR/MWAIT in the AMD specific
> > > CPU initialize code than add workarounds everywhere else.
> > 
> > Why is that?
> > MONITOR/MWAIT is usable.
> 
> If it doesn't save power it's not usable imho.

But you can also think of monitor/mwait as a power saving means
for fast synchronization.

> > And I think this should 
> > be indicated by cpuinfo.
> > It's just inappropriate to use it in pm_idle.
> 
> There are no other users anyways and user space can't use it.
> Ok in theory you could add a X86_FEATURE_MWAIT_DOESNT_SAVE_POWER
> and check that, but just clearing it seems simpler and equivalent.

It is not equivalent. Usually users check /proc/cpuinfo for their
CPU features. Deleting that flag is kind of obfuscation.

I guess some time ago people did not care about their "svm" or "vmx"
flags. Nowadays (e.g. with kvm) some people are quite happy
if one of those strings occurs in /proc/cpuinfo (and they are quite
disappointed if this feature was disabled by BIOS).

Why not state it positively for CPUs that are able to even enter
C1 with MWAIT, e.g. X86_FEATURE_MWAIT_ENTERS_CSTATE?

If you dislike this wording than I still prefer to have the MWAIT
flag visible and but introducing an MWAIT_DOESNT_ENTER_CSTATE thing.

> What would perhaps make sense is to add a idle=mwait command
> line option for this though. So that the benchmarkers who currently
> use idle=poll could migrate to idle=mwait. That option would need
> to check the real cpuid bit of course again, but that should be
> easy enough.

That sounds good. Except that I prefer to check for
X86_FEATURE_MWAIT.



Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center
email: andreas.herrmann3@amd.com
phone: +49-351-277-4919

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[H. Peter Anvin]

Andreas Herrmann wrote:
> 
> It is not equivalent. Usually users check /proc/cpuinfo for their
> CPU features. Deleting that flag is kind of obfuscation.
> 
> I guess some time ago people did not care about their "svm" or "vmx"
> flags. Nowadays (e.g. with kvm) some people are quite happy
> if one of those strings occurs in /proc/cpuinfo (and they are quite
> disappointed if this feature was disabled by BIOS).
> 

What you're saying is that you want it to appear in /proc/cpuinfo for 
marketing reasons even though it's not usable.

The ones in /proc/cpuinfo are cooked values anyway; there is plenty of 
history to that effect.

I would agree with Andi that if as far as Linux is concerned mwait is 
unusable on AMD Fam10 processors, then the CPU detection code should 
turn this bit off on AMD Fam10 processors.

	-hpa

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Andi Kleen]

On Thursday 05 April 2007 18:20:49 Andreas Herrmann wrote:
> On Thu, Apr 05, 2007 at 05:37:17PM +0200, Andi Kleen wrote:
> > 
> > > > > This patch will enable default_idle for non-Intel
> > > > > CPUs even if mwait is supported.
> > > > 
> > > > It would be better to clear MONITOR/MWAIT in the AMD specific
> > > > CPU initialize code than add workarounds everywhere else.
> > > 
> > > Why is that?
> > > MONITOR/MWAIT is usable.
> > 
> > If it doesn't save power it's not usable imho.
> 
> But you can also think of monitor/mwait as a power saving means
> for fast synchronization.

Just the kernel doesn't use it for that (except in idle) and user
space can't use it anyways because it's ring 0 only.

> It is not equivalent. Usually users check /proc/cpuinfo for their
> CPU features. Deleting that flag is kind of obfuscation.

On the other hand wouldn't they expect mwait to be used if it's displayed
there? I don't see it as an obfuscation to make this clear.

Also it's not that someone's marketing strategy will depend on MWAIT
being displayed in cpuinfo. If it was something hyped like SSE* 
or 3dnow perhaps, but that's not the case.

> I guess some time ago people did not care about their "svm" or "vmx"
> flags. Nowadays (e.g. with kvm) some people are quite happy
> if one of those strings occurs in /proc/cpuinfo (and they are quite
> disappointed if this feature was disabled by BIOS).

Well older kernels don't know it, so actually a lot of people
learned to use another program to check anyways.

> Why not state it positively for CPUs that are able to even enter
> C1 with MWAIT, e.g. X86_FEATURE_MWAIT_ENTERS_CSTATE?

It seems a bit counterintuitive to add special code for CPUs
that do something right to avoid adding code to CPUs that do otherwise.

> > What would perhaps make sense is to add a idle=mwait command
> > line option for this though. So that the benchmarkers who currently
> > use idle=poll could migrate to idle=mwait. That option would need
> > to check the real cpuid bit of course again, but that should be
> > easy enough.
> 
> That sounds good. Except that I prefer to check for
> X86_FEATURE_MWAIT.

Ok, perhaps the second cpuid would be a bit ugly. Then do two
flags if you prefer that.

-Andi

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[Markus Rechberger]

H. Peter Anvin wrote:
> Andreas Herrmann wrote:
>>
>> It is not equivalent. Usually users check /proc/cpuinfo for their
>> CPU features. Deleting that flag is kind of obfuscation.
>>
>> I guess some time ago people did not care about their "svm" or "vmx"
>> flags. Nowadays (e.g. with kvm) some people are quite happy
>> if one of those strings occurs in /proc/cpuinfo (and they are quite
>> disappointed if this feature was disabled by BIOS).
>>
>
> What you're saying is that you want it to appear in /proc/cpuinfo for 
> marketing reasons even though it's not usable.
>
> The ones in /proc/cpuinfo are cooked values anyway; there is plenty of 
> history to that effect.
>
> I would agree with Andi that if as far as Linux is concerned mwait is 
> unusable on AMD Fam10 processors, then the CPU detection code should 
> turn this bit off on AMD Fam10 processors.
>
please see:

http://kvm.qumranet.com/kvmwiki/FAQ

"How can I tell if I have Intel VT or AMD-V?"

Markus

-- 
Markus Rechberger
Operating System Research Center
AMD Saxony LLC & Co. KG

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[H. Peter Anvin]

Markus Rechberger wrote:
> please see:
> 
> http://kvm.qumranet.com/kvmwiki/FAQ
> 
> "How can I tell if I have Intel VT or AMD-V?"

Yes, what's your point?

	-hpa

RE: [PATCH] x86: limit mwait_idle to Intel CPUs

[aherrman]

Peter Anvin wrote:
> Andreas Herrmann wrote:
> > 
> > It is not equivalent. Usually users check /proc/cpuinfo for their
> > CPU features. Deleting that flag is kind of obfuscation.
> > 
> > I guess some time ago people did not care about their "svm" or "vmx"
> > flags. Nowadays (e.g. with kvm) some people are quite happy
> > if one of those strings occurs in /proc/cpuinfo (and they are quite
> > disappointed if this feature was disabled by BIOS).
> > 

> What you're saying is that you want it to appear in /proc/cpuinfo for 
> marketing reasons even though it's not usable.

Bruellller!
So you are saying I am a marketing guy -- wasn't aware of that.
Just big fun.

> The ones in /proc/cpuinfo are cooked values anyway; there is plenty of 
> history to that effect.

I don't know this history. And I don't care.
I thought /proc/cpuinfo should show an (almost) complete list
of CPU features. If this is not the case it's a pity.

> I would agree with Andi that if as far as Linux is concerned mwait is 
> unusable on AMD Fam10 processors, then the CPU detection code should 
> turn this bit off on AMD Fam10 processors.

And finally I was not aware that you and Andi think of monitor/mwait
as a synonym for Intel's native C-States.

So I guess, what you really want is that for AMD CPUs the
monitor-flag (aka native C-state-flag) gets removed.

And if somedays another use case for monitor/mwait appears, the flag
has to be reintroduced for AMD CPUs.

Fine with me.
The only drawback is that Andi's idea of idle=mwait wouldn't make
sense anymore.


Regards,

Andreas

Re: [PATCH] x86: limit mwait_idle to Intel CPUs

[H. Peter Anvin]

aherrman@arcor.de wrote:
> 
>> The ones in /proc/cpuinfo are cooked values anyway; there is plenty of 
>> history to that effect.
> 
> I don't know this history. And I don't care.
> I thought /proc/cpuinfo should show an (almost) complete list
> of CPU features. If this is not the case it's a pity.
> 

It's a list of features which are implemented and working, as far as 
they make sense.  In other words, it's *Linux* concept of the feature 
set of the CPU.

>> I would agree with Andi that if as far as Linux is concerned mwait is 
>> unusable on AMD Fam10 processors, then the CPU detection code should 
>> turn this bit off on AMD Fam10 processors.
> 
> And finally I was not aware that you and Andi think of monitor/mwait
> as a synonym for Intel's native C-States.
> 
> So I guess, what you really want is that for AMD CPUs the
> monitor-flag (aka native C-state-flag) gets removed.
> 
> And if somedays another use case for monitor/mwait appears, the flag
> has to be reintroduced for AMD CPUs.
> 
> Fine with me.
> The only drawback is that Andi's idea of idle=mwait wouldn't make
> sense anymore.

Well, if there is use for the AMD implementation of monitor/mwait, then 
that's a different situation, and probably would call for multiple 
feature bits.

	-hpa