[RFC/PATCH 0/2] powernv:cpuidle: Enable winkle idle state

From: "Gautham R. Shenoy" <redacted>

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(-)

-- 
1.9.4

From: "Gautham R. Shenoy" <redacted>

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.

Signed-off-by: Gautham R. Shenoy <redacted>
---
 drivers/cpuidle/cpuidle.c | 7 +++++++
 include/linux/cpuidle.h   | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c73207a..b4debc7 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c

@@ -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;
 }
 

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb31373..f3fe855 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h

@@ -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);

-- 
1.9.4

From: "Gautham R. Shenoy" <redacted>

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.

Signed-off-by: Gautham R. Shenoy <redacted>
---
 drivers/cpuidle/cpuidle-powernv.c | 44 ++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index f7ca891..0437d8a 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c

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

-- 
1.9.4

On 08/19/2016 12:26 AM, Gautham R. Shenoy wrote:

From: "Gautham R. Shenoy" <redacted>

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 ?

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

... for each cpu.


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

On 25/08/16 00:44, Daniel Lezcano wrote:

On 08/19/2016 12:26 AM, Gautham R. Shenoy wrote:

quoted

From: "Gautham R. Shenoy" <redacted>

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

On 08/24/2016 04:48 PM, Balbir Singh wrote:

On 25/08/16 00:44, Daniel Lezcano wrote:

quoted

On 08/19/2016 12:26 AM, Gautham R. Shenoy wrote:

quoted

From: "Gautham R. Shenoy" <redacted>

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

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


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

On 25/08/16 01:06, Daniel Lezcano wrote:

On 08/24/2016 04:48 PM, Balbir Singh wrote:

quoted

On 25/08/16 00:44, Daniel Lezcano wrote:

quoted

On 08/19/2016 12:26 AM, Gautham R. Shenoy wrote:

quoted

From: "Gautham R. Shenoy" <redacted>

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

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.

On 08/25/2016 03:46 PM, Balbir Singh wrote:

On 25/08/16 01:06, Daniel Lezcano wrote:

quoted

On 08/24/2016 04:48 PM, Balbir Singh wrote:

quoted

On 25/08/16 00:44, Daniel Lezcano wrote:

quoted

On 08/19/2016 12:26 AM, Gautham R. Shenoy wrote:

quoted

From: "Gautham R. Shenoy" <redacted>

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

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.

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.


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