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:

quoted

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.

[...]

quoted

+``energy_efficiency_enable``
+	This attribute is only present on platforms, which has CPUs
matching

						which have

Thanks, I will fix that.

quoted

+	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.

[...]

quoted

@@ -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.

[...]

quoted

+
+	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.

quoted

+	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".

?

quoted

+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