[v2 PATCH 0/2]: cpuidle: Introducing cpuidle infrastructure to POWER

Hi,



"Cpuidle" is a CPU Power Management infrastrusture which helps manage
idle CPUs in a clean and efficient manner. The architecture can register
its driver (in this case, pseries_idle driver) so that it subscribes for
cpuidle feature. Cpuidle has a set of governors (ladder and menu),
which will decide the best idle state to be chosen for the current situation,
based on heuristics, and calculates the expected residency time
for the current idle state. So based on this, the cpu is put into
the right idle state.

Currently, cpuidle infrasture is exploited by ACPI to choose between
the available ACPI C-states. This patch-set is aimed at enabling
cpuidle for powerpc and provides a sample implementation for pseries.

Currently, in the pseries_dedicated_idle_sleep(), the processor would
poll for a time period, which is called the snooze, and only then it
is ceded, which would put the processor in nap state. Cpuidle aims at
separating this into 2 different idle states. Based on the expected
residency time predicted by the cpuidle governor, the idle state is
chosen directly. So, choosing to enter the nap state directly based on
the decision made by cpuidle would avoid unnecessary snoozing before
entering nap.

This patch-set tries to achieve the above objective by introducing a
pseries processor idle driver called pseries_idle_driver in
arch/powerpc/platform/pseries/processor_idle.c, which implements the
idle loop which would replace the pseries_dedicated_idle_sleep()
when cpuidle is enabled.

Experiment conducted:
----------------------

The following experiment was conducted on a completely idle JS22 blade,
to prove that using cpuidle infrastructure, the amount of nap time increases.

Nap and snooze times were sampled for all the cpus.
For a window of 1000 samples, When cpuidle was enabled,
the total nap time was of the order of a few seconds (5-10s), whereas
the total snooze time was of the order of a few milliseconds(10-30 ms).

When cpuidle infrastructure was disabled and the regular
pseries_dedicated_idle_sleep() idle loop was used, the snooze time itself
was of the order of hundreds of milliseconds. (100 - 500 ms).
This is clearly due to unnecessary snoozing before napping even on a
completely idle system.


The previous post in this area can be found at
http://lkml.org/lkml/2009/8/19/150

Changes from the previous version:
----------------------------------

Earlier I used the the name TPMD (Thermal and Power Management Devices)
to refer to this module which hooks on to the cpuidle infrastructure.
I have renamed this to Pseries Processor Idle, in order to avoid confusion.


Patches included in this set:
------------------------------
PATCH 1/2 - Enable cpuidle for pSeries.
PATCH 2/2 - Implement Pseries Processor Idle idle module


Any feedback on the overall design and idea is immensely valuable.

--arun

* Arun R Bharadwaj [off-list ref] [2009-08-26 16:37:21]:

This patch enables the cpuidle option in Kconfig for pSeries.

Currently cpuidle infrastructure is enabled only for x86.
This code is almost completely borrowed from x86 to enable
cpuidle for pSeries.

Signed-off-by: Arun R Bharadwaj <redacted>
---
 arch/powerpc/Kconfig                   |   17 +++++++++++++++++
 arch/powerpc/include/asm/system.h      |    2 ++
 arch/powerpc/platforms/pseries/setup.c |   21 +++++++++++++++++++++
 3 files changed, 40 insertions(+)

Index: linux.trees.git/arch/powerpc/Kconfig
===================================================================

--- linux.trees.git.orig/arch/powerpc/Kconfig
+++ linux.trees.git/arch/powerpc/Kconfig

@@ -88,6 +88,9 @@ config ARCH_HAS_ILOG2_U64
 	bool
 	default y if 64BIT
 
+config ARCH_HAS_CPU_IDLE_WAIT
+	def_bool y
+
 config GENERIC_HWEIGHT
 	bool
 	default y

@@ -243,6 +246,20 @@ source "kernel/Kconfig.freezer"
 source "arch/powerpc/sysdev/Kconfig"
 source "arch/powerpc/platforms/Kconfig"
 
+menu "Power management options"
+
+source "drivers/cpuidle/Kconfig"
+
+config PSERIES_PROCESSOR_IDLE
+	bool "Idle Power Management Support for pSeries"
+	depends on PPC_PSERIES && CPU_IDLE
+	default y
+	help
+	  Idle Power Management Support for pSeries. This hooks onto cpuidle
+	  infrastructure to help in idle cpu power management.
+
+endmenu
+
 menu "Kernel options"
 
 config HIGHMEM

