Skip to contentSkip to conversation

x86: limit mwait_idle to Intel CPUs

PATCH In review Andreas Herrmann 6 participants 12 messages Apr 5, 2007

lore
Share & tools
lore

Andreas Herrmann

author
#0 lore raw

PATCH 1/1 x86: limit mwait_idle to Intel CPUs

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 <[email protected]>
---
 arch/i386/kernel/process.c   |    4 +++-
 arch/x86_64/kernel/process.c |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)
arch/i386/kernel/process.c +3 -1
index 393a67d..e3067e4 100644
@@ -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.
arch/x86_64/kernel/process.c +2 -1
index d8d5ccc..fed830c 100644
@@ -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
Signature · 1 line
1.5.0.6
#1 lore raw
Quoted 6 lines · 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?

Quoted 2 lines

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

author
#2 lore raw
Quoted 9 lines · Andi Kleen
Quoted 6 lines · 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).

Quoted 6 lines
Quoted 3 lines

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

Signature · 2 lines
AMD Saxony, Dresden, Germany
Operating System Research Center
#3 lore raw
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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.

Quoted 3 lines

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

author
#4 lore raw
Quoted 11 lines · Andi Kleen
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 5 lines

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

Signature · 4 lines
AMD Saxony, Dresden, Germany
Operating System Research Center
email: [email protected]
phone: +49-351-277-4919
#5 lore raw
Quoted 9 lines · Andreas Herrmann

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

#6 lore raw
Quoted 11 lines · H. Peter Anvin
Quoted 9 lines · Andreas Herrmann

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).

Quoted 6 lines

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

Signature · 3 lines
Markus Rechberger
Operating System Research Center
AMD Saxony LLC & Co. KG
#8 lore raw
In reply to H. Peter Anvin
Quoted 5 lines · Peter Anvin
Quoted 4 lines · Andreas Herrmann

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

Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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

#9 lore raw
Quoted 7 lines · [email protected] wrote:
Quoted 2 lines

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.

Quoted 8 lines
Quoted 1 line

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.

Quoted 6 lines

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

#10 lore raw
In reply to Andreas Herrmann
Quoted 8 lines · On Thursday 05 April 2007 18:20:49 Andreas Herrmann wrote:
Quoted 7 lines · Andi Kleen
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 8 lines
Quoted 5 lines

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

#11 lore raw
In reply to Andi Kleen
Quoted 7 lines
Quoted 5 lines

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.