Skip to contentSkip to conversation

powernv:cpuidle: Enable winkle idle state

PATCH In review Gautham R. Shenoy 3 participants 8 messages Aug 18, 2016 – Aug 25, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover powernv:cpuidle: Enable winkle idle state

From: "Gautham R. Shenoy" <[email protected]>

Hi,

The patches in these series enable support for Winkle idle state in CPU-Idle.

The first patch is a platform-independent CPU-Idle patch that allows CPU-Idle states to be disabled at start (Currently they are all enabled by default).

The second patch adds the winkle enablement for powernv-cpuidle. By default, the winkle idle-state is disabled. It can be enabled by writing zero to the per-cpu cpuidle sysfs control file named "disable".

This series has been lightly tested on a 2-socket POWER8 system and the machine was pretty stable while running kernbench and ebizzy. I didn't see any regressions with those.

I haven't yet evaluated the impact that these patches might have on latency sensitive workloads. I hope to do that in a day or two.

On the power-savings front, I could observe 6-8% additional power-savings when winkle state was enabled on an idle system with SMT=on. With SMT=off, additional idle power-savings observed with winkle enabled were greater than 15%. The numbers indicate that it might be worth the while to pursue this!

Gautham R. Shenoy (2):
cpuidle: Allow idle-states to be disabled at start powernv:cpuidle: Enable winkle idle state in CPU-Idle.

 drivers/cpuidle/cpuidle-powernv.c | 44 ++++++++++++++++++++++++++++++++-------
 drivers/cpuidle/cpuidle.c         |  7 +++++++
 include/linux/cpuidle.h           |  7 ++++++-
 3 files changed, 49 insertions(+), 9 deletions(-)
Signature · 1 line
1.9.4
#1 lore raw

PATCH 1/2 cpuidle: Allow idle-states to be disabled at start

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

The state can be enabled at run time by echo'ing a zero to the sysfs "disable" control file.

---
 drivers/cpuidle/cpuidle.c | 7 +++++++
 include/linux/cpuidle.h   | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)
drivers/cpuidle/cpuidle.c +7 -0
index c73207a..b4debc7 100644
@@ -439,7 +439,14 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev)
 
 static void __cpuidle_device_init(struct cpuidle_device *dev)
 {
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	int i;
+
 	memset(dev->states_usage, 0, sizeof(dev->states_usage));
+	for (i = 0; i < drv->state_count; i++) {
+		if (drv->states[i].disable_use_at_start)
+			dev->states_usage[i].disable = 1;
+	}
 	dev->last_residency = 0;
 }
 
