[patch 0/2] Support for IA32_ENERGY_PERF_BIAS MSR

[patch 0/2] Support for IA32_ENERGY_PERF_BIAS MSR

[venkatesh.pallipadi]

There is a new hardware feature, which lets system software to set
Energy Performance Preference. This is a opaque knob in the form of
IA32_ENERGY_PERF_BIAS MSR, which has a 4 bit Energy Performance
Preference Hint.

The support for this feature is indicated by CPUID.06H.ECX.bit3. Refer to
Intel Architectures Software Developer's Manual for more info.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

-- 

[patch 1/2] x86: Look for IA32_ENERGY_PERF_BIAS support

[venkatesh.pallipadi]

[-- Attachment #1: 0001-x86-Look-for-IA32_ENERGY_PERF_BIAS-support.patch --]
[-- Type: text/plain, Size: 2503 bytes --]

There is a new hardware feature, which lets system software to set
Energy Performance Preference. This is a opaque knob in the form of
IA32_ENERGY_PERF_BIAS MSR, which has a 4 bit Energy Performance
Preference Hint. When supported, hardware can use this hint to resolve
energy-performance tradeoffs. The support for this feature is indicated
by CPUID.06H.ECX.bit3. Refer to Intel Architectures Software Developer's
Manual for more info.

This patch, looks up for the support of this feature. It shows up as
"epb" in /proc/cpuinfo flags, when supported.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---
 arch/x86/include/asm/cpufeature.h          |    1 +
 arch/x86/include/asm/msr-index.h           |    2 ++
 arch/x86/kernel/cpu/addon_cpuid_features.c |    1 +
 3 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 0cd82d0..0e1d52c 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -161,6 +161,7 @@
  */
 #define X86_FEATURE_IDA		(7*32+ 0) /* Intel Dynamic Acceleration */
 #define X86_FEATURE_ARAT	(7*32+ 1) /* Always Running APIC Timer */
+#define X86_FEATURE_EPB		(7*32+ 2) /* IA32_ENERGY_PERF_BIAS support */
 
 /* Virtualization flags: Linux defined */
 #define X86_FEATURE_TPR_SHADOW  (8*32+ 0) /* Intel TPR Shadow */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1cd58cd..feeb918 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -230,6 +230,8 @@
 
 #define MSR_IA32_MISC_ENABLE		0x000001a0
 
+#define MSR_IA32_ENERGY_PERF_BIAS	0x000001b0
+
 /* MISC_ENABLE bits: architectural */
 #define MSR_IA32_MISC_ENABLE_FAST_STRING	(1ULL << 0)
 #define MSR_IA32_MISC_ENABLE_TCC		(1ULL << 1)
diff --git a/arch/x86/kernel/cpu/addon_cpuid_features.c b/arch/x86/kernel/cpu/addon_cpuid_features.c
index 97ad79c..3bc5eda 100644
--- a/arch/x86/kernel/cpu/addon_cpuid_features.c
+++ b/arch/x86/kernel/cpu/addon_cpuid_features.c
@@ -32,6 +32,7 @@ void __cpuinit init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 	static const struct cpuid_bit __cpuinitconst cpuid_bits[] = {
 		{ X86_FEATURE_IDA, CR_EAX, 1, 0x00000006 },
 		{ X86_FEATURE_ARAT, CR_EAX, 2, 0x00000006 },
+		{ X86_FEATURE_EPB, CR_ECX, 3, 0x00000006 },
 		{ X86_FEATURE_NPT,   CR_EDX, 0, 0x8000000a },
 		{ X86_FEATURE_LBRV,  CR_EDX, 1, 0x8000000a },
 		{ X86_FEATURE_SVML,  CR_EDX, 2, 0x8000000a },
-- 
1.6.0.6

-- 

[patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[venkatesh.pallipadi]

[-- Attachment #1: 0002-x86-Driver-to-manage-ENERGY_PERF_BIAS-based-on-cpuf.patch --]
[-- Type: text/plain, Size: 7163 bytes --]

Manage IA32_ENERGY_PERF_BIAS setting.

By default, this driver sets IA32_ENERGY_PERF_BIAS as follows
0 when cpufreq performance governor is being used
15 when cpufreq powersave governor is being used
7 otherwise

There is an option to disable setting IA32_ENERGY_PERF_BIAS using
epb=disable boot option.
There is an option to manual override IA32_ENERGY_PERF_BIAS using
epb=<0..15> where user set energy_perf_bias value will be set,
irrespective of cpufreq governor.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---
 Documentation/kernel-parameters.txt            |    4 +
 arch/x86/kernel/cpu/cpufreq/Kconfig            |    6 +
 arch/x86/kernel/cpu/cpufreq/Makefile           |    1 +
 arch/x86/kernel/cpu/cpufreq/energy_perf_bias.c |  186 ++++++++++++++++++++++++
 4 files changed, 197 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/cpufreq/energy_perf_bias.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 8c666d8..4945add 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			Default value is 0.
 			Value can be changed at runtime via /selinux/enforce.
 
+	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
+			"disable" - Kernel will not modify this MSR
+			<0..15> - Kernel will set this MSR to i/p static value
+
 	ether=		[HW,NET] Ethernet cards parameters
 			This option is obsoleted by the "netdev=" option, which
 			has equivalent usage. See its documentation for details.
diff --git a/arch/x86/kernel/cpu/cpufreq/Kconfig b/arch/x86/kernel/cpu/cpufreq/Kconfig
index f138c6c..1addc05 100644
--- a/arch/x86/kernel/cpu/cpufreq/Kconfig
+++ b/arch/x86/kernel/cpu/cpufreq/Kconfig
@@ -26,6 +26,12 @@ config X86_ACPI_CPUFREQ
 
 	  If in doubt, say N.
 
+config X86_ENERGY_PERF_BIAS
+	def_bool y
+	depends on X86_ACPI_CPUFREQ
+	help
+	  Support for x86 Intel ENERGY_PERF_BIAS MSR
+
 config ELAN_CPUFREQ
 	tristate "AMD Elan SC400 and SC410"
 	select CPU_FREQ_TABLE
diff --git a/arch/x86/kernel/cpu/cpufreq/Makefile b/arch/x86/kernel/cpu/cpufreq/Makefile
index 509296d..5290428 100644
--- a/arch/x86/kernel/cpu/cpufreq/Makefile
+++ b/arch/x86/kernel/cpu/cpufreq/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_X86_SPEEDSTEP_SMI)		+= speedstep-smi.o
 obj-$(CONFIG_X86_SPEEDSTEP_CENTRINO)	+= speedstep-centrino.o
 obj-$(CONFIG_X86_P4_CLOCKMOD)		+= p4-clockmod.o
 obj-$(CONFIG_X86_CPUFREQ_NFORCE2)	+= cpufreq-nforce2.o
+obj-$(CONFIG_X86_ENERGY_PERF_BIAS)	+= energy_perf_bias.o
diff --git a/arch/x86/kernel/cpu/cpufreq/energy_perf_bias.c b/arch/x86/kernel/cpu/cpufreq/energy_perf_bias.c
new file mode 100644
index 0000000..2bd4e74
--- /dev/null
+++ b/arch/x86/kernel/cpu/cpufreq/energy_perf_bias.c
@@ -0,0 +1,186 @@
+/*
+ * x86 IA32_ENERGY_PERF_BIAS MSR driver
+ * This MSR lets software set a Energy Performance Preference, which
+ * can then be used by hardware to make Energy Performance tradeoffs.
+ */
+
+#include <linux/cpu.h>
+#include <linux/init.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/cpufreq.h>
+
+#include <asm/msr.h>
+#include <asm/system.h>
+#include <asm/processor.h>
+
+#define ENERGY_PERF_BIAS_BITS		0xff
+
+#define ENERGY_PERF_BIAS_INVALID	(-1)
+#define ENERGY_PERF_BIAS_PERF		0
+#define ENERGY_PERF_BIAS_ONDEMAND	7
+#define ENERGY_PERF_BIAS_POWER		15
+
+static int epb_override = ENERGY_PERF_BIAS_INVALID; /* User bias override */
+static int epb_disable; /* User disable option */
+
+#define is_epb_override_set() (epb_override != ENERGY_PERF_BIAS_INVALID)
+
+/*
+ * epb=disable
+ * Kernel will not touch ENERGY_PERF_BIAS
+ *
+ * epb=<0..15>
+ * Kernel will leave ENERGY_PERF_BIAS at user specified value, independent of
+ * cpufreq policy
+ *
+ * Default is to change ENERGY_PERF_BIAS based on cpufreq governor
+ */
+static int __init epb_setup(char *str)
+{
+	if (str) {
+		if (!strncmp("disable", str, 7)) {
+			epb_disable = 1;
+		} else if (isdigit(*str)) {
+			unsigned long val;
+			val = (uint) simple_strtoul(str, NULL, 0);
+			if (val >= ENERGY_PERF_BIAS_PERF &&
+			    val <= ENERGY_PERF_BIAS_POWER) {
+				epb_override = (uint) val;
+			}
+		}
+	}
+	return 0;
+}
+__setup("epb=", epb_setup);
+
+static void set_epb_on_cpu(int val, int cpu)
+{
+	val &= ENERGY_PERF_BIAS_BITS;
+	wrmsr_safe_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, val, 0);
+}
+
+/* Policy notifier to hook into cpufreq policy updates */
+static int epb_policy_notifier(struct notifier_block *nb, unsigned long val,
+				void *data)
+{
+	int cpu;
+	int epb_val;
+	struct cpufreq_policy *policy = data;
+	struct cpufreq_governor *gov;
+
+	if (val != CPUFREQ_NOTIFY)
+		return 0;
+
+	if (!policy || !policy->governor)
+		return 0;
+
+	cpu = policy->cpu;
+	gov = policy->governor;
+
+	if (!strncmp(gov->name, "performance", strlen("performance")))
+		epb_val = ENERGY_PERF_BIAS_PERF;
+	else if (!strncmp(gov->name, "powersave", strlen("powersave")))
+		epb_val = ENERGY_PERF_BIAS_POWER;
+	else
+		epb_val = ENERGY_PERF_BIAS_ONDEMAND;
+
+	set_epb_on_cpu(epb_val, cpu);
+	return 0;
+}
+
+static struct notifier_block policy_nb = {
+        .notifier_call = epb_policy_notifier,
+};
+
+static void epb_cpu_online(int cpu)
+{
+	set_epb_on_cpu(epb_override, cpu);
+}
+
+/* Resume notifier to update the MSR on boot CPU on resume */
+static int epb_resume(struct sys_device *sys_dev)
+{
+        unsigned int cpu = sys_dev->id;
+
+	if (cpu != 0)
+		return 0;
+
+	epb_cpu_online(cpu);
+	return 0;
+}
+
+static struct sysdev_driver epb_sysdev_driver = {
+        .resume        = epb_resume,
+};
+
+/* Online notifier to update the MSR on all non-boot CPU on resume and online */
+static int __cpuinit epb_cpu_notifier(struct notifier_block *nfb,
+					unsigned long action, void *hcpu)
+{
+	int cpu = (long)hcpu;
+
+	if (action == CPU_ONLINE || action == CPU_ONLINE_FROZEN)
+		epb_cpu_online(cpu);
+
+	return 0;
+}
+
+static struct notifier_block cpu_nb = {
+	.notifier_call = epb_cpu_notifier,
+};
+
+
+static int __init epb_init(void)
+{
+	int ret;
+	int cpu;
+
+	if (!boot_cpu_has(X86_FEATURE_EPB) || epb_disable) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	if (!is_epb_override_set()) {
+		ret = cpufreq_register_notifier(&policy_nb,
+						CPUFREQ_POLICY_NOTIFIER);
+		goto err;
+	} else {
+		ret = sysdev_driver_register(&cpu_sysdev_class,
+						&epb_sysdev_driver);
+		if (ret)
+			goto err;
+
+		ret = register_cpu_notifier(&cpu_nb);
+		if (ret)
+			goto err_sysdev_driver;
+
+		for_each_online_cpu(cpu)
+			set_epb_on_cpu(epb_override, cpu);
+	}
+	return 0;
+
+err_sysdev_driver:
+	sysdev_driver_unregister(&cpu_sysdev_class, &epb_sysdev_driver);
+err:
+	return ret;
+}
+
+static void __exit epb_exit(void)
+{
+	if (!boot_cpu_has(X86_FEATURE_EPB) || epb_disable)
+		return;
+
+	if (!is_epb_override_set()) {
+		cpufreq_unregister_notifier(&policy_nb,
+						CPUFREQ_POLICY_NOTIFIER);
+	} else {
+		sysdev_driver_unregister(&cpu_sysdev_class, &epb_sysdev_driver);
+		unregister_cpu_notifier(&cpu_nb);
+	}
+}
+
+__initcall(epb_init);
+__exitcall(epb_exit);
-- 
1.6.0.6

-- 

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Randy Dunlap]

On 03/02/10 16:06, venkatesh.pallipadi@intel.com wrote:

{bah, more difficult to review an attachment, so no leading '>'}


diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 8c666d8..4945add 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			Default value is 0.
 			Value can be changed at runtime via /selinux/enforce.
 
+	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
+			"disable" - Kernel will not modify this MSR
+			<0..15> - Kernel will set this MSR to i/p static value
+


Should be more like:

	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
			Format: { disable | <0...15> }
			"disable" - Kernel will not modify this MSR
			<0..15> - Kernel will set this MSR to i/p static value


But what is "i/p"?  Use whatever word it should be, please.
What do the values mean?
And what does IA32 have to do with this?  does it not apply to x86_64?

thanks,
-- 
~Randy

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Matthew Garrett]