Index: linux.trees.git/arch/powerpc/platforms/pseries/setup.c
===================================================================

--- linux.trees.git.orig/arch/powerpc/platforms/pseries/setup.c
+++ linux.trees.git/arch/powerpc/platforms/pseries/setup.c

@@ -278,6 +278,27 @@ static struct notifier_block pci_dn_reco
 	.notifier_call = pci_dn_reconfig_notifier,
 };
 
+static void do_nothing(void *unused)
+{
+}
+
+/*
+ * cpu_idle_wait - Used to ensure that all the CPUs discard old value of
+ * pm_idle and update to new pm_idle value. Required while changing pm_idle
+ * handler on SMP systems.
+ *
+ * Caller must have changed pm_idle to the new value before the call. Old
+ * pm_idle value will not be used by any CPU after the return of this function.
+ */
+void cpu_idle_wait(void)
+{
+	/* Ensure that new value of pm_idle is set before proceeding */
+	smp_mb();
+	/* kick all the CPUs so that they exit out of pm_idle */
+	smp_call_function(do_nothing, NULL, 1);
+}
+EXPORT_SYMBOL_GPL(cpu_idle_wait);
+
 static void __init pSeries_setup_arch(void)
 {
 	/* Discover PIC type and setup ppc_md accordingly */

Index: linux.trees.git/arch/powerpc/include/asm/system.h
===================================================================

--- linux.trees.git.orig/arch/powerpc/include/asm/system.h
+++ linux.trees.git/arch/powerpc/include/asm/system.h

@@ -546,5 +546,7 @@ extern void account_system_vtime(struct 
 
 extern struct dentry *powerpc_debugfs_root;
 
+void cpu_idle_wait(void);
+
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_SYSTEM_H */

* Arun R Bharadwaj [off-list ref] [2009-08-26 16:37:21]:

This patch creates arch/powerpc/platforms/pseries/processor_idle.c,
which implements the cpuidle infrastructure for pseries.
It implements a pseries_cpuidle_loop() which would be the main idle loop
called from cpu_idle(). It makes decision of entering either snooze or nap
state based on the decision taken by the cpuidle governor.

Signed-off-by: Arun R Bharadwaj <redacted>
---
 arch/powerpc/platforms/pseries/Makefile         |    1 
 arch/powerpc/platforms/pseries/processor_idle.c |  181 ++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/pseries.h        |   14 +
 arch/powerpc/platforms/pseries/setup.c          |    3 
 4 files changed, 196 insertions(+), 3 deletions(-)

Index: linux.trees.git/arch/powerpc/platforms/pseries/Makefile
===================================================================

--- linux.trees.git.orig/arch/powerpc/platforms/pseries/Makefile
+++ linux.trees.git/arch/powerpc/platforms/pseries/Makefile

@@ -26,3 +26,4 @@ obj-$(CONFIG_HCALL_STATS)	+= hvCall_inst
 obj-$(CONFIG_PHYP_DUMP)	+= phyp_dump.o
 obj-$(CONFIG_CMM)		+= cmm.o
 obj-$(CONFIG_DTL)		+= dtl.o
+obj-$(CONFIG_PSERIES_PROCESSOR_IDLE)	+= processor_idle.o

Index: linux.trees.git/arch/powerpc/platforms/pseries/pseries.h
===================================================================

--- linux.trees.git.orig/arch/powerpc/platforms/pseries/pseries.h
+++ linux.trees.git/arch/powerpc/platforms/pseries/pseries.h

@@ -10,6 +10,8 @@
 #ifndef _PSERIES_PSERIES_H
 #define _PSERIES_PSERIES_H
 
+#include <linux/cpuidle.h>
+
 extern void __init fw_feature_init(const char *hypertas, unsigned long len);
 
 struct pt_regs;

@@ -40,4 +42,16 @@ extern unsigned long rtas_poweron_auto;
 
 extern void find_udbg_vterm(void);
 
+DECLARE_PER_CPU(unsigned long, smt_snooze_delay);
+
+#ifdef CONFIG_PSERIES_PROCESSOR_IDLE
+struct pseries_processor_power {
+	struct cpuidle_device dev;
+	int count;
+	int id;
+};
+
+extern struct cpuidle_driver pseries_idle_driver;
+#endif
+
 #endif /* _PSERIES_PSERIES_H */

Index: linux.trees.git/arch/powerpc/platforms/pseries/processor_idle.c
===================================================================

--- /dev/null
+++ linux.trees.git/arch/powerpc/platforms/pseries/processor_idle.c

@@ -0,0 +1,181 @@
+/*
+ *  processor_idle - idle state cpuidle driver.
+ *  Adapted from drivers/acpi/processor_idle.c
+ *
+ *  Arun R Bharadwaj <arun@linux.vnet.ibm.com>
+ *
+ *  Copyright (C) 2009 IBM Corporation.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
+#include <linux/cpuidle.h>
+
+#include <asm/paca.h>
+#include <asm/reg.h>
+#include <asm/machdep.h>
+
+#include "plpar_wrappers.h"
+#include "pseries.h"
+
+MODULE_AUTHOR("Arun R Bharadwaj");
+MODULE_DESCRIPTION("pSeries Idle State Driver");
+MODULE_LICENSE("GPL");
+
+struct cpuidle_driver pseries_idle_driver = {
+	.name =		"pseries_idle",
+	.owner =	THIS_MODULE,
+};
+
+void (*pm_idle)(void);
+EXPORT_SYMBOL_GPL(pm_idle);
+
+DEFINE_PER_CPU(struct pseries_processor_power, power);
+
+#define IDLE_STATE_COUNT	2
+
+static int pseries_idle_init(struct pseries_processor_power *power)
+{
+	return cpuidle_register_device(&power->dev);
+}
+
+static void snooze(void)
+{
+	local_irq_enable();
+	set_thread_flag(TIF_POLLING_NRFLAG);
+	while (!need_resched()) {
+		ppc64_runlatch_off();
+		HMT_low();
+		HMT_very_low();
+	}
+	HMT_medium();
+	clear_thread_flag(TIF_POLLING_NRFLAG);
+	smp_mb();
+	local_irq_disable();
+}
+
+static void nap(void)
+{
+	ppc64_runlatch_off();
+	HMT_medium();
+	cede_processor();
+}
+
+static int pseries_cpuidle_loop(struct cpuidle_device *dev,
+				struct cpuidle_state *st)
+{
+	ktime_t t1, t2;
+	s64 diff;
+	int ret;
+	unsigned long in_purr, out_purr;
+
+	get_lppaca()->idle = 1;
+	get_lppaca()->donate_dedicated_cpu = 1;
+	in_purr = mfspr(SPRN_PURR);
+
+	t1 = ktime_get();
+
+	if (strcmp(st->desc, "snooze") == 0)
+		snooze();
+	else
+		nap();
+
+	t2 = ktime_get();
+	diff = ktime_to_us(ktime_sub(t2, t1));
+	if (diff > INT_MAX)
+		diff = INT_MAX;
+
+	ret = (int) diff;
+
+	out_purr = mfspr(SPRN_PURR);
+	get_lppaca()->wait_state_cycles += out_purr - in_purr;
+	get_lppaca()->donate_dedicated_cpu = 0;
+	get_lppaca()->idle = 0;
+
+	return ret;
+}
+
+static int pseries_setup_cpuidle(struct pseries_processor_power *power)
+{
+	int i;
+	struct cpuidle_state *state;
+	struct cpuidle_device *dev = &power->dev;
+
+	dev->cpu = power->id;
+
+	dev->enabled = 0;
+	for (i = 0; i < IDLE_STATE_COUNT; i++) {
+		state = &dev->states[i];
+
+		snprintf(state->name, CPUIDLE_NAME_LEN, "IDLE%d", i);
+		state->enter = pseries_cpuidle_loop;
+
+		switch (i) {
+		case 0:
+			strncpy(state->desc, "snooze", CPUIDLE_DESC_LEN);
+			state->exit_latency = 0;
+			state->target_residency = 0;
+			break;
+
+		case 1:
+			strncpy(state->desc, "nap", CPUIDLE_DESC_LEN);
+			state->exit_latency = 1;
+			state->target_residency =
+					__get_cpu_var(smt_snooze_delay);
+			break;
+		}
+	}
+
+	power->dev.state_count = i;
+	return 0;
+}
+
+static int pseries_get_power_info(struct pseries_processor_power *power,
+				int cpu)
+{
+	power->id = cpu;
+	power->count = IDLE_STATE_COUNT;
+	return 0;
+}
+
+static int __init pseries_processor_idle_init(void)
+{
+	int cpu;
+	int result = cpuidle_register_driver(&pseries_idle_driver);
+
+	if (result < 0)
+		return result;
+
+	printk(KERN_DEBUG "pSeries idle driver registered\n");
+
+	for_each_online_cpu(cpu) {
+		pseries_get_power_info(&per_cpu(power, cpu), cpu);
+		pseries_setup_cpuidle(&per_cpu(power, cpu));
+		pseries_idle_init(&per_cpu(power, cpu));
+	}
+
+	printk(KERN_DEBUG "Using cpuidle idle loop\n");
+	ppc_md.power_save = pm_idle;
+	return 0;
+}
+
+late_initcall(pseries_processor_idle_init);

Index: linux.trees.git/arch/powerpc/platforms/pseries/setup.c
===================================================================

--- linux.trees.git.orig/arch/powerpc/platforms/pseries/setup.c
+++ linux.trees.git/arch/powerpc/platforms/pseries/setup.c

@@ -521,9 +521,6 @@ static int __init pSeries_probe(void)
 	return 1;
 }
 
-
-DECLARE_PER_CPU(unsigned long, smt_snooze_delay);
-
 static void pseries_dedicated_idle_sleep(void)
 { 
 	unsigned int cpu = smp_processor_id();

On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote:

+void (*pm_idle)(void);
+EXPORT_SYMBOL_GPL(pm_idle);

Seriously.. this caused plenty problems over on x86 and you're doing the
exact same dumb thing?

* Peter Zijlstra [off-list ref] [2009-08-26 13:27:18]:

On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote:

quoted

+void (*pm_idle)(void);
+EXPORT_SYMBOL_GPL(pm_idle);

Seriously.. this caused plenty problems over on x86 and you're doing the
exact same dumb thing?

Hi Peter,

Cpuidle assumes pm_idle to be the default idle power management
function. So i should either do this, or change the stuff in cpuidle.c
so that it is more abstract.

--arun

On Wed, 2009-08-26 at 17:02 +0530, Arun R Bharadwaj wrote:

* Peter Zijlstra [off-list ref] [2009-08-26 13:27:18]:

quoted

On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote:

quoted

+void (*pm_idle)(void);
+EXPORT_SYMBOL_GPL(pm_idle);

Seriously.. this caused plenty problems over on x86 and you're doing the
exact same dumb thing?

Hi Peter,

Cpuidle assumes pm_idle to be the default idle power management
function. So i should either do this, or change the stuff in cpuidle.c
so that it is more abstract.

I would much prefer the latter, I've been telling the x86 power folks to
fix this like forever, but they never seem to get around to it.

They even tried adding a second such unmanaged function pointer for
play-dead, instead of integrating everything into a single management
interface.

On Wed, 2009-08-26 at 13:27 +0200, Peter Zijlstra wrote:

On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote:

quoted

+void (*pm_idle)(void);
+EXPORT_SYMBOL_GPL(pm_idle);

Seriously.. this caused plenty problems over on x86 and you're doing the
exact same dumb thing?

I already said I didn't want this export.

First thing first: We already have a ppc_md.power_save() callback,
filled by the platform. which implements the "low level" part of idle on
powerpc (ie, the actual putting of the CPU into some kind of wait state)
and is called by our idle loop.

We -also- have a higher level ppc_md.idle_loop() which allows the
platform to completely override the idle loop, though we rarely do it (I
think only iSeries does it nowadays).

So I see no need to -add- another callback here. I'm not entirely sure
what the cpuidle framework does, it's no obvious from Arun commit
messages I must say, but it doesn't look like the right approach for
integration. In fact, pSeries already have a choice between different
powersave models depending on what kind of hypervisor is there.

So Arun, please try to fit nicely within the existing interfaces, or if
you want to add a new one, please justify very precisely what design
decisions lead you to that.

Finally, there's also a problem with your first patch 1/2: You are
setting a Kconfig flag unconditionally indicating that the arch supports
some idle wait function, but you only implement it somewhere in
arch/powerpc/platform/pseries, so you'll break the build of any other
platform. You also prevent another platform to implement a different one
and be built in the same kernel.

For such generic callbacks, if it's justified (and only if it is), you
can add a ppc_md. hook for the platform to fill, and you need to cater
for platforms that don't.

Cheers,
Ben.