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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Jun 2020 02:12:08 -0700
From:   Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     rjw@...ysocki.net, viresh.kumar@...aro.org, lenb@...nel.org,
        dsmythies@...us.net, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, peterz@...radead.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow
 enable/disable energy efficiency

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