| Message ID | alpine.LFD.2.02.1103301819250.31342@x980 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
* Len Brown <lenb@kernel.org> wrote: > From: Len Brown <len.brown@intel.com> > > Since 2.6.35 (23016bf0d25), Linux prints the existence of "epb" in /proc/cpuinfo, > Since 2.6.38 (d5532ee7b40), the x86_energy_perf_policy(8) utility is available > in-tree to update MSR_IA32_ENERGY_PERF_BIAS. > > However, the typical BIOS fails to initialize the MSR, > and the typical Linux distro neglects to invoke x86_energy_perf_policy(8). > > The result is that some modern hardware is running in hardware default, > which is "performance" mode, rather than the intended design default > of "normal" mode. > > Initialize the MSR to the "normal" setting during kernel boot. > > Of course, x86_energy_perf_policy(8) is available to change > the default after boot, should the user have a policy preference. > > cc: stable@kernel.org > Signed-off-by: Len Brown <len.brown@intel.com> > --- > arch/x86/include/asm/msr-index.h | 3 +++ > arch/x86/kernel/cpu/intel.c | 14 ++++++++++++++ > 2 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index 43a18c7..91fedd9 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -250,6 +250,9 @@ > #define MSR_IA32_TEMPERATURE_TARGET 0x000001a2 > > #define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0 > +#define ENERGY_PERF_BIAS_PERFORMANCE 0 > +#define ENERGY_PERF_BIAS_NORMAL 6 > +#define ENERGY_PERF_BIAS_POWERSWAVE 15 > > #define MSR_IA32_PACKAGE_THERM_STATUS 0x000001b1 > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index d16c2c5..48cca4a 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -448,6 +448,20 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c) > > if (cpu_has(c, X86_FEATURE_VMX)) > detect_vmx_virtcap(c); > + > + /* > + * Initialize MSR_IA32_ENERGY_PERF_BIAS if BIOS did not. > + * x86_energy_perf_policy(8) is available to change it at run-time > + */ > + if (cpu_has(c, X86_FEATURE_EPB)) { > + u64 epb; > + > + rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); > + if ((epb & 0xF) == 0) { > + epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL; > + wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); > + } > + } Dunno, this patch appears to silently modify the system to be slower than it was before under Linux. Won't people report this as a regression if this change reduces performance for them? They wont be able to see your comments in the code and in the changelog either, when this happens to them. They might look into /proc/cpuinfo and see 'epb' there but it wont tell them anything. They wont know about a utility available in tools/power/x86/ either. So this patch has 'future trouble' written all over it i'm afraid. Thanks, Ingo
> > From: Len Brown <len.brown@intel.com> > > > > Since 2.6.35 (23016bf0d25), Linux prints the existence of "epb" in /proc/cpuinfo, > > Since 2.6.38 (d5532ee7b40), the x86_energy_perf_policy(8) utility is available > > in-tree to update MSR_IA32_ENERGY_PERF_BIAS. > > > > However, the typical BIOS fails to initialize the MSR, > > and the typical Linux distro neglects to invoke x86_energy_perf_policy(8). > > > > The result is that some modern hardware is running in hardware default, > > which is "performance" mode, rather than the intended design default > > of "normal" mode. > > > > Initialize the MSR to the "normal" setting during kernel boot. > > > > Of course, x86_energy_perf_policy(8) is available to change > > the default after boot, should the user have a policy preference. > > > > cc: stable@kernel.org > > Signed-off-by: Len Brown <len.brown@intel.com> > > --- > > arch/x86/include/asm/msr-index.h | 3 +++ > > arch/x86/kernel/cpu/intel.c | 14 ++++++++++++++ > > 2 files changed, 17 insertions(+), 0 deletions(-) > > ... > > Dunno, this patch appears to silently modify the system to be slower than it > was before under Linux. > > Won't people report this as a regression if this change reduces performance for > them? > > They wont be able to see your comments in the code and in the changelog either, > when this happens to them. They might look into /proc/cpuinfo and see 'epb' > there but it wont tell them anything. They wont know about a utility available > in tools/power/x86/ either. This patch makes no change to the epb feature indicator /proc/cpuinfo. > So this patch has 'future trouble' written all over it i'm afraid. EPB is limited to SNB and later. So the installed base as yet is small. (it also exists on WSM-EP, but doesn't do so much there) EPB will have a more significant effect on future hardware. Linux currently trails competing operating systems in energy efficiency on SNB due to this setting, and Linux will trail competing operating systems even more on future hardware if this default is not fixed. Will it be possible to measure a performance difference between "performance" and "normal"? Yes, it will be possible. Will 99.9% of users notice? Nope. More likely they'll notice the the power savings that are disabled in "performance" mode. I should have called it "benchmark" mode instead of "performance" mode... thanks, Len Brown, Intel Open Source Technology Center
* Len Brown <lenb@kernel.org> wrote: > > > > From: Len Brown <len.brown@intel.com> > > > > > > Since 2.6.35 (23016bf0d25), Linux prints the existence of "epb" in /proc/cpuinfo, > > > Since 2.6.38 (d5532ee7b40), the x86_energy_perf_policy(8) utility is available > > > in-tree to update MSR_IA32_ENERGY_PERF_BIAS. > > > > > > However, the typical BIOS fails to initialize the MSR, > > > and the typical Linux distro neglects to invoke x86_energy_perf_policy(8). > > > > > > The result is that some modern hardware is running in hardware default, > > > which is "performance" mode, rather than the intended design default > > > of "normal" mode. > > > > > > Initialize the MSR to the "normal" setting during kernel boot. > > > > > > Of course, x86_energy_perf_policy(8) is available to change > > > the default after boot, should the user have a policy preference. > > > > > > cc: stable@kernel.org > > > Signed-off-by: Len Brown <len.brown@intel.com> > > > --- > > > arch/x86/include/asm/msr-index.h | 3 +++ > > > arch/x86/kernel/cpu/intel.c | 14 ++++++++++++++ > > > 2 files changed, 17 insertions(+), 0 deletions(-) > > > > ... > > > > Dunno, this patch appears to silently modify the system to be slower than it > > was before under Linux. > > > > Won't people report this as a regression if this change reduces performance for > > them? > > > > They wont be able to see your comments in the code and in the changelog either, > > when this happens to them. They might look into /proc/cpuinfo and see 'epb' > > there but it wont tell them anything. They wont know about a utility available > > in tools/power/x86/ either. > > This patch makes no change to the epb feature indicator > /proc/cpuinfo. I know. I reacted to this bit in the changelog: > > > Since 2.6.35 (23016bf0d25), Linux prints the existence of "epb" in /proc/cpuinfo, Printing the existence of a CPU feature does nothing to inform users. > > So this patch has 'future trouble' written all over it i'm afraid. > > EPB is limited to SNB and later. > So the installed base as yet is small. > (it also exists on WSM-EP, but doesn't do so much there) > EPB will have a more significant effect on future hardware. > > Linux currently trails competing operating systems in energy > efficiency on SNB due to this setting, and Linux will trail > competing operating systems even more on future hardware > if this default is not fixed. > > Will it be possible to measure a performance difference between > "performance" and "normal"? Yes, it will be possible. > Will 99.9% of users notice? Nope. More likely they'll notice > the the power savings that are disabled in "performance" mode. > > I should have called it "benchmark" mode instead of "performance" mode... That's all fair but does not address the concerns i raised. A silent change during bootup is asking for trouble. So how about informing users, how about making it non-silent? An informative printk that also mentions the power configuration tool, etc. This solves the concerns i mentioned. Thanks, Ingo
> So how about informing users, how about making it non-silent? An informative > printk that also mentions the power configuration tool, etc. This solves the > concerns i mentioned. Something like this? rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); if ((epb & 0xF) == 0) { printk_once(KERN_WARN, "x86: updated energy_perf_bias" " to 'normal' from 'performance'\n" "You can view and update epb via utility," " such as x86_energy_perf_policy(8)\n"); epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL; wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); }
Ack. Let's just do this. Ingo?
Linus
On Wed, Jul 13, 2011 at 1:44 PM, Len Brown <lenb@kernel.org> wrote:
>
>> So how about informing users, how about making it non-silent? An informative
>> printk that also mentions the power configuration tool, etc. This solves the
>> concerns i mentioned.
>
> Something like this?
>
> rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> if ((epb & 0xF) == 0) {
> printk_once(KERN_WARN, "x86: updated energy_perf_bias"
> " to 'normal' from 'performance'\n"
> "You can view and update epb via utility,"
> " such as x86_energy_perf_policy(8)\n");
> epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL;
> wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> }
>
>
> Ok. So... what "serious bug" does this fix? You really need to use cc: > stable less. Tweaking performance/power ratio is _not_ serious bug. While the performance difference may not be significant, the energy difference may be. Some people think it is serious when Linux has worse out-of-box energy efficiency than Windows on the same hardware. Some people think that it is serious when their Linux distribution has worse energy efficiency than competing Linux distributions. Greg has given me a hard time for not cc'ing stable _enough_. I guess the folks that answer the stable mail get to decide... cheers, -Len
On 07/13/2011 01:49 PM, Linus Torvalds wrote: > Ack. Let's just do this. Ingo? > > Linus Ingo is travelling this week, but this seems to have converged. -hpa
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 43a18c7..91fedd9 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -250,6 +250,9 @@ #define MSR_IA32_TEMPERATURE_TARGET 0x000001a2 #define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0 +#define ENERGY_PERF_BIAS_PERFORMANCE 0 +#define ENERGY_PERF_BIAS_NORMAL 6 +#define ENERGY_PERF_BIAS_POWERSWAVE 15 #define MSR_IA32_PACKAGE_THERM_STATUS 0x000001b1 diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index d16c2c5..48cca4a 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -448,6 +448,20 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c) if (cpu_has(c, X86_FEATURE_VMX)) detect_vmx_virtcap(c); + + /* + * Initialize MSR_IA32_ENERGY_PERF_BIAS if BIOS did not. + * x86_energy_perf_policy(8) is available to change it at run-time + */ + if (cpu_has(c, X86_FEATURE_EPB)) { + u64 epb; + + rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); + if ((epb & 0xF) == 0) { + epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL; + wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb); + } + } } #ifdef CONFIG_X86_32