Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1344435 > unrolled thread

[PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel

Back to article view | Back to linux.kernel

Contents

  [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel Thomas Renninger <trenn@suse.de> - 2016-02-26 17:40 +0100
    Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel Thomas Renninger <trenn@suse.de> - 2016-02-26 17:50 +0100
    Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-27 00:20 +0100
      Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel Thomas Renninger <trenn@suse.de> - 2016-03-01 13:20 +0100
        Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-02 01:30 +0100
          Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel Thomas Renninger <trenn@suse.de> - 2016-03-04 09:40 +0100

#1344435 — [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel

The assumption that BIOSes never want to have this register being set to
full performance (zero) is wrong.

While wrongly overruling this BIOS setting and set it from performance
to normal did not hurt that much, because nobody really knew the effects inside
Intel processors.

But with Broadwell-EP processor (E5-2687W v4) the CPU will not enter turbo modes
if this value is not set to performance.

So switch logic to tell the user in a friendly way (info) that the CPU is in
performance mode and how to switch via userspace if this is not intended.

But otherwise trust that the BIOS has set the correct value here and do not
blindly overrule.

How this has been found: SLE11 had this patch, SLE12 it slipped through.
It took quite some time to nail down that this patch missing is the reason
for not entering turbo modes with this specific processor.

Signed-off-by: Thomas Renninger <trenn@suse.com>

--- a/arch/x86/kernel/cpu/intel.c	2016-02-26 17:19:55.731042972 +0100
+++ b/arch/x86/kernel/cpu/intel.c	2016-02-26 17:20:48.598020581 +0100
@@ -377,8 +377,12 @@ static void init_intel_energy_perf(struc
 	u64 epb;
 
 	/*
-	 * Initialize MSR_IA32_ENERGY_PERF_BIAS if not already initialized.
-	 * (x86_energy_perf_policy(8) is available to change it at run-time.)
+	 * On server platforms energy bias typically is set to
+	 * performance on purpose.
+	 * On other platforms it may happen that MSR_IA32_ENERGY_PERF_BIAS
+	 * did not get initialized properly by BIOS.
+	 * Best is to to keep BIOS settings and give the user a hint whether
+	 * to change it via cpupower-set(8) userspace tool at runtime.
 	 */
 	if (!cpu_has(c, X86_FEATURE_EPB))
 		return;
@@ -387,10 +391,8 @@ static void init_intel_energy_perf(struc
 	if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
 		return;
 
-	pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 'performance'\n");
-	pr_warn_once("ENERGY_PERF_BIAS: View and update with x86_energy_perf_policy(8)\n");
-	epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL;
-	wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
+	pr_info_once("ENERGY_PERF_BIAS is set to 'performance'\n");
+	pr_info_once("ENERGY_PERF_BIAS: Update with cpupower-set(8)\n");
 }
 
 static void intel_bsp_resume(struct cpuinfo_x86 *c)

[toc] | [next] | [standalone]

#1344452

This in fact is a re-send, including x86 maintainers.
Even this is a PM (Power Management) issue, the code is in the
x86 architecture paths.

From last submit:
> > Patch is against latest linux-pm kernel.
> > Rafael: Can you queue this one up, please.

> Well, I'm not an x86 arch maintainer.
> Can you at least CC them, please?

Can someone, best would be a x86 maintainer?, take the patch please if
it is considered ok.

Thanks,

       Thomas

[toc] | [prev] | [next] | [standalone]

#1344826

On Friday, February 26, 2016 05:38:00 PM Thomas Renninger wrote:
> The assumption that BIOSes never want to have this register being set to
> full performance (zero) is wrong.
> 
> While wrongly overruling this BIOS setting and set it from performance
> to normal did not hurt that much, because nobody really knew the effects inside
> Intel processors.
> 
> But with Broadwell-EP processor (E5-2687W v4) the CPU will not enter turbo modes
> if this value is not set to performance.
> 
> So switch logic to tell the user in a friendly way (info) that the CPU is in
> performance mode and how to switch via userspace if this is not intended.
> 
> But otherwise trust that the BIOS has set the correct value here and do not
> blindly overrule.
> 
> How this has been found: SLE11 had this patch, SLE12 it slipped through.
> It took quite some time to nail down that this patch missing is the reason
> for not entering turbo modes with this specific processor.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>
> 
> --- a/arch/x86/kernel/cpu/intel.c	2016-02-26 17:19:55.731042972 +0100
> +++ b/arch/x86/kernel/cpu/intel.c	2016-02-26 17:20:48.598020581 +0100
> @@ -377,8 +377,12 @@ static void init_intel_energy_perf(struc
>  	u64 epb;
>  
>  	/*
> -	 * Initialize MSR_IA32_ENERGY_PERF_BIAS if not already initialized.
> -	 * (x86_energy_perf_policy(8) is available to change it at run-time.)
> +	 * On server platforms energy bias typically is set to
> +	 * performance on purpose.
> +	 * On other platforms it may happen that MSR_IA32_ENERGY_PERF_BIAS
> +	 * did not get initialized properly by BIOS.
> +	 * Best is to to keep BIOS settings and give the user a hint whether
> +	 * to change it via cpupower-set(8) userspace tool at runtime.
>  	 */
>  	if (!cpu_has(c, X86_FEATURE_EPB))
>  		return;
> @@ -387,10 +391,8 @@ static void init_intel_energy_perf(struc
>  	if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
>  		return;
>  
> -	pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 'performance'\n");
> -	pr_warn_once("ENERGY_PERF_BIAS: View and update with x86_energy_perf_policy(8)\n");
> -	epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL;
> -	wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> +	pr_info_once("ENERGY_PERF_BIAS is set to 'performance'\n");
> +	pr_info_once("ENERGY_PERF_BIAS: Update with cpupower-set(8)\n");

This doesn't need to be cpupower-set IMO.

>  }
>  
>  static void intel_bsp_resume(struct cpuinfo_x86 *c)
> 

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1346562

On Saturday, February 27, 2016 12:15:47 AM Rafael J. Wysocki wrote:
> On Friday, February 26, 2016 05:38:00 PM Thomas Renninger wrote:
> > The assumption that BIOSes never want to have this register being set to
> > full performance (zero) is wrong.
> > 
> > While wrongly overruling this BIOS setting and set it from performance
> > to normal did not hurt that much, because nobody really knew the effects
> > inside Intel processors.
> > 
> > But with Broadwell-EP processor (E5-2687W v4) the CPU will not enter turbo
> > modes if this value is not set to performance.
> > 
> > So switch logic to tell the user in a friendly way (info) that the CPU is
> > in performance mode and how to switch via userspace if this is not
> > intended.
> > 
> > But otherwise trust that the BIOS has set the correct value here and do
> > not
> > blindly overrule.
> > 
> > How this has been found: SLE11 had this patch, SLE12 it slipped through.
> > It took quite some time to nail down that this patch missing is the reason
> > for not entering turbo modes with this specific processor.
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.com>
> > 
> > --- a/arch/x86/kernel/cpu/intel.c	2016-02-26 17:19:55.731042972 +0100
> > +++ b/arch/x86/kernel/cpu/intel.c	2016-02-26 17:20:48.598020581 +0100
> > @@ -377,8 +377,12 @@ static void init_intel_energy_perf(struc
> > 
> >  	u64 epb;
> >  	
> >  	/*
> > 
> > -	 * Initialize MSR_IA32_ENERGY_PERF_BIAS if not already initialized.
> > -	 * (x86_energy_perf_policy(8) is available to change it at run-time.)
> > +	 * On server platforms energy bias typically is set to
> > +	 * performance on purpose.
> > +	 * On other platforms it may happen that MSR_IA32_ENERGY_PERF_BIAS
> > +	 * did not get initialized properly by BIOS.
> > +	 * Best is to to keep BIOS settings and give the user a hint whether
> > +	 * to change it via cpupower-set(8) userspace tool at runtime.
> > 
> >  	 */
> >  	
> >  	if (!cpu_has(c, X86_FEATURE_EPB))
> >  	
> >  		return;
> > 
> > @@ -387,10 +391,8 @@ static void init_intel_energy_perf(struc
> > 
> >  	if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
> >  	
> >  		return;
> > 
> > -	pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 
'performance'\n");
> > -	pr_warn_once("ENERGY_PERF_BIAS: View and update with
> > x86_energy_perf_policy(8)\n"); -	epb = (epb & ~0xF) |
> > ENERGY_PERF_BIAS_NORMAL;
> > -	wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> > +	pr_info_once("ENERGY_PERF_BIAS is set to 'performance'\n");
> > +	pr_info_once("ENERGY_PERF_BIAS: Update with cpupower-set(8)\n");
> 
> This doesn't need to be cpupower-set IMO.

You mean why switch the message from:
x86_energy_perf_policy to cpupower-set
?

IMO x86_energy_perf_policy should not exist. It has been introduce before
cpupower set -b.
Having an extra tool/binary for this functionality is an unneeded packaging 
overhead for distros.
Also having more and more of such CPU specific tools is not userfriendly.
cpupower supports all power relevant features of your CPU and on all 
architectures (or at least it should). People should know this one better
than "x86_energy_perf_policy" and theoretically intuitively find it, even 
without a message.

So it would be nice to get the message fixed as well.

  Thomas

[toc] | [prev] | [next] | [standalone]

#1347239

On Tuesday, March 01, 2016 01:17:37 PM Thomas Renninger wrote:
> On Saturday, February 27, 2016 12:15:47 AM Rafael J. Wysocki wrote:
> > On Friday, February 26, 2016 05:38:00 PM Thomas Renninger wrote:
> > > The assumption that BIOSes never want to have this register being set to
> > > full performance (zero) is wrong.
> > > 
> > > While wrongly overruling this BIOS setting and set it from performance
> > > to normal did not hurt that much, because nobody really knew the effects
> > > inside Intel processors.
> > > 
> > > But with Broadwell-EP processor (E5-2687W v4) the CPU will not enter turbo
> > > modes if this value is not set to performance.
> > > 
> > > So switch logic to tell the user in a friendly way (info) that the CPU is
> > > in performance mode and how to switch via userspace if this is not
> > > intended.
> > > 
> > > But otherwise trust that the BIOS has set the correct value here and do
> > > not
> > > blindly overrule.
> > > 
> > > How this has been found: SLE11 had this patch, SLE12 it slipped through.
> > > It took quite some time to nail down that this patch missing is the reason
> > > for not entering turbo modes with this specific processor.
> > > 
> > > Signed-off-by: Thomas Renninger <trenn@suse.com>
> > > 
> > > --- a/arch/x86/kernel/cpu/intel.c	2016-02-26 17:19:55.731042972 +0100
> > > +++ b/arch/x86/kernel/cpu/intel.c	2016-02-26 17:20:48.598020581 +0100
> > > @@ -377,8 +377,12 @@ static void init_intel_energy_perf(struc
> > > 
> > >  	u64 epb;
> > >  	
> > >  	/*
> > > 
> > > -	 * Initialize MSR_IA32_ENERGY_PERF_BIAS if not already initialized.
> > > -	 * (x86_energy_perf_policy(8) is available to change it at run-time.)
> > > +	 * On server platforms energy bias typically is set to
> > > +	 * performance on purpose.
> > > +	 * On other platforms it may happen that MSR_IA32_ENERGY_PERF_BIAS
> > > +	 * did not get initialized properly by BIOS.
> > > +	 * Best is to to keep BIOS settings and give the user a hint whether
> > > +	 * to change it via cpupower-set(8) userspace tool at runtime.
> > > 
> > >  	 */
> > >  	
> > >  	if (!cpu_has(c, X86_FEATURE_EPB))
> > >  	
> > >  		return;
> > > 
> > > @@ -387,10 +391,8 @@ static void init_intel_energy_perf(struc
> > > 
> > >  	if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
> > >  	
> > >  		return;
> > > 
> > > -	pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 
> 'performance'\n");
> > > -	pr_warn_once("ENERGY_PERF_BIAS: View and update with
> > > x86_energy_perf_policy(8)\n"); -	epb = (epb & ~0xF) |
> > > ENERGY_PERF_BIAS_NORMAL;
> > > -	wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> > > +	pr_info_once("ENERGY_PERF_BIAS is set to 'performance'\n");
> > > +	pr_info_once("ENERGY_PERF_BIAS: Update with cpupower-set(8)\n");
> > 
> > This doesn't need to be cpupower-set IMO.
> 
> You mean why switch the message from:
> x86_energy_perf_policy to cpupower-set
> ?
> 
> IMO x86_energy_perf_policy should not exist. It has been introduce before
> cpupower set -b.
> Having an extra tool/binary for this functionality is an unneeded packaging 
> overhead for distros.
> Also having more and more of such CPU specific tools is not userfriendly.
> cpupower supports all power relevant features of your CPU and on all 
> architectures (or at least it should). People should know this one better
> than "x86_energy_perf_policy" and theoretically intuitively find it, even 
> without a message.
> 
> So it would be nice to get the message fixed as well.

My point is that since "cpupower set -b" is not the only way to set this,
it doesn't seem appropriate to refer to it explicitly from a kernel message.

I actually don't think the second message is necessary at all.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1349941

On Wednesday, March 02, 2016 01:26:18 AM Rafael J. Wysocki wrote:
> On Tuesday, March 01, 2016 01:17:37 PM Thomas Renninger wrote:
> > > >  	if (!cpu_has(c, X86_FEATURE_EPB))z
> > > >  	
> > > >  		return;
> > > > 
> > > > @@ -387,10 +391,8 @@ static void init_intel_energy_perf(struc
> > > > 
> > > >  	if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
> > > >  	
> > > >  		return;
> > > > 
> > > > -	pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was
> > 
> > 'performance'\n");
> > 
> > > > -	pr_warn_once("ENERGY_PERF_BIAS: View and update with
> > > > x86_energy_perf_policy(8)\n"); -	epb = (epb & ~0xF) |
> > > > ENERGY_PERF_BIAS_NORMAL;
> > > > -	wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> > > > +	pr_info_once("ENERGY_PERF_BIAS is set to 'performance'\n");
> > > > +	pr_info_once("ENERGY_PERF_BIAS: Update with cpupower-
set(8)\n");
> > > 
> > > This doesn't need to be cpupower-set IMO.
> > 
> > You mean why switch the message from:
> > x86_energy_perf_policy to cpupower-set
> > ?
> > 
> > IMO x86_energy_perf_policy should not exist. It has been introduce before
> > cpupower set -b.
> > Having an extra tool/binary for this functionality is an unneeded
> > packaging
> > overhead for distros.
> > Also having more and more of such CPU specific tools is not userfriendly.
> > cpupower supports all power relevant features of your CPU and on all
> > architectures (or at least it should). People should know this one better
> > than "x86_energy_perf_policy" and theoretically intuitively find it, even
> > without a message.
> > 
> > So it would be nice to get the message fixed as well.
> 
> My point is that since "cpupower set -b" is not the only way to set this,
> it doesn't seem appropriate to refer to it explicitly from a kernel message.
> 
> I actually don't think the second message is necessary at all.

Hmm, thinking a bit more about this, I think the whole 
init_intel_energy_perf() function check should vanish.

The check should get moved into the powertop userspace tool.
This one is used to optimize platform for power saving features.

This would also keep the kernel core code clean...

If you agree I will send the patch.

      Thomas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web