Have you done any measurements with this bit being set dynamically by 
ondemand depending on the target frequency?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pallipadi, Venkatesh]

On Tue, 2010-03-02 at 16:30 -0800, Randy Dunlap wrote:
> On 03/02/10 16:06, venkatesh.pallipadi@intel.com wrote:
> 
> {bah, more difficult to review an attachment, so no leading '>'}

Sorry. Not sure why/how you are seeing attachment issue. I used quilt
mail to send this out (I have used this method earlier as well) and I do
see patches inline .

> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 8c666d8..4945add 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
>  			Default value is 0.
>  			Value can be changed at runtime via /selinux/enforce.
>  
> +	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
> +			"disable" - Kernel will not modify this MSR
> +			<0..15> - Kernel will set this MSR to i/p static value
> +
> 
> 
> Should be more like:
> 
> 	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
> 			Format: { disable | <0...15> }

OK. Will change.

> 			"disable" - Kernel will not modify this MSR
> 			<0..15> - Kernel will set this MSR to i/p static value
> 
> 
> But what is "i/p"?  Use whatever word it should be, please.

I meant input. will change to something like "specified value"

> What do the values mean?

The way MSR is defined, the value is opaque. It can be used for
different optimizations in different CPUs.

> And what does IA32 have to do with this?  does it not apply to x86_64?