include/linux/cpuidle.h +6 -1
index bb31373..f3fe855 100644
@@ -44,7 +44,12 @@ struct cpuidle_state {
 	int		power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
 	bool		disabled; /* disabled on all CPUs */
-
+	/*
+	 * disable_use_at_start: If true, then this idle state will be
+	 * disabled by default. It can be enabled at runtime using the
+	 * per-cpu cpuidle sysfs control file named "disable".
+	 */
+	bool            disable_use_at_start;
 	int (*enter)	(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index);
Signature · 1 line
1.9.4
#2 lore raw

Re: [RFC/PATCH 1/2] cpuidle: Allow idle-states to be disabled at start

Quoted 7 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

Why do you need to do that ?

Quoted 2 lines

The state can be enabled at run time by echo'ing a zero to the sysfs "disable" control file.

... for each cpu.

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#3 lore raw

Re: [RFC/PATCH 1/2] cpuidle: Allow idle-states to be disabled at start

Quoted 11 lines · Daniel Lezcano
Quoted 7 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

Why do you need to do that ?

I think patch 2/2 explains the reason as it uses this infrastructure

Balbir Singh

#4 lore raw

Re: [RFC/PATCH 1/2] cpuidle: Allow idle-states to be disabled at start

Quoted 12 lines · Balbir Singh
Quoted 9 lines · Daniel Lezcano
Quoted 7 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

Quoted 4 lines
Quoted 2 lines

Why do you need to do that ?

I think patch 2/2 explains the reason as it uses this infrastructure

Ok, let me elaborate the question, I was not clear.

Why the userspace can't setup the system environment at boot time by disabling the state instead of adding extra code to disable it at boot time in the kernel and then re-enable it from userspace ?

-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#5 lore raw

Re: [RFC/PATCH 1/2] cpuidle: Allow idle-states to be disabled at start

Quoted 16 lines · Daniel Lezcano
Quoted 15 lines · Balbir Singh
Quoted 11 lines · Daniel Lezcano
Quoted 7 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

Why do you need to do that ?

Quoted 7 lines
Quoted 1 line

I think patch 2/2 explains the reason as it uses this infrastructure

Ok, let me elaborate the question, I was not clear.

Why the userspace can't setup the system environment at boot time by disabling the state instead of adding extra code to disable it at boot time in the kernel and then re-enable it from userspace ?

Gautham's patches don't want to have those states enabled by default. They are unlikely to be what production systems need, but likely what a knowledgeable person can look into selectively enable for experimentation.

@Gautham?

Balbir Singh.

#6 lore raw

Re: [RFC/PATCH 1/2] cpuidle: Allow idle-states to be disabled at start

Quoted 23 lines · Balbir Singh
Quoted 20 lines · Daniel Lezcano
Quoted 12 lines · Balbir Singh
Quoted 9 lines · Daniel Lezcano
Quoted 7 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

Currently all the idle states registered by a cpu-idle driver are enabled by default. This patch adds a mechanism which allows the driver to hint if an idle-state should start in a disabled state. The cpu-idle core will use this hint to appropriately initialize the usage->disable knob of the CPU device idle state.

Quoted 4 lines
Quoted 2 lines

Why do you need to do that ?

I think patch 2/2 explains the reason as it uses this infrastructure

Ok, let me elaborate the question, I was not clear.

Quoted 8 lines
Quoted 3 lines

Why the userspace can't setup the system environment at boot time by disabling the state instead of adding extra code to disable it at boot time in the kernel and then re-enable it from userspace ?

Gautham's patches don't want to have those states enabled by default. They are unlikely to be what production systems need, but likely what a knowledgeable person can look into selectively enable for experimentation.

Why not invert the logic ?

A knowledgeable person can look into selectively disable for production.

In addition, a kernel command line option to specify which state to disable would be appropriate and beneficial for all existing drivers.

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#7 lore raw

PATCH 2/2 powernv:cpuidle: Enable winkle idle state in CPU-Idle.

In reply to Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

cpu-idle on powernv currently has support for only snooze, nap and fastsleep states. Winkle idle state was excluded due to its large exit-latency.

This patch adds winkle as a cpu-idle state for experimental purposes. This state is disabled at start by default. However, should an adventurous user want to enable it on a particular CPU(s), they can do so by echo'ing a zero into the per-cpu sysfs cpuidle control file named "disable" corresponding to this state.

---
 drivers/cpuidle/cpuidle-powernv.c | 44 ++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 8 deletions(-)
drivers/cpuidle/cpuidle-powernv.c +36 -8
index f7ca891..0437d8a 100644
@@ -20,7 +20,6 @@
 #include <asm/opal.h>
 #include <asm/runlatch.h>
 
-#define POWERNV_THRESHOLD_LATENCY_NS 200000
 
 struct cpuidle_driver powernv_idle_driver = {
 	.name             = "powernv_idle",
@@ -95,6 +94,30 @@ static int fastsleep_loop(struct cpuidle_device *dev,
 
 	return index;
 }
+
+static int winkle_loop(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv,
+				int index)
+{
+	unsigned long old_lpcr = mfspr(SPRN_LPCR);
+	unsigned long new_lpcr;
+
+	if (unlikely(system_state < SYSTEM_RUNNING))
+		return index;
+
+	new_lpcr = old_lpcr;
+	/* Do not exit powersave upon decrementer as we've setup the timer
+	 * offload.
+	 */
+	new_lpcr &= ~LPCR_PECE1;
+
+	mtspr(SPRN_LPCR, new_lpcr);
+	power7_winkle();
+
+	mtspr(SPRN_LPCR, old_lpcr);
+
+	return index;
+}
 #endif
 
 static int stop_loop(struct cpuidle_device *dev,
@@ -246,13 +269,6 @@ static int powernv_add_idle_states(void)
 		"ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states);
 
 	for (i = 0; i < dt_idle_states; i++) {
-		/*
-		 * If an idle state has exit latency beyond
-		 * POWERNV_THRESHOLD_LATENCY_NS then don't use it
-		 * in cpu-idle.
-		 */
-		if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS)
-			continue;
 
 		/*
 		 * Cpuidle accepts exit_latency and target_residency in us.
@@ -301,6 +317,18 @@ static int powernv_add_idle_states(void)
 			powernv_states[nr_idle_states].enter = stop_loop;
 			stop_psscr_table[nr_idle_states] = psscr_val[i];
 		}
+
+		if (flags[i] & OPAL_PM_WINKLE_ENABLED) {
+			int state_idx = nr_idle_states;
+
+			strcpy(powernv_states[state_idx].name, "Winkle");
+			strcpy(powernv_states[state_idx].desc, "Winkle");
+			powernv_states[state_idx].flags =
+						CPUIDLE_FLAG_TIMER_STOP;
+			powernv_states[state_idx].target_residency = 500000;
+			powernv_states[state_idx].enter = winkle_loop;
+			powernv_states[state_idx].disable_use_at_start = true;
+		}
 #endif
 		powernv_states[nr_idle_states].exit_latency =
 				((unsigned int)latency_ns[i]) / 1000;
Signature · 1 line
1.9.4