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:7.785 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site