IA32_ in any MSR name is the convention that is generally followed for
all MSRs in Intel SDM. It means that the MSR is architectural (can be
detected with CPUID or some other standard way and not CPU Model
specific.

Thanks,
Venki

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pallipadi, Venkatesh]

On Wed, 2010-03-03 at 08:40 -0800, Matthew Garrett wrote:
> Have you done any measurements with this bit being set dynamically by 
> ondemand depending on the target frequency?
> 

The not so good part about this feature is that its setting is opaque.
Different CPUs can use this value for different optimizations. So, in
future we may have to change the value of this MSR based on frequency or
other metrics. But, for CPUs which support this MSR today, changing the
value with frequency do not make any difference.

Thanks,
Venki

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pavel Machek]

Hi!

> index 8c666d8..4945add 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
>  			Default value is 0.
>  			Value can be changed at runtime via /selinux/enforce.
>  
> +	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
> +			"disable" - Kernel will not modify this MSR
> +			<0..15> - Kernel will set this MSR to i/p static value
> +
> 
> 
> Should be more like:
> 
> 	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
> 			Format: { disable | <0...15> }
> 			"disable" - Kernel will not modify this MSR
> 			<0..15> - Kernel will set this MSR to i/p static value
> 
> 
> But what is "i/p"?  Use whatever word it should be, please.
> What do the values mean?
> And what does IA32 have to do with this?  does it not apply to x86_64?

