lkml.org 
[lkml]   [2020]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [UPDATE][PATCH v3 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
From
Date
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


\
 
 \ /
  Last update: 2020-06-26 11:12    [W:0.119 / U:1.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site