Re: [PATCH] Do not modify MSR_IA32_ENERGY_PERF_BIAS in kernel

[Thomas Renninger <trenn@suse.de>]

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