Exactly. This is end user documentation, it should not even talk about
MSRs. Tell us what the setting does...

Also... does it make change to tweak the setting during runtime? Maybe
different settings for AC and battery power?

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pallipadi, Venkatesh]

On Wed, 2010-03-03 at 13:57 -0800, Pavel Machek wrote:
> Hi!
> 
> > index 8c666d8..4945add 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
> >  			Default value is 0.
> >  			Value can be changed at runtime via /selinux/enforce.
> >  
> > +	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
> > +			"disable" - Kernel will not modify this MSR
> > +			<0..15> - Kernel will set this MSR to i/p static value
> > +
> > 
> > 
> > Should be more like:
> > 
> > 	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
> > 			Format: { disable | <0...15> }
> > 			"disable" - Kernel will not modify this MSR
> > 			<0..15> - Kernel will set this MSR to i/p static value
> > 
> > 
> > But what is "i/p"?  Use whatever word it should be, please.
> > What do the values mean?
> > And what does IA32 have to do with this?  does it not apply to x86_64?
> 
> Exactly. This is end user documentation, it should not even talk about
> MSRs. Tell us what the setting does...

The not so good part of this feature is that the setting here is opaque.
Software can set this based on its preference, for example 0 for
performance 15 for power and 7 for balanced. Different CPUs can use this
information to do different optimizations or power-performance tradeoffs
in the hardware. The only thing that user knows here is that there is
this dial with 16 possible values. I can remove the MSR name here. But,
I think that will end up confusing the end user on what this thing is
and how it is related to all the other tunables we have in the kernel.
Having the MSR name gives a hint.

