RE: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[Doug Smythies]

Hi Srinivas,

I saw your V3.
I do not understand your reluctance to use

arch/x86/include/asm/msr-index.h

as the place to define anything MSR related.
Please explain.

One more comment about 1/3 of the way down below.

... Doug

On 2020.06.23 08:53 Doug Smythies wrote:
> On 2020.06.22 22:13 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.
> >
> > This attribute is present in both HWP and non-HWP mode as this has an
> > effect in both modes. Refer to Intel Software Developer's manual for
> > details. The scope of this bit is package wide.
> 
> I do and always have. However these manuals are 1000's of pages,
> are updated often, and it can be difficult to find the correct page
> for the correct processor. So it is great that, in general, the same
> mnemonics are used in the code as the manuals.
> 
> >
> > Suggested-by: Len Brown <lenb@xxxxxxxxxx>
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> >  Documentation/admin-guide/pm/intel_pstate.rst |  7 +++
> >  drivers/cpufreq/intel_pstate.c                | 49 ++++++++++++++++++-
> >  2 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-
> > guide/pm/intel_pstate.rst
> > index 39d80bc29ccd..939bfdc53f4f 100644
> > --- a/Documentation/admin-guide/pm/intel_pstate.rst
> > +++ b/Documentation/admin-guide/pm/intel_pstate.rst
> > @@ -431,6 +431,13 @@ argument is passed to the kernel in the command line.
> >  	supported in the current configuration, writes to this attribute will
> >  	fail with an appropriate error.
> >
> > +``energy_efficiency_enable``
> > +	This attribute is only present on platforms, which has CPUs matching
> > +	Kaby Lake desktop CPU model. By default "energy_efficiency" is disabled
> 
> So, why not mention Coffee Lake also, as you did above?

And I still think you need to add "Coffee Lake" here also.

> 
> > +	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.
> > +
> >  Interpretation of Policy Attributes
> >  -----------------------------------
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index 8e23a698ce04..1cf6d06f2314 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1218,6 +1218,44 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
> >  	return count;
> >  }
> >
> > +#define MSR_IA32_POWER_CTL_BIT_EE	19
> 
> (same comment as the other day, for another patch) In my opinion and for
> consistency, this bit should be defined in
> 
> arch/x86/include/asm/msr-index.h
> 
> like so (I defined the other bits also):
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e8370e64a155..1a6084067f18 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -255,6 +255,12 @@
> 
>  #define MSR_IA32_POWER_CTL             0x000001fc
> 
> +/* POWER_CTL bits (some are model specific): */
> +
> +#define POWER_CTL_C1E                  1
> +#define POWER_CTL_EEO                  19
> +#define POWER_CTL_RHO                  20
> +
>  #define MSR_IA32_MC0_CTL               0x00000400
>  #define MSR_IA32_MC0_STATUS            0x00000401
>  #define MSR_IA32_MC0_ADDR              0x00000402
> 
> There is another almost identical file at:
> 
> tools/arch/x86/include/asm/msr-index.h
> 
> and I am not sure which one is the master, but
> the former contains stuff that the later does not.
> 
> I have defined the bits names in a consistent way
> as observed elsewhere in the file. i.e. drop the
> "MSR_IA32_" prefix and add the bit.
> 
> Now, tying this back to my earlier comment about the
> manuals:
> The file "tools/arch/x86/include/asm/msr-index.h"
> is an excellent gateway reference for those
> 1000's of pages of software reference manuals.
> 
> As a user that uses them all the time, but typically
> only digs deep into those manuals once every year or
> two, I find tremendous value added via the msr-index.h
> file.
> 
> > +
> > +static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
> > +					     struct kobj_attribute *attr,
> > +					     char *buf)
> > +{
> > +	u64 power_ctl;
> > +	int enable;
> > +
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;
> > +	return sprintf(buf, "%d\n", !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)
> > +		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > +	else
> > +		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > +	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	mutex_unlock(&intel_pstate_driver_lock);
> > +
> > +	return count;
> > +}
> > +
> >  show_one(max_perf_pct, max_perf_pct);
> >  show_one(min_perf_pct, min_perf_pct);
> >
> > @@ -1228,6 +1266,7 @@ define_one_global_rw(min_perf_pct);
> >  define_one_global_ro(turbo_pct);
> >  define_one_global_ro(num_pstates);
> >  define_one_global_rw(hwp_dynamic_boost);
> > +define_one_global_rw(energy_efficiency_enable);
> >
> >  static struct attribute *intel_pstate_attributes[] = {
> >  	&status.attr,
> > @@ -1241,6 +1280,8 @@ static const struct attribute_group intel_pstate_attr_group = {
> >  	.attrs = intel_pstate_attributes,
> >  };
> >
> > +static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[];
> > +
> >  static void __init intel_pstate_sysfs_expose_params(void)
> >  {
> >  	struct kobject *intel_pstate_kobject;
> > @@ -1273,6 +1314,12 @@ static void __init intel_pstate_sysfs_expose_params(void)
> >  				       &hwp_dynamic_boost.attr);
> >  		WARN_ON(rc);
> >  	}
> > +
> > +	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
> > +		rc = sysfs_create_file(intel_pstate_kobject,
> > +				       &energy_efficiency_enable.attr);
> > +		WARN_ON(rc);
> > +	}
> >  }
> >  /************************** sysfs end ************************/
> >
> > @@ -1288,8 +1335,6 @@ static void intel_pstate_hwp_enable(struct cpudata *cpudata)
> >  		cpudata->epp_default = intel_pstate_get_epp(cpudata, 0);
> >  }
> >
> > -#define MSR_IA32_POWER_CTL_BIT_EE	19
> > -
> >  /* Disable energy efficiency optimization */
> >  static void intel_pstate_disable_ee(int cpu)
> >  {
> > --
> > 2.25.4