Re: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[Srinivas Pandruvada]

On Fri, 2020-06-26 at 10:49 +0200, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 03:49:31PM -0700, Srinivas Pandruvada wrote:
> > By default intel_pstate driver disables energy efficiency by
> > setting
> > MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP
> > mode.
> > This CPU model is also shared by Coffee Lake desktop CPUs. This
> > allows
> > these systems to reach maximum possible frequency. But this adds
> > power
> > penalty, which some customers don't want. They want some way to
> > enable/
> > disable dynamically.
> > 
> > So, add an additional attribute "energy_efficiency_enable" under
> > /sys/devices/system/cpu/intel_pstate/ for these CPU models. This
> > allows
> > to read and write bit 19 ("Disable Energy Efficiency Optimization")
> > in
> > the MSR IA32_POWER_CTL.
> 
> 

[...]

> > +``energy_efficiency_enable``
> > +	This attribute is only present on platforms, which has CPUs
> > matching
> 
> 						which have
> 
Thanks, I will fix that.

> > +	Kaby Lake or Coffee Lake desktop CPU model. By default
> > +	"energy_efficiency" is disabled on these CPU models in HWP mode
> > by this
> > +	driver. Enabling energy efficiency may limit maximum operating
> > +	frequency in both HWP and non HWP mode. In non HWP mode, this
> > attribute
> > +	has an effect in turbo range only. But in HWP mode, this
> > attribute also
> > +	has an effect in non turbo range.
> 
> Those last two sentences could be simplified - read strange.
I will try to address this.

[...]

> > @@ -254,6 +254,7 @@
> >  #define MSR_PEBS_FRONTEND		0x000003f7
> >  
> >  #define MSR_IA32_POWER_CTL		0x000001fc
> > +#define MSR_IA32_POWER_CTL_BIT_EE	19
> 
> Sort that MSR in - I know, the rest is not sorted either but we can
> start somewhere. So pls put it...
> 
I will.

> #define MSR_LBR_SELECT                  0x000001c8
> #define MSR_LBR_TOS                     0x000001c9
> 
> <--- here.
> 
> 

[...]

> > +
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >>
> > MSR_IA32_POWER_CTL_BIT_EE;
> 
> So you can simplify to:
> 
> 	enable = !!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE));
> 
> methinks.
> 
Better.

> > +	return sprintf(buf, "%d\n", !enable);
> 
> If this bit is called
> 
> 	"Disable Energy Efficiency Optimization"
> 
> why do you call your function and sysfs file "enable"? This is making
> it
> more confusing.
> 
> Why don't you call it simply: "energy_efficiency" and have it
> intuitive:
> 
> 1 - enabled
> 0 - disabled
> 
I think your suggestion is good. The one other attributes under this
directory has similar style. I will get rid of "_enable".

> ?
> 
> > +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> > +					      struct kobj_attribute *b,
> > +					      const char *buf, size_t
> > count)
> > +{
> > +	u64 power_ctl;
> > +	u32 input;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 10, &input);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&intel_pstate_driver_lock);
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	if (input)
> 
> This is too lax - it will be enabled for any !0 value. Please accept
> only 0 and 1.
> 
OK.


Thanks for the review.


- Srinivas