Also, the expectation here is that kernel will do the right thing by
default. The option here is to the user who_knows_what_he_is_doing to
override the kernel default.

> 
> Also... does it make change to tweak the setting during runtime? Maybe
> different settings for AC and battery power?

Yes. Matthew mentioned in other response aboue setting this based on
freq. For the CPUs that support this feature currently, we don't see
advantage in setting this feature at run time.

Thanks,
Venki

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pavel Machek]

> On Wed, 2010-03-03 at 13:57 -0800, Pavel Machek wrote:
> > Hi!
> > 
> > > index 8c666d8..4945add 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -749,6 +749,10 @@ and is between 256 and 4096 characters. It is defined in the file
> > >  			Default value is 0.
> > >  			Value can be changed at runtime via /selinux/enforce.
> > >  
> > > +	epb		[X86] Control IA32_ENERGY_PERF_BIAS setting
> > > +			"disable" - Kernel will not modify this MSR
> > > +			<0..15> - Kernel will set this MSR to i/p static value
> > > +
> > > 
> > > 
> > > Should be more like:
> > > 
> > > 	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
> > > 			Format: { disable | <0...15> }
> > > 			"disable" - Kernel will not modify this MSR
> > > 			<0..15> - Kernel will set this MSR to i/p static value
> > > 
> > > 
> > > But what is "i/p"?  Use whatever word it should be, please.
> > > What do the values mean?
> > > And what does IA32 have to do with this?  does it not apply to x86_64?
> > 
> > Exactly. This is end user documentation, it should not even talk about
> > MSRs. Tell us what the setting does...
> 
> The not so good part of this feature is that the setting here is opaque.
> Software can set this based on its preference, for example 0 for
> performance 15 for power and 7 for balanced. Different CPUs can use this
> information to do different optimizations or power-performance tradeoffs
> in the hardware. The only thing that user knows here is that there is
> this dial with 16 possible values. I can remove the MSR name here. But,
> I think that will end up confusing the end user on what this thing is
> and how it is related to all the other tunables we have in the kernel.
> Having the MSR name gives a hint.

You should say what the setting does; you can mention below what MSR
it corresponds to, but "Control IA32_ENERGY_PERF_BIAS setting" is not
suitable user documentation.

> Also, the expectation here is that kernel will do the right thing by
> default. The option here is to the user who_knows_what_he_is_doing to
> override the kernel default.

You did not give user enough information to do anything intelligent...

> > Also... does it make change to tweak the setting during runtime? Maybe
> > different settings for AC and battery power?
> 
> Yes. Matthew mentioned in other response aboue setting this based on
> freq. For the CPUs that support this feature currently, we don't see
> advantage in setting this feature at run time.

If the feature is useless, then why set it at all?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Pavel Machek [mailto:pavel@ucw.cz] 
>Sent: Friday, March 05, 2010 1:20 AM
>To: Pallipadi, Venkatesh
>Cc: Randy Dunlap; Ingo Molnar; H Peter Anvin; Thomas Gleixner; 
>Len Brown; Dave Jones; linux-kernel@vger.kernel.org; 
>linux-acpi@vger.kernel.org
>Subject: Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on 
>cpufreq governor
>
>> On Wed, 2010-03-03 at 13:57 -0800, Pavel Machek wrote:
>> > Hi!
>> > 
>> > > index 8c666d8..4945add 100644
>> > > --- a/Documentation/kernel-parameters.txt
>> > > +++ b/Documentation/kernel-parameters.txt
>> > > @@ -749,6 +749,10 @@ and is between 256 and 4096 
>characters. It is defined in the file
>> > >  			Default value is 0.
>> > >  			Value can be changed at runtime 
>via /selinux/enforce.
>> > >  
>> > > +	epb		[X86] Control 
>IA32_ENERGY_PERF_BIAS setting
>> > > +			"disable" - Kernel will not 
>modify this MSR
>> > > +			<0..15> - Kernel will set this 
>MSR to i/p static value
>> > > +
>> > > 
>> > > 
>> > > Should be more like:
>> > > 
>> > > 	epb=		[X86] Control IA32_ENERGY_PERF_BIAS setting
>> > > 			Format: { disable | <0...15> }
>> > > 			"disable" - Kernel will not modify this MSR
>> > > 			<0..15> - Kernel will set this MSR to 
>i/p static value
>> > > 
>> > > 
>> > > But what is "i/p"?  Use whatever word it should be, please.
>> > > What do the values mean?
>> > > And what does IA32 have to do with this?  does it not 
>apply to x86_64?
>> > 
>> > Exactly. This is end user documentation, it should not 
>even talk about
>> > MSRs. Tell us what the setting does...
>> 
>> The not so good part of this feature is that the setting 
>here is opaque.
>> Software can set this based on its preference, for example 0 for
>> performance 15 for power and 7 for balanced. Different CPUs 
>can use this
>> information to do different optimizations or 
>power-performance tradeoffs
>> in the hardware. The only thing that user knows here is that there is
>> this dial with 16 possible values. I can remove the MSR name 
>here. But,
>> I think that will end up confusing the end user on what this thing is
>> and how it is related to all the other tunables we have in 
>the kernel.
>> Having the MSR name gives a hint.
>
>You should say what the setting does; you can mention below what MSR
>it corresponds to, but "Control IA32_ENERGY_PERF_BIAS setting" is not
>suitable user documentation.
>
>> Also, the expectation here is that kernel will do the right thing by
>> default. The option here is to the user who_knows_what_he_is_doing to
>> override the kernel default.
>
>You did not give user enough information to do anything intelligent...

I have rephrased it in the newer version sent yday with more info.

>> > Also... does it make change to tweak the setting during 
>runtime? Maybe
>> > different settings for AC and battery power?
>> 
>> Yes. Matthew mentioned in other response aboue setting this based on
>> freq. For the CPUs that support this feature currently, we don't see
>> advantage in setting this feature at run time.
>
>If the feature is useless, then why set it at all?

I just said changing it at run time doesn't give us benefits. Not that
the feature is useless. Having the default value for the tunable in
mid-range does increase energy-efficiency than the tunable being
at performance level.

Thanks,
Venki

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pavel Machek]

Hi!

> >You should say what the setting does; you can mention below what MSR
> >it corresponds to, but "Control IA32_ENERGY_PERF_BIAS setting" is not
> >suitable user documentation.
> >
> >> Also, the expectation here is that kernel will do the right thing by
> >> default. The option here is to the user who_knows_what_he_is_doing to
> >> override the kernel default.
> >
> >You did not give user enough information to do anything intelligent...
> 
> I have rephrased it in the newer version sent yday with more info.

Good.

> >> > Also... does it make change to tweak the setting during 
> >runtime? Maybe
> >> > different settings for AC and battery power?
> >> 
> >> Yes. Matthew mentioned in other response aboue setting this based on
> >> freq. For the CPUs that support this feature currently, we don't see
> >> advantage in setting this feature at run time.
> >
> >If the feature is useless, then why set it at all?
> 
> I just said changing it at run time doesn't give us benefits. Not
>that

That can be only true if it does not give benefits period... AC and
battery power are quite different scenarios.

> the feature is useless. Having the default value for the tunable in
> mid-range does increase energy-efficiency than the tunable being
> at performance level.

So... what does it really do? What is the difference in power
consumption, and what is the difference in performance?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Matthew Garrett]

On Fri, Mar 05, 2010 at 09:40:29PM +0100, Pavel Machek wrote:

> That can be only true if it does not give benefits period... AC and
> battery power are quite different scenarios.

No, they're not.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pavel Machek]

On Fri 2010-03-05 20:55:22, Matthew Garrett wrote:
> On Fri, Mar 05, 2010 at 09:40:29PM +0100, Pavel Machek wrote:
> 
> > That can be only true if it does not give benefits period... AC and
> > battery power are quite different scenarios.
> 
> No, they're not.

Yes, they are.

Would you care to elaborate? I may very well want top power on AC
power, and max powersavings on battery; most people do.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Matthew Garrett]

On Fri, Mar 05, 2010 at 10:13:30PM +0100, Pavel Machek wrote:
> On Fri 2010-03-05 20:55:22, Matthew Garrett wrote:
> > On Fri, Mar 05, 2010 at 09:40:29PM +0100, Pavel Machek wrote:
> > 
> > > That can be only true if it does not give benefits period... AC and
> > > battery power are quite different scenarios.
> > 
> > No, they're not.
> 
> Yes, they are.
> 
> Would you care to elaborate? I may very well want top power on AC
> power, and max powersavings on battery; most people do.

You may want that. But power constraints aren't limited to battery, and 
being on battery doesn't inherently mean that you're power constrained. 
Mixing these concepts results in all kinds of issues.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 2/2] x86: Manage ENERGY_PERF_BIAS based on cpufreq governor

[Pavel Machek]

On Fri 2010-03-05 22:49:09, Matthew Garrett wrote:
> On Fri, Mar 05, 2010 at 10:13:30PM +0100, Pavel Machek wrote:
> > On Fri 2010-03-05 20:55:22, Matthew Garrett wrote:
> > > On Fri, Mar 05, 2010 at 09:40:29PM +0100, Pavel Machek wrote:
> > > 
> > > > That can be only true if it does not give benefits period... AC and
> > > > battery power are quite different scenarios.
> > > 
> > > No, they're not.
> > 
> > Yes, they are.
> > 
> > Would you care to elaborate? I may very well want top power on AC
> > power, and max powersavings on battery; most people do.
> 
> You may want that. But power constraints aren't limited to battery, and 
> being on battery doesn't inherently mean that you're power constrained. 
> Mixing these concepts results in all kinds of issues.

That's of course true.

But if something gives so limited benefit that it does not make sense
to tweak between power constrained and not-power-constrained
environments (I used AC and battery as an example), it is useless.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html