* [RFC PATCH v3 2/4] cpuidle: (POWER) cpuidle driver for pSeries
From: Deepthi Dharwar @ 2011-11-30 12:46 UTC (permalink / raw)
To: benh, len.brown; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111130124608.972.87712.stgit@deepthi-ThinkPad-T420>
This patch implements a back-end cpuidle driver for pSeries
based on pseries_dedicated_idle_loop and pseries_shared_idle_loop
routines. The driver is built only if CONFIG_CPU_IDLE is set. This
cpuidle driver uses global registration of idle states and
not per-cpu.
Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
---
arch/powerpc/include/asm/system.h | 8 +
arch/powerpc/kernel/sysfs.c | 2
arch/powerpc/platforms/pseries/Kconfig | 9 +
arch/powerpc/platforms/pseries/Makefile | 1
arch/powerpc/platforms/pseries/processor_idle.c | 326 +++++++++++++++++++++++
arch/powerpc/platforms/pseries/pseries.h | 3
arch/powerpc/platforms/pseries/setup.c | 3
arch/powerpc/platforms/pseries/smp.c | 1
8 files changed, 350 insertions(+), 3 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/processor_idle.c
diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
index ff66680..f56a0a7 100644
--- a/arch/powerpc/include/asm/system.h
+++ b/arch/powerpc/include/asm/system.h
@@ -223,6 +223,14 @@ extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
extern int powersave_nap; /* set if nap mode can be used in idle loop */
void cpu_idle_wait(void);
+#ifdef CONFIG_PSERIES_IDLE
+extern void update_smt_snooze_delay(int snooze);
+extern int pseries_notify_cpuidle_add_cpu(int cpu);
+#else
+static inline void update_smt_snooze_delay(int snooze) {}
+static inline int pseries_notify_cpuidle_add_cpu(int cpu) { return 0; }
+#endif
+
/*
* Atomic exchange
*
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index ce035c1..ebe5d78 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -18,6 +18,7 @@
#include <asm/machdep.h>
#include <asm/smp.h>
#include <asm/pmc.h>
+#include <asm/system.h>
#include "cacheinfo.h"
@@ -51,6 +52,7 @@ static ssize_t store_smt_snooze_delay(struct sys_device *dev,
return -EINVAL;
per_cpu(smt_snooze_delay, cpu->sysdev.id) = snooze;
+ update_smt_snooze_delay(snooze);
return count;
}
diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
index c81f6bb..ae7b6d4 100644
--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig
@@ -120,3 +120,12 @@ config DTL
which are accessible through a debugfs file.
Say N if you are unsure.
+
+config PSERIES_IDLE
+ tristate "Cpuidle driver for pSeries platforms"
+ depends on CPU_IDLE
+ depends on PPC_PSERIES
+ default y
+ help
+ Select this option to enable processor idle state management
+ through cpuidle subsystem.
diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile
index 3556e40..236db46 100644
--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PHYP_DUMP) += phyp_dump.o
obj-$(CONFIG_CMM) += cmm.o
obj-$(CONFIG_DTL) += dtl.o
obj-$(CONFIG_IO_EVENT_IRQ) += io_event_irq.o
+obj-$(CONFIG_PSERIES_IDLE) += processor_idle.o
ifeq ($(CONFIG_PPC_PSERIES),y)
obj-$(CONFIG_SUSPEND) += suspend.o
diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
new file mode 100644
index 0000000..352b78a
--- /dev/null
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -0,0 +1,326 @@
+/*
+ * processor_idle - idle state cpuidle driver.
+ * Adapted from drivers/idle/intel_idle.c and
+ * drivers/acpi/processor_idle.c
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
+#include <linux/cpuidle.h>
+#include <linux/cpu.h>
+
+#include <asm/paca.h>
+#include <asm/reg.h>
+#include <asm/system.h>
+#include <asm/machdep.h>
+#include <asm/firmware.h>
+
+#include "plpar_wrappers.h"
+#include "pseries.h"
+
+struct cpuidle_driver pseries_idle_driver = {
+ .name = "pseries_idle",
+ .owner = THIS_MODULE,
+};
+
+#define MAX_IDLE_STATE_COUNT 2
+
+static int max_idle_state = MAX_IDLE_STATE_COUNT - 1;
+static struct cpuidle_device __percpu *pseries_cpuidle_devices;
+static struct cpuidle_state *cpuidle_state_table;
+
+void update_smt_snooze_delay(int snooze)
+{
+ struct cpuidle_driver *drv = cpuidle_get_driver();
+ if (drv)
+ drv->states[0].target_residency = snooze;
+}
+
+static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
+{
+
+ *kt_before = ktime_get_real();
+ *in_purr = mfspr(SPRN_PURR);
+ /*
+ * Indicate to the HV that we are idle. Now would be
+ * a good time to find other work to dispatch.
+ */
+ get_lppaca()->idle = 1;
+}
+
+static inline s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
+{
+ get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
+ get_lppaca()->idle = 0;
+
+ return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
+}
+
+static int snooze_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ unsigned long in_purr;
+ ktime_t kt_before;
+ unsigned long start_snooze;
+ long snooze = drv->states[0].target_residency;
+
+ idle_loop_prolog(&in_purr, &kt_before);
+
+ if (snooze) {
+ start_snooze = get_tb() + snooze * tb_ticks_per_usec;
+ local_irq_enable();
+ set_thread_flag(TIF_POLLING_NRFLAG);
+
+ while ((snooze < 0) || (get_tb() < start_snooze)) {
+ if (need_resched() || cpu_is_offline(dev->cpu))
+ goto out;
+ ppc64_runlatch_off();
+ HMT_low();
+ HMT_very_low();
+ }
+
+ HMT_medium();
+ clear_thread_flag(TIF_POLLING_NRFLAG);
+ smp_mb();
+ local_irq_disable();
+ }
+
+out:
+ HMT_medium();
+ dev->last_residency =
+ (int)idle_loop_epilog(in_purr, kt_before);
+ return index;
+}
+
+static int dedicated_cede_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ unsigned long in_purr;
+ ktime_t kt_before;
+
+ idle_loop_prolog(&in_purr, &kt_before);
+ get_lppaca()->donate_dedicated_cpu = 1;
+
+ ppc64_runlatch_off();
+ HMT_medium();
+ cede_processor();
+
+ get_lppaca()->donate_dedicated_cpu = 0;
+ dev->last_residency =
+ (int)idle_loop_epilog(in_purr, kt_before);
+ return index;
+}
+
+static int shared_cede_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ unsigned long in_purr;
+ ktime_t kt_before;
+
+ idle_loop_prolog(&in_purr, &kt_before);
+
+ /*
+ * Yield the processor to the hypervisor. We return if
+ * an external interrupt occurs (which are driven prior
+ * to returning here) or if a prod occurs from another
+ * processor. When returning here, external interrupts
+ * are enabled.
+ */
+ cede_processor();
+
+ dev->last_residency =
+ (int)idle_loop_epilog(in_purr, kt_before);
+ return index;
+}
+
+/*
+ * States for dedicated partition case.
+ */
+static struct cpuidle_state dedicated_states[MAX_IDLE_STATE_COUNT] = {
+ { /* Snooze */
+ .name = "snooze",
+ .desc = "snooze",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = &snooze_loop },
+ { /* CEDE */
+ .name = "CEDE",
+ .desc = "CEDE",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 1,
+ .target_residency = 10,
+ .enter = &dedicated_cede_loop },
+};
+
+/*
+ * States for shared partition case.
+ */
+static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = {
+ { /* Shared Cede */
+ .name = "Shared Cede",
+ .desc = "Shared Cede",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = &shared_cede_loop },
+};
+
+int pseries_notify_cpuidle_add_cpu(int cpu)
+{
+ struct cpuidle_device *dev =
+ per_cpu_ptr(pseries_cpuidle_devices, cpu);
+ if (dev && cpuidle_get_driver()) {
+ cpuidle_disable_device(dev);
+ cpuidle_enable_device(dev);
+ }
+ return 0;
+}
+
+/*
+ * pseries_cpuidle_driver_init()
+ */
+static int pseries_cpuidle_driver_init(void)
+{
+ int idle_state;
+ struct cpuidle_driver *drv = &pseries_idle_driver;
+
+ drv->state_count = 0;
+
+ for (idle_state = 0; idle_state < MAX_IDLE_STATE_COUNT; ++idle_state) {
+
+ if (idle_state > max_idle_state)
+ break;
+
+ /* is the state not enabled? */
+ if (cpuidle_state_table[idle_state].enter == NULL)
+ continue;
+
+ drv->states[drv->state_count] = /* structure copy */
+ cpuidle_state_table[idle_state];
+
+ if (cpuidle_state_table == dedicated_states)
+ drv->states[drv->state_count].target_residency =
+ __get_cpu_var(smt_snooze_delay);
+
+ drv->state_count += 1;
+ }
+
+ return 0;
+}
+
+/* pseries_idle_devices_uninit(void)
+ * unregister cpuidle devices and de-allocate memory
+ */
+static void pseries_idle_devices_uninit(void)
+{
+ int i;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(i) {
+ dev = per_cpu_ptr(pseries_cpuidle_devices, i);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(pseries_cpuidle_devices);
+ return;
+}
+
+/* pseries_idle_devices_init()
+ * allocate, initialize and register cpuidle device
+ */
+static int pseries_idle_devices_init(void)
+{
+ int i;
+ struct cpuidle_driver *drv = &pseries_idle_driver;
+ struct cpuidle_device *dev;
+
+ pseries_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+ if (pseries_cpuidle_devices == NULL)
+ return -ENOMEM;
+
+ for_each_possible_cpu(i) {
+ dev = per_cpu_ptr(pseries_cpuidle_devices, i);
+ dev->state_count = drv->state_count;
+ dev->cpu = i;
+ if (cpuidle_register_device(dev)) {
+ printk(KERN_DEBUG \
+ "cpuidle_register_device %d failed!\n", i);
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * pseries_idle_probe()
+ * Choose state table for shared versus dedicated partition
+ */
+static int pseries_idle_probe(void)
+{
+
+ if (!firmware_has_feature(FW_FEATURE_SPLPAR))
+ return -ENODEV;
+
+ if (max_idle_state == 0) {
+ printk(KERN_DEBUG "pseries processor idle disabled.\n");
+ return -EPERM;
+ }
+
+ if (get_lppaca()->shared_proc)
+ cpuidle_state_table = shared_states;
+ else
+ cpuidle_state_table = dedicated_states;
+
+ return 0;
+}
+
+static int __init pseries_processor_idle_init(void)
+{
+ int retval;
+
+ retval = pseries_idle_probe();
+ if (retval)
+ return retval;
+
+ pseries_cpuidle_driver_init();
+ retval = cpuidle_register_driver(&pseries_idle_driver);
+ if (retval) {
+ printk(KERN_DEBUG "Registration of pseries driver failed.\n");
+ return retval;
+ }
+
+ retval = pseries_idle_devices_init();
+ if (retval) {
+ pseries_idle_devices_uninit();
+ cpuidle_unregister_driver(&pseries_idle_driver);
+ return retval;
+ }
+
+ printk(KERN_DEBUG "pseries_idle_driver registered\n");
+
+ return 0;
+}
+
+static void __exit pseries_processor_idle_exit(void)
+{
+
+ pseries_idle_devices_uninit();
+ cpuidle_unregister_driver(&pseries_idle_driver);
+
+ return;
+}
+
+module_init(pseries_processor_idle_init);
+module_exit(pseries_processor_idle_exit);
+
+MODULE_AUTHOR("Deepthi Dharwar <deepthi@linux.vnet.ibm.com>");
+MODULE_DESCRIPTION("Cpuidle driver for POWER");
+MODULE_LICENSE("GPL");
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 24c7162..9a3dda0 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -57,4 +57,7 @@ extern struct device_node *dlpar_configure_connector(u32);
extern int dlpar_attach_node(struct device_node *);
extern int dlpar_detach_node(struct device_node *);
+/* Snooze Delay, pseries_idle */
+DECLARE_PER_CPU(long, smt_snooze_delay);
+
#endif /* _PSERIES_PSERIES_H */
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index c3408ca..9c6716a 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -586,9 +586,6 @@ static int __init pSeries_probe(void)
return 1;
}
-
-DECLARE_PER_CPU(long, smt_snooze_delay);
-
static void pseries_dedicated_idle_sleep(void)
{
unsigned int cpu = smp_processor_id();
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index 26e93fd..bbc3c42 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -148,6 +148,7 @@ static void __devinit smp_xics_setup_cpu(int cpu)
set_cpu_current_state(cpu, CPU_STATE_ONLINE);
set_default_offline_state(cpu);
#endif
+ pseries_notify_cpuidle_add_cpu(cpu);
}
static int __devinit smp_pSeries_kick_cpu(int nr)
^ permalink raw reply related
* [RFC PATCH v3 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Deepthi Dharwar @ 2011-11-30 12:46 UTC (permalink / raw)
To: benh, len.brown; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111130124608.972.87712.stgit@deepthi-ThinkPad-T420>
This patch provides cpu_idle_wait() routine for the powerpc
platform which is required by the cpuidle subsystem. This
routine is required to change the idle handler on SMP systems.
The equivalent routine for x86 is in arch/x86/kernel/process.c
but the powerpc implementation is different.
cpuidle_disable variable is to enable/disable cpuidle
framework if power_save option is set during the boot
time.
Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
---
arch/powerpc/Kconfig | 4 ++++
arch/powerpc/include/asm/processor.h | 2 ++
arch/powerpc/include/asm/system.h | 1 +
arch/powerpc/kernel/idle.c | 27 +++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 951e18f..beeeec7 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -87,6 +87,10 @@ config ARCH_HAS_ILOG2_U64
bool
default y if 64BIT
+config ARCH_HAS_CPU_IDLE_WAIT
+ bool
+ default y
+
config GENERIC_HWEIGHT
bool
default y
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index eb11a44..811b7e7 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -382,6 +382,8 @@ static inline unsigned long get_clean_sp(struct pt_regs *regs, int is_32)
}
#endif
+enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
+
#endif /* __KERNEL__ */
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_PROCESSOR_H */
diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
index e30a13d..ff66680 100644
--- a/arch/powerpc/include/asm/system.h
+++ b/arch/powerpc/include/asm/system.h
@@ -221,6 +221,7 @@ extern unsigned long klimit;
extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
extern int powersave_nap; /* set if nap mode can be used in idle loop */
+void cpu_idle_wait(void);
/*
* Atomic exchange
diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
index 39a2baa..8574b0e 100644
--- a/arch/powerpc/kernel/idle.c
+++ b/arch/powerpc/kernel/idle.c
@@ -39,9 +39,13 @@
#define cpu_should_die() 0
#endif
+unsigned long cpuidle_disable = IDLE_NO_OVERRIDE;
+EXPORT_SYMBOL(cpuidle_disable);
+
static int __init powersave_off(char *arg)
{
ppc_md.power_save = NULL;
+ cpuidle_disable = IDLE_POWERSAVE_OFF;
return 0;
}
__setup("powersave=off", powersave_off);
@@ -102,6 +106,29 @@ void cpu_idle(void)
}
}
+
+/*
+ * cpu_idle_wait - Used to ensure that all the CPUs come out of the old
+ * idle loop and start using the new idle loop.
+ * Required while changing idle handler on SMP systems.
+ * Caller must have changed idle handler to the new value before the call.
+ * This window may be larger on shared systems.
+ */
+void cpu_idle_wait(void)
+{
+ int cpu;
+ smp_mb();
+
+ /* kick all the CPUs so that they exit out of old idle routine */
+ get_online_cpus();
+ for_each_online_cpu(cpu) {
+ if (cpu != smp_processor_id())
+ smp_send_reschedule(cpu);
+ }
+ put_online_cpus();
+}
+EXPORT_SYMBOL_GPL(cpu_idle_wait);
+
int powersave_nap;
#ifdef CONFIG_SYSCTL
^ permalink raw reply related
* [RFC PATCH v3 0/4] cpuidle: (POWER) cpuidle driver for pSeries
From: Deepthi Dharwar @ 2011-11-30 12:46 UTC (permalink / raw)
To: benh, len.brown; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
This patch series ports the cpuidle framework for ppc64 platform and
implements a cpuidle back-end driver for ppc64 (pSeries) platform.
Currently idle states are managed by pseries_{dedicated,shared}_idle_sleep()
routines in arch/powerpc/platforms/pseries/setup.c. There are
two idle states (snooze and cede) that are exploited by
these routines based on simple heuristics.
Moving the idle states over to cpuidle framework can take advantage of
the advanced heuristics, tunables, and features provided by cpuidle
framework. Additional idle states like extended cede with hints would be
included and exploited using the cpuidle framework. The statistics and
tracing infrastructure provided by the cpuidle framework also helps in
enabling power management related tools and help tune the system and
applications.
This series aims to maintain compatibility and functionality to
existing pSeries idle cpu management code. There are no new functions
or idle states added as part of this series.
The previous version of this patch can be found at
https://lkml.org/lkml/2011/11/17/127
Changes from the previous version (v2):
1] Rebased to latest 3.2-rc3
2] Incorporated the changes from the feedback provided by Ben
in the previous version of this series.
This patch series includes:
[1/4] - Provides arch specific cpu_idle_wait() function required by cpuidle
subsystem.
[2/4] - pseries_idle cpuidle driver
[3/4] - Enables cpuidle for pSeries and directly calls cpuidle_idle_call()
[4/4] - Handles powersave=off kernel boot parameter and disables registration
of pseries_idle cpuidle driver.
This series has been tested on ppc64 pSeries POWER7 system with the snooze
and cede states
--
arch/powerpc/Kconfig | 4
arch/powerpc/include/asm/processor.h | 3
arch/powerpc/include/asm/system.h | 9 +
arch/powerpc/kernel/idle.c | 27 ++
arch/powerpc/kernel/sysfs.c | 2
arch/powerpc/platforms/Kconfig | 6
arch/powerpc/platforms/pseries/Kconfig | 9 +
arch/powerpc/platforms/pseries/Makefile | 1
arch/powerpc/platforms/pseries/processor_idle.c | 329 +++++++++++++++++++++++
arch/powerpc/platforms/pseries/pseries.h | 3
arch/powerpc/platforms/pseries/setup.c | 104 +------
arch/powerpc/platforms/pseries/smp.c | 1
include/linux/cpuidle.h | 2
13 files changed, 411 insertions(+), 89 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/processor_idle.c
-Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Benjamin Herrenschmidt @ 2011-11-30 4:52 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <4ED58614.9090204@linux.vnet.ibm.com>
On Wed, 2011-11-30 at 06:55 +0530, Deepthi Dharwar wrote:
> I was trying to add a return value for power_save for all arch/powepc
> idle functions but a few of them directly call *.S routines, as they
> are asm.
>
> What would be a good way to change the return value for asm
> routines ?
> Do we make a change in asm only, put the return value in r3 or write a
> wrapper function which would call these asm routines and return an
> int ?
No, add li r3,0 at the end, but beware that their return point might not
be ovbvious since we often return from an interrupt which modifies the
return address ... Let me know if there's some you can't figure out and
I'll help you.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Deepthi Dharwar @ 2011-11-30 1:25 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linux-pm, linux-pm, linux-kernel, linuxppc-dev
In-Reply-To: <4ED47F48.2070107@linux.vnet.ibm.com>
On 11/29/2011 12:14 PM, Deepthi Dharwar wrote:
> On 11/29/2011 02:09 AM, Benjamin Herrenschmidt wrote:
>
>> On Mon, 2011-11-28 at 16:33 +0530, Deepthi Dharwar wrote:
>>
>>> On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
>>> cpuidle_idle_call by default here. This would result in calling of
>>> cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
>>> default idle is never executed.
>>> This would be a major design flaw. No fallback idle routine.
>>>
>>> We propose to fix this by checking the return value of
>>> ppc_md.power_save() call from void to int.
>>> Right now return value is void, but if we change this to int, this
>>> would solve two problems. One being removing the cast to a function
>>> pointer in the prev patch and this design flaw stated above.
kernel/idle.c: ppc_md.power_save = NULL;
>>>
>>> So by checking the return value of ppc_md.power_save(), we can invoke
>>> the default idle on failure. But my only concern is about the effects of
>>> changing the ppc_md.power_save() to return int on other powerpc
>>> architectures. Would it be a good idea to change the return type to int
>>> which would help us flag an error and fallback to default idle?
>>
>> I would have preferred an approach where the cpuidle module sets
>> ppc_md.power_save when loaded and restores it when unloaded ... but that
>> would have to go into the cpuidle core as a powerpc specific tweak and
>> might not be generally well received.
>>
>> So go for it, add the return value, but you'll have to update all the
>> idle functions (grep for power_save in arch/powerpc to find them).
>>
>
>
> Thanks Ben. Yes, I will update all the idle functions under powerpc.
> I will re-work these patches with the discussed changes.
>
> Regards,
> Deepthi
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>
>
Hi Ben,
I was trying to add a return value for power_save for all arch/powepc
idle functions but a few of them directly call *.S routines, as they
are asm.
What would be a good way to change the return value for asm routines ?
Do we make a change in asm only, put the return value in r3 or write a
wrapper function which would call these asm routines and return an int ?
Regards,
Deepthi
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Konrad Rzeszutek Wilk @ 2011-11-29 20:08 UTC (permalink / raw)
To: Borislav Petkov
Cc: Linux SCSI List, Florian Mickler, Network Development,
Linux PM List, x86, Linux Wireless List,
Linux Kernel Mailing List, DRI, Linux ACPI, mingo, hpa, tglx,
Kernel Testers List, Linus Torvalds, Andrew Morton,
Maciej Rutecki
In-Reply-To: <20111129183428.GE8200@aftab>
On Tue, Nov 29, 2011 at 07:34:28PM +0100, Borislav Petkov wrote:
> On Tue, Nov 29, 2011 at 01:04:14PM -0500, Konrad Rzeszutek Wilk wrote:
> > This patch:
> >
> > commit d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
> > Author: Len Brown <len.brown@intel.com>
> > Date: Fri Apr 1 18:28:35 2011 -0400
> >
> > cpuidle: replace xen access to x86 pm_idle and default_idle
> >
> > ..scribble on pm_idle and access default_idle,
> > have it simply disable_cpuidle() so acpi_idle will not load and
> > architecture default HLT will be used.
> >
> > idea was to have one call - disable_cpuidle() which would make
> > pm_idle not be molested by other code. It disallows cpuidle_idle_call
> > and acpi_idle_call to not set pm_idle (which is excellent). But the
>
> what is acpi_idle_call, I can't find it anywhere.
You are right. I had "acpi_idle_enter_*" and its friend in mind. Which
are called from the cpuidle_idle_call.
Let me fix that comment up.
>
> > amd_e400_idle and mwait_idle can still setup pm_idle which we really
> > do not want.
>
> This is not the case: rather select_idle_routine()/idle_setup() sets
> pm_idle.
Yes. Let me fix up the comment.
>
> [..]
>
> > +bool set_pm_idle_to_default()
> > +{
> > + if (!pm_idle) {
> > + pm_idle = default_idle;
> > + return true;
> > + }
> > + return false;
> > +}
>
> I don't understand what you're trying to achieve here? Do you want
> default_idle to be always the pm_idle for xen or what is the deal here?
Yes (always want default_idle).
>
> If yes, then simply do:
>
> bool set_pm_idle_to_default(void) // remember to add "void" for no function args
> {
> bool ret = !!pm_idle;
>
> pm_idle = default_idle;
That would work too.
>
> return ret;
>
> }
>
> ...
>
> > void stop_this_cpu(void *dummy)
> > {
> > local_irq_disable();
> > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> > index 46d6d21..7506181 100644
> > --- a/arch/x86/xen/setup.c
> > +++ b/arch/x86/xen/setup.c
> > @@ -448,6 +448,6 @@ void __init xen_arch_setup(void)
> > #endif
> > disable_cpuidle();
> > boot_option_idle_override = IDLE_HALT;
> > -
> > + WARN_ON(!set_pm_idle_to_default());
>
> and then do
>
> WARN_ON(set_pm_idle_to_default());
>
> instead of having arbitrary confusing logic. This way you can warn
> whether something else set pm_idle already. Or?
That would work as well.
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Konrad Rzeszutek Wilk @ 2011-11-29 18:04 UTC (permalink / raw)
To: Rafael J. Wysocki, tglx, mingo, hpa, x86, borislav.petkov
Cc: Linux SCSI List, Florian Mickler, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI, Linux ACPI,
Andrew Morton, Kernel Testers List, Linus Torvalds, Linux PM List,
Maciej Rutecki
In-Reply-To: <20111122135412.GA29905@phenom.dumpdata.com>
On Tue, Nov 22, 2011 at 08:54:12AM -0500, Konrad Rzeszutek Wilk wrote:
> > Subject : Regression in 3.1 causes Xen to use wrong idle routine
> > Submitter : Stefan Bader <stefan.bader@canonical.com>
> > Date : 2011-10-26 10:24
> > Message-ID : 4EA7DFD1.9060608@canonical.com
> > References : http://marc.info/?l=linux-acpi&m=131962467924564&w=2
>
> The patch mentioned in http://mid.gmane.org/20111115144004.GE22675@phenom.dumpdata.com
> should do it. But the patch needs an Ack from ACPI/x86 folks.
This patch (mentioned in the URL above) fixes the issue. Could it be
applied to the x86 tree for 3.2 or get an Ack, please?
>From 4f10ec7a7b9ff24657696aa98f25bcecde247373 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 21 Nov 2011 18:02:02 -0500
Subject: [PATCH] xen/pm_idle: Make pm_idle be default_idle under Xen.
This patch:
commit d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
Author: Len Brown <len.brown@intel.com>
Date: Fri Apr 1 18:28:35 2011 -0400
cpuidle: replace xen access to x86 pm_idle and default_idle
..scribble on pm_idle and access default_idle,
have it simply disable_cpuidle() so acpi_idle will not load and
architecture default HLT will be used.
idea was to have one call - disable_cpuidle() which would make
pm_idle not be molested by other code. It disallows cpuidle_idle_call
and acpi_idle_call to not set pm_idle (which is excellent). But the
amd_e400_idle and mwait_idle can still setup pm_idle which we really
do not want. In case of mwait_idle we can hit some instances where:
Brought up 2 CPUs
invalid opcode: 0000 [#1] SMP
CPU 1
Modules linked in:
Pid: 0, comm: swapper Not tainted 3.1.0-0.rc6.git0.3.fc16.x86_64 #1
RIP: e030:[<ffffffff81015d1d>] [<ffffffff81015d1d>] mwait_idle+0x6f/0xb4
RSP: e02b:ffff8801d28ddf10 EFLAGS: 00010082
RAX: ffff8801d28dc010 RBX: ffff8801d28ddfd8 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000001
RBP: ffff8801d28ddf10 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000000001 R11: ffff8801d28ddfd8 R12: ffffffff81b590d0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8801dff81000(0000) knlGS:0000000000000000
CS: e033 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000000 CR3: 0000000001a05000 CR4: 0000000000002660
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000000
Process swapper (pid: 0, threadinfo ffff8801d28dc000, task ffff8801d28cae60)
Stack:
ffff8801d28ddf40 ffffffff8100e2ed ffff8801dff8e390 c136dfe72feab515
0000000000000000 0000000000000000 ffff8801d28ddf50 ffffffff8149ee78
0000000000000000 0000000000000000 0000000000000000 0000000000000000
Call Trace:
[<ffffffff8100e2ed>] cpu_idle+0xae/0xe8
[<ffffffff8149ee78>] cpu_bringup_and_idle+0xe/0x10
RIP [<ffffffff81015d1d>] mwait_idle+0x6f/0xb4
RSP <ffff8801d28ddf10>
RH BZ #739499 and Ubuntu #881076
In case of amd_e400_idle we don't get so spectacular crashes, but
we do end up making an MSR which is trapped in the hypervisor,
and then follow it up with a yield hypercall. Meaning we end up
going to hypervisor twice instead of just once.
Lets make pm_idle be default_idle to take care of that.
Reported-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
arch/x86/include/asm/system.h | 1 +
arch/x86/kernel/process.c | 8 ++++++++
arch/x86/xen/setup.c | 2 +-
3 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index c2ff2a1..2d2f01c 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -401,6 +401,7 @@ extern unsigned long arch_align_stack(unsigned long sp);
extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
void default_idle(void);
+bool set_pm_idle_to_default(void);
void stop_this_cpu(void *dummy);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 1f7f8c8..336b299 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -404,6 +404,14 @@ void default_idle(void)
EXPORT_SYMBOL(default_idle);
#endif
+bool set_pm_idle_to_default()
+{
+ if (!pm_idle) {
+ pm_idle = default_idle;
+ return true;
+ }
+ return false;
+}
void stop_this_cpu(void *dummy)
{
local_irq_disable();
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 46d6d21..7506181 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -448,6 +448,6 @@ void __init xen_arch_setup(void)
#endif
disable_cpuidle();
boot_option_idle_override = IDLE_HALT;
-
+ WARN_ON(!set_pm_idle_to_default());
fiddle_vdso();
}
--
1.7.7.3
^ permalink raw reply related
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Deepthi Dharwar @ 2011-11-29 7:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322550115.23348.72.camel@pasglop>
On 11/29/2011 12:31 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2011-11-29 at 12:12 +0530, Deepthi Dharwar wrote:
>>
>> Yes, this could be problematic as there is small window for the
>> race condition to occur . Otherwise we need to manually schedule
>> it by running a kernel thread but this would definitely have a
>> overhead and would be an overkill.
>
> Depends what this "window" is. IE. What are you trying to protect
> yourself against ? What's the risk ?
>
> If it's just module unload, then stop_machine is probably your
> friend :-)
>
> Cheers,
> Ben.
>
>
Yup, it is the module unload that I am worried about. Otherwise
manually doing it using kernel thread would be an overkill -:(
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Benjamin Herrenschmidt @ 2011-11-29 7:01 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <4ED47EC2.2090802@linux.vnet.ibm.com>
On Tue, 2011-11-29 at 12:12 +0530, Deepthi Dharwar wrote:
>
> Yes, this could be problematic as there is small window for the
> race condition to occur . Otherwise we need to manually schedule
> it by running a kernel thread but this would definitely have a
> overhead and would be an overkill.
Depends what this "window" is. IE. What are you trying to protect
yourself against ? What's the risk ?
If it's just module unload, then stop_machine is probably your
friend :-)
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Deepthi Dharwar @ 2011-11-29 6:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322512771.23348.45.camel@pasglop>
On 11/29/2011 02:09 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2011-11-28 at 16:33 +0530, Deepthi Dharwar wrote:
>
>> On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
>> cpuidle_idle_call by default here. This would result in calling of
>> cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
>> default idle is never executed.
>> This would be a major design flaw. No fallback idle routine.
>>
>> We propose to fix this by checking the return value of
>> ppc_md.power_save() call from void to int.
>> Right now return value is void, but if we change this to int, this
>> would solve two problems. One being removing the cast to a function
>> pointer in the prev patch and this design flaw stated above.
>>
>> So by checking the return value of ppc_md.power_save(), we can invoke
>> the default idle on failure. But my only concern is about the effects of
>> changing the ppc_md.power_save() to return int on other powerpc
>> architectures. Would it be a good idea to change the return type to int
>> which would help us flag an error and fallback to default idle?
>
> I would have preferred an approach where the cpuidle module sets
> ppc_md.power_save when loaded and restores it when unloaded ... but that
> would have to go into the cpuidle core as a powerpc specific tweak and
> might not be generally well received.
>
> So go for it, add the return value, but you'll have to update all the
> idle functions (grep for power_save in arch/powerpc to find them).
>
Thanks Ben. Yes, I will update all the idle functions under powerpc.
I will re-work these patches with the discussed changes.
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Deepthi Dharwar @ 2011-11-29 6:42 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322512522.23348.43.camel@pasglop>
On 11/29/2011 02:05 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2011-11-28 at 16:32 +0530, Deepthi Dharwar wrote:
>
>>> Additionally, I'm a bit worried (but maybe we already discussed that a
>>> while back, I don't know) but cpu_idle_wait() has "wait" in the name,
>>> which makes me think it might need to actually -wait- for all cpus to
>>> have come out of the function.
>>
>> cpu_idle_wait is used to ensure that all the CPUs discard old idle
>> handler and update to new one. Required while changing idle
>> handler on SMP systems.
>>
>>> Now your implementation doesn't provide that guarantee. It might be
>>> fine, I don't know, but if it is, you'd better document it well in the
>>> comments surrounding the code, because as it is, all you do is shoot an
>>> interrupt which will cause the target CPU to eventually come out of idle
>>> some time in the future.
>>
>>
>> I was hoping that sending an explicit reschedule to the cpus would
>> do the trick but sure we can add some documentation around the code.
>
> Well, the question is what guarantee do you expect. Sending a reschedule
> IPI will take the other CPUs out of the actual sleep mode, but it will
> be some time from there back to getting out of the handler function
> (first back out of hypervisor etc...).
>
> The code as you implemented it doesn't wait for that to happen. It might
> be fine ... or not. I don't know what semantics you are after precisely.
>
> Cheers,
> Ben.
>
>
Yes, this could be problematic as there is small window for the
race condition to occur . Otherwise we need to manually schedule
it by running a kernel thread but this would definitely have a
overhead and would be an overkill.
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Benjamin Herrenschmidt @ 2011-11-28 20:39 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <4ED36A7D.9070308@linux.vnet.ibm.com>
On Mon, 2011-11-28 at 16:33 +0530, Deepthi Dharwar wrote:
> On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
> cpuidle_idle_call by default here. This would result in calling of
> cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
> default idle is never executed.
> This would be a major design flaw. No fallback idle routine.
>
> We propose to fix this by checking the return value of
> ppc_md.power_save() call from void to int.
> Right now return value is void, but if we change this to int, this
> would solve two problems. One being removing the cast to a function
> pointer in the prev patch and this design flaw stated above.
>
> So by checking the return value of ppc_md.power_save(), we can invoke
> the default idle on failure. But my only concern is about the effects of
> changing the ppc_md.power_save() to return int on other powerpc
> architectures. Would it be a good idea to change the return type to int
> which would help us flag an error and fallback to default idle?
I would have preferred an approach where the cpuidle module sets
ppc_md.power_save when loaded and restores it when unloaded ... but that
would have to go into the cpuidle core as a powerpc specific tweak and
might not be generally well received.
So go for it, add the return value, but you'll have to update all the
idle functions (grep for power_save in arch/powerpc to find them).
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Benjamin Herrenschmidt @ 2011-11-28 20:35 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <4ED36A37.3030409@linux.vnet.ibm.com>
On Mon, 2011-11-28 at 16:32 +0530, Deepthi Dharwar wrote:
> > Additionally, I'm a bit worried (but maybe we already discussed that a
> > while back, I don't know) but cpu_idle_wait() has "wait" in the name,
> > which makes me think it might need to actually -wait- for all cpus to
> > have come out of the function.
>
> cpu_idle_wait is used to ensure that all the CPUs discard old idle
> handler and update to new one. Required while changing idle
> handler on SMP systems.
>
> > Now your implementation doesn't provide that guarantee. It might be
> > fine, I don't know, but if it is, you'd better document it well in the
> > comments surrounding the code, because as it is, all you do is shoot an
> > interrupt which will cause the target CPU to eventually come out of idle
> > some time in the future.
>
>
> I was hoping that sending an explicit reschedule to the cpus would
> do the trick but sure we can add some documentation around the code.
Well, the question is what guarantee do you expect. Sending a reschedule
IPI will take the other CPUs out of the actual sleep mode, but it will
be some time from there back to getting out of the handler function
(first back out of hypervisor etc...).
The code as you implemented it doesn't wait for that to happen. It might
be fine ... or not. I don't know what semantics you are after precisely.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Deepthi Dharwar @ 2011-11-28 11:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322435233.23348.19.camel@pasglop>
On 11/28/2011 04:37 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2011-11-17 at 16:59 +0530, Deepthi Dharwar wrote:
>> This patch makes pseries_idle_driver not to be registered when
>> power_save=off kernel boot option is specified. The
>> boot_option_idle_override variable used here is similar to
>> its usage on x86.
>
> Quick Q. With your changes, the CPU will never get into idle at all
> until cpuidle initializes and the driver loads.
>
> That means not only much later in the boot process, but potentially
> never if the distro has the driver as a module and fails to load it, or
> similar.
>
> Can't that be an issue ? Shouldn't we keep at least one of the basic
> idle functions as a fallback ?
>
On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
cpuidle_idle_call by default here. This would result in calling of
cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
default idle is never executed.
This would be a major design flaw. No fallback idle routine.
We propose to fix this by checking the return value of
ppc_md.power_save() call from void to int.
Right now return value is void, but if we change this to int, this
would solve two problems. One being removing the cast to a function
pointer in the prev patch and this design flaw stated above.
So by checking the return value of ppc_md.power_save(), we can invoke
the default idle on failure. But my only concern is about the effects of
changing the ppc_md.power_save() to return int on other powerpc
architectures. Would it be a good idea to change the return type to int
which would help us flag an error and fallback to default idle?
> Cheers,
> Ben.
>
>
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 3/4] cpuidle: (POWER) Enable cpuidle and directly call cpuidle_idle_call() for pSeries
From: Deepthi Dharwar @ 2011-11-28 11:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322435131.23348.17.camel@pasglop>
On 11/28/2011 04:35 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
>> This patch enables cpuidle for pSeries and cpuidle_idle_call() is
>> directly called from the idle loop. As a result pseries_idle cpuidle
>> driver registered with cpuidle subsystem comes into action. This patch
>> also removes the routines pseries_shared_idle_sleep and
>> pseries_dedicated_idle_sleep as they are now implemented as part of
>> pseries_idle cpuidle driver.
>>
>> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>> Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
>> Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
>> ---
>> arch/powerpc/platforms/Kconfig | 6 ++
>> arch/powerpc/platforms/pseries/setup.c | 86 +-------------------------------
>> include/linux/cpuidle.h | 2 -
>> 3 files changed, 8 insertions(+), 86 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
>> index e458872..0d2a028 100644
>> --- a/arch/powerpc/platforms/Kconfig
>> +++ b/arch/powerpc/platforms/Kconfig
>> @@ -211,6 +211,12 @@ config PPC_PASEMI_CPUFREQ
>>
>> endmenu
>>
>> +menu "CPUIdle driver"
>> +
>> +source "drivers/cpuidle/Kconfig"
>> +
>> +endmenu
>> +
>> config PPC601_SYNC_FIX
>> bool "Workarounds for PPC601 bugs"
>> depends on 6xx && (PPC_PREP || PPC_PMAC)
>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>> index 9c6716a..f624e74 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -39,6 +39,7 @@
>> #include <linux/irq.h>
>> #include <linux/seq_file.h>
>> #include <linux/root_dev.h>
>> +#include <linux/cpuidle.h>
>>
>> #include <asm/mmu.h>
>> #include <asm/processor.h>
>> @@ -74,9 +75,6 @@ EXPORT_SYMBOL(CMO_PageSize);
>>
>> int fwnmi_active; /* TRUE if an FWNMI handler is present */
>>
>> -static void pseries_shared_idle_sleep(void);
>> -static void pseries_dedicated_idle_sleep(void);
>> -
>> static struct device_node *pSeries_mpic_node;
>>
>> static void pSeries_show_cpuinfo(struct seq_file *m)
>> @@ -374,18 +372,9 @@ static void __init pSeries_setup_arch(void)
>>
>> pSeries_nvram_init();
>>
>> - /* Choose an idle loop */
>> if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
>> vpa_init(boot_cpuid);
>> - if (get_lppaca()->shared_proc) {
>> - printk(KERN_DEBUG "Using shared processor idle loop\n");
>> - ppc_md.power_save = pseries_shared_idle_sleep;
>> - } else {
>> - printk(KERN_DEBUG "Using dedicated idle loop\n");
>> - ppc_md.power_save = pseries_dedicated_idle_sleep;
>> - }
>> - } else {
>> - printk(KERN_DEBUG "Using default idle loop\n");
>> + ppc_md.power_save = (void *)cpuidle_idle_call;
>> }
>
> I very very much dislike that cast. You should not have to cast a
> function pointer ... EVER.
Yes, I ll fix this.
This actually bought out a design flaw with the current pseries idle as
mentioned by you in the next patch of the series.
>
>> if (firmware_has_feature(FW_FEATURE_LPAR))
>> @@ -586,77 +575,6 @@ static int __init pSeries_probe(void)
>> return 1;
>> }
>>
>
> Cheers,
> Ben.
>
>
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 2/4] cpuidle: (POWER) cpuidle driver for pSeries
From: Deepthi Dharwar @ 2011-11-28 11:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322435008.23348.15.camel@pasglop>
On 11/28/2011 04:33 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
>> This patch implements a backhand cpuidle driver for pSeries
>> based on pseries_dedicated_idle_loop and pseries_shared_idle_loop
>> routines. The driver is built only if CONFIG_CPU_IDLE is set. This
>> cpuidle driver uses global registration of idle states and
>> not per-cpu.
>
> .../...
>
>> +#define MAX_IDLE_STATE_COUNT 2
>> +
>> +static int max_cstate = MAX_IDLE_STATE_COUNT - 1;
>
> Why "cstate" ? This isn't an x86 :-)
Sure, I shall change it right away :)
>
>> +static struct cpuidle_device __percpu *pseries_idle_cpuidle_devices;
>
> Shorter name please. pseries_cpuidle_devs is fine.
I ll do so.
>
>> +static struct cpuidle_state *cpuidle_state_table;
>> +
>> +void update_smt_snooze_delay(int snooze)
>> +{
>> + struct cpuidle_driver *drv = cpuidle_get_driver();
>> + if (drv)
>> + drv->states[0].target_residency = snooze;
>> +}
>> +
>> +static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>> +{
>> +
>> + *kt_before = ktime_get_real();
>> + *in_purr = mfspr(SPRN_PURR);
>> + /*
>> + * Indicate to the HV that we are idle. Now would be
>> + * a good time to find other work to dispatch.
>> + */
>> + get_lppaca()->idle = 1;
>> + get_lppaca()->donate_dedicated_cpu = 1;
>> +}
>
> I notice that you call this on shared processors as well. The old ocde
> used to not set donate_dedicated_cpu in that case. I assume that's not a
> big deal and that the HV will just ignore it in the shared processor
> case but please add a comment after you've verified it.
>
Yes, the old code does not set donate_dedicated_cpu. But yes I will
try testing it in a shared proc config but also remove this
initialization for shared idle loop.
>> +static inline s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>> +{
>> + get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>> + get_lppaca()->donate_dedicated_cpu = 0;
>> + get_lppaca()->idle = 0;
>> +
>> + return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
>> +}
>> +
>> +
>> +static int snooze_loop(struct cpuidle_device *dev,
>> + struct cpuidle_driver *drv,
>> + int index)
>> +{
>> + unsigned long in_purr;
>> + ktime_t kt_before;
>> +
>> + idle_loop_prolog(&in_purr, &kt_before);
>> +
>> + 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();
>> +
>> + dev->last_residency =
>> + (int)idle_loop_epilog(in_purr, kt_before);
>> + return index;
>> +}
>
> So your snooze loop has no timeout, is that handled by the cpuidle
> driver using some kind of timer ? That sounds a lot less efficient than
> passing a max delay to the snooze loop to handle getting into a deeper
> state after a bit of snoozing rather than interrupting etc...
My bad, snooze_loop is essential for a time out. Nope cpuidle
driver doesn't have any timer mechanism. I ll fix it.
Need to add loop for snooze time.
>> +static int dedicated_cede_loop(struct cpuidle_device *dev,
>> + struct cpuidle_driver *drv,
>> + int index)
>> +{
>> + unsigned long in_purr;
>> + ktime_t kt_before;
>> +
>> + idle_loop_prolog(&in_purr, &kt_before);
>> +
>> + ppc64_runlatch_off();
>> + HMT_medium();
>> + cede_processor();
>> +
>> + dev->last_residency =
>> + (int)idle_loop_epilog(in_purr, kt_before);
>> + return index;
>> +}
>> +
>> +static int shared_cede_loop(struct cpuidle_device *dev,
>> + struct cpuidle_driver *drv,
>> + int index)
>> +{
>> + unsigned long in_purr;
>> + ktime_t kt_before;
>> +
>> + idle_loop_prolog(&in_purr, &kt_before);
>> +
>> + /*
>> + * Yield the processor to the hypervisor. We return if
>> + * an external interrupt occurs (which are driven prior
>> + * to returning here) or if a prod occurs from another
>> + * processor. When returning here, external interrupts
>> + * are enabled.
>> + */
>> + cede_processor();
>> +
>> + dev->last_residency =
>> + (int)idle_loop_epilog(in_purr, kt_before);
>> + return index;
>> +}
>> +
>> +/*
>> + * States for dedicated partition case.
>> + */
>> +static struct cpuidle_state dedicated_states[MAX_IDLE_STATE_COUNT] = {
>> + { /* Snooze */
>> + .name = "snooze",
>> + .desc = "snooze",
>> + .flags = CPUIDLE_FLAG_TIME_VALID,
>> + .exit_latency = 0,
>> + .target_residency = 0,
>> + .enter = &snooze_loop },
>> + { /* CEDE */
>> + .name = "CEDE",
>> + .desc = "CEDE",
>> + .flags = CPUIDLE_FLAG_TIME_VALID,
>> + .exit_latency = 1,
>> + .target_residency = 10,
>> + .enter = &dedicated_cede_loop },
>> +};
>> +
>> +/*
>> + * States for shared partition case.
>> + */
>> +static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = {
>> + { /* Shared Cede */
>> + .name = "Shared Cede",
>> + .desc = "Shared Cede",
>> + .flags = CPUIDLE_FLAG_TIME_VALID,
>> + .exit_latency = 0,
>> + .target_residency = 0,
>> + .enter = &shared_cede_loop },
>> +};
>> +
>> +int pseries_notify_cpuidle_add_cpu(int cpu)
>> +{
>> + struct cpuidle_device *dev =
>> + per_cpu_ptr(pseries_idle_cpuidle_devices, cpu);
>> + if (dev && cpuidle_get_driver()) {
>> + cpuidle_disable_device(dev);
>> + cpuidle_enable_device(dev);
>> + }
>> + return 0;
>> +}
>> +
>> +/*
>> + * pseries_idle_cpuidle_driver_init()
>> + */
>> +static int pseries_idle_cpuidle_driver_init(void)
>> +{
>
> Drop the "idle", those names are too long.
Sure.
>
>> + int cstate;
>
> And use something else than 'cstate', we are among civilized people
> here ;-)
Yes :)
>
>> + struct cpuidle_driver *drv = &pseries_idle_driver;
>> +
>> + drv->state_count = 0;
>> +
>> + for (cstate = 0; cstate < MAX_IDLE_STATE_COUNT; ++cstate) {
>> +
>> + if (cstate > max_cstate)
>> + break;
>> +
>> + /* is the state not enabled? */
>> + if (cpuidle_state_table[cstate].enter == NULL)
>> + continue;
>> +
>> + drv->states[drv->state_count] = /* structure copy */
>> + cpuidle_state_table[cstate];
>> +
>> + if (cpuidle_state_table == dedicated_states)
>> + drv->states[drv->state_count].target_residency =
>> + __get_cpu_var(smt_snooze_delay);
>> +
>> + drv->state_count += 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* pseries_idle_devices_uninit(void)
>> + * unregister cpuidle devices and de-allocate memory
>> + */
>> +static void pseries_idle_devices_uninit(void)
>> +{
>> + int i;
>> + struct cpuidle_device *dev;
>> +
>> + for_each_possible_cpu(i) {
>> + dev = per_cpu_ptr(pseries_idle_cpuidle_devices, i);
>> + cpuidle_unregister_device(dev);
>> + }
>> +
>> + free_percpu(pseries_idle_cpuidle_devices);
>> + return;
>> +}
>> +
>> +/* pseries_idle_devices_init()
>> + * allocate, initialize and register cpuidle device
>> + */
>> +static int pseries_idle_devices_init(void)
>> +{
>> + int i;
>> + struct cpuidle_driver *drv = &pseries_idle_driver;
>> + struct cpuidle_device *dev;
>> +
>> + pseries_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + if (pseries_idle_cpuidle_devices == NULL)
>> + return -ENOMEM;
>> +
>> + for_each_possible_cpu(i) {
>> + dev = per_cpu_ptr(pseries_idle_cpuidle_devices, i);
>> + dev->state_count = drv->state_count;
>> + dev->cpu = i;
>> + if (cpuidle_register_device(dev)) {
>> + printk(KERN_DEBUG \
>> + "cpuidle_register_device %d failed!\n", i);
>> + return -EIO;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * pseries_idle_probe()
>> + * Choose state table for shared versus dedicated partition
>> + */
>> +static int pseries_idle_probe(void)
>> +{
>> + if (max_cstate == 0) {
>> + printk(KERN_DEBUG "pseries processor idle disabled.\n");
>> + return -EPERM;
>> + }
>> +
>> + if (!firmware_has_feature(FW_FEATURE_SPLPAR)) {
>> + printk(KERN_DEBUG "Using default idle\n");
>> + return -ENODEV;
>> + }
>
> Don't even printk here, this will happen on all !pseries machines and
> the debug output isn't useful. Also move the check of max-cstate to
> after the splpar test.
I ll move the check above.
> Overall, I quite like these patches, my comments are all pretty minor,
> hopefully the next round should be the one.
Thank you.
>
> Cheers,
> Ben.
>
>
Regards,
Deepthi
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Deepthi Dharwar @ 2011-11-28 11:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <1322434096.23348.6.camel@pasglop>
Hi Ben,
Thanks a lot for the review.
On 11/28/2011 04:18 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
>> This patch provides cpu_idle_wait() routine for the powerpc
>> platform which is required by the cpuidle subsystem. This
>> routine is requied to change the idle handler on SMP systems.
>> The equivalent routine for x86 is in arch/x86/kernel/process.c
>> but the powerpc implementation is different.
>>
>> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>> Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
>> Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
>> ---
>
> No, that patch also adds this idle boot override thing (can you pick a
> shorter name for boot_option_idle_override btw ?) which seems unrelated
> and without any explanation as to what it's supposed to be about.
Yes, we can pick a better and shorter name for this variable.
This variable is used to determine if cpuidle framework
needs to be enabled and pseries_driver to be loaded or not.
We disable cpuidle framework only when powersave_off option is set or
not enabled by the user.
>
> Additionally, I'm a bit worried (but maybe we already discussed that a
> while back, I don't know) but cpu_idle_wait() has "wait" in the name,
> which makes me think it might need to actually -wait- for all cpus to
> have come out of the function.
cpu_idle_wait is used to ensure that all the CPUs discard old idle
handler and update to new one. Required while changing idle
handler on SMP systems.
> Now your implementation doesn't provide that guarantee. It might be
> fine, I don't know, but if it is, you'd better document it well in the
> comments surrounding the code, because as it is, all you do is shoot an
> interrupt which will cause the target CPU to eventually come out of idle
> some time in the future.
I was hoping that sending an explicit reschedule to the cpus would
do the trick but sure we can add some documentation around the code.
>
> Cheers,
> Ben.
>
>> arch/powerpc/Kconfig | 4 ++++
>> arch/powerpc/include/asm/processor.h | 2 ++
>> arch/powerpc/include/asm/system.h | 1 +
>> arch/powerpc/kernel/idle.c | 26 ++++++++++++++++++++++++++
>> 4 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index b177caa..87f8604 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -87,6 +87,10 @@ config ARCH_HAS_ILOG2_U64
>> bool
>> default y if 64BIT
>>
>> +config ARCH_HAS_CPU_IDLE_WAIT
>> + bool
>> + default y
>> +
>> config GENERIC_HWEIGHT
>> bool
>> default y
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index eb11a44..811b7e7 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -382,6 +382,8 @@ static inline unsigned long get_clean_sp(struct pt_regs *regs, int is_32)
>> }
>> #endif
>>
>> +enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
>> +
>> #endif /* __KERNEL__ */
>> #endif /* __ASSEMBLY__ */
>> #endif /* _ASM_POWERPC_PROCESSOR_H */
>> diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
>> index e30a13d..ff66680 100644
>> --- a/arch/powerpc/include/asm/system.h
>> +++ b/arch/powerpc/include/asm/system.h
>> @@ -221,6 +221,7 @@ extern unsigned long klimit;
>> extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
>>
>> extern int powersave_nap; /* set if nap mode can be used in idle loop */
>> +void cpu_idle_wait(void);
>>
>> /*
>> * Atomic exchange
>> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
>> index 39a2baa..b478c72 100644
>> --- a/arch/powerpc/kernel/idle.c
>> +++ b/arch/powerpc/kernel/idle.c
>> @@ -39,9 +39,13 @@
>> #define cpu_should_die() 0
>> #endif
>>
>> +unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE;
>> +EXPORT_SYMBOL(boot_option_idle_override);
>> +
>> static int __init powersave_off(char *arg)
>> {
>> ppc_md.power_save = NULL;
>> + boot_option_idle_override = IDLE_POWERSAVE_OFF;
>> return 0;
>> }
>> __setup("powersave=off", powersave_off);
>> @@ -102,6 +106,28 @@ void cpu_idle(void)
>> }
>> }
>>
>> +
>> +/*
>> + * cpu_idle_wait - Used to ensure that all the CPUs come out of the old
>> + * idle loop and start using the new idle loop.
>> + * Required while changing idle handler on SMP systems.
>> + * Caller must have changed idle handler to the new value before the call.
>> + */
>> +void cpu_idle_wait(void)
>> +{
>> + int cpu;
>> + smp_mb();
>> +
>> + /* kick all the CPUs so that they exit out of old idle routine */
>> + get_online_cpus();
>> + for_each_online_cpu(cpu) {
>> + if (cpu != smp_processor_id())
>> + smp_send_reschedule(cpu);
>> + }
>> + put_online_cpus();
>> +}
>> +EXPORT_SYMBOL_GPL(cpu_idle_wait);
>> +
>> int powersave_nap;
>>
>> #ifdef CONFIG_SYSCTL
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
Regards,
Deepthi
^ permalink raw reply
* [PATCH] hugetlb: release pages in the error path of hugetlb_cow() (was: Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1)
From: Michal Hocko @ 2011-11-28 8:33 UTC (permalink / raw)
To: stable
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Andy Lutomirski, Florian Mickler, Andrew Morton,
Kernel Testers List, Linus Torvalds, Linux PM List,
Maciej Rutecki
In-Reply-To: <CA+55aFygSFt+O5KLoiE_0V+o45eKfsoDDV5ML8EF=J0n9z_D-Q@mail.gmail.com>
On Mon 21-11-11 14:18:29, Linus Torvalds wrote:
> On Mon, Nov 21, 2011 at 1:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > Subject : hugetlb oops on 3.1.0-rc8-devel
> > Submitter : Andy Lutomirski <luto@amacapital.net>
> > Date : 2011-11-01 22:20
> > Message-ID : CALCETrW1mpVCz2tO5roaz1r6vnno+srHR-dHA6_pkRi2qiCfdw@mail.gmail.com
> > References : http://marc.info/?l=linux-kernel&m=132018604426692&w=2
>
> Despite the subject line, that's not an oops, it's a BUG_ON().
>
> And it *should* be fixed by commit ea4039a34c4c ("hugetlb: release
> pages in the error path of hugetlb_cow()") although I don't think Andy
> ever confirmed that (since it was hard to trigger).
AFAICS the issue has been introduced by 0fe6e20b (hugetlb, rmap:
add reverse mapping for hugepage) in 2.6.36-rc1 so this is a stable
material. I do not see the patch in any stable branch so here we go.
The patch is on top of 3.0.y branch and it applies as is to 3.1.y
as well.
---
>From fdaa4aaa008cce149a5fd60934112acd8988e0b6 Mon Sep 17 00:00:00 2001
From: Hillf Danton <dhillf@gmail.com>
Date: Tue, 15 Nov 2011 14:36:12 -0800
Subject: [PATCH] hugetlb: release pages in the error path of hugetlb_cow()
commit ea4039a34c4c206d015d34a49d0b00868e37db1d upstream.
If we fail to prepare an anon_vma, the {new, old}_page should be released,
or they will leak.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <jweiner@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
mm/hugetlb.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bfcf153..2b57cd9 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2415,6 +2415,8 @@ retry_avoidcopy:
* anon_vma prepared.
*/
if (unlikely(anon_vma_prepare(vma))) {
+ page_cache_release(new_page);
+ page_cache_release(old_page);
/* Caller expects lock to be held */
spin_lock(&mm->page_table_lock);
return VM_FAULT_OOM;
--
1.7.7.3
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
^ permalink raw reply related
* Re: [RFC PATCH v2 4/4] cpuidle: (POWER) Handle power_save=off
From: Benjamin Herrenschmidt @ 2011-11-27 23:07 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111117112906.9191.54050.stgit@localhost6.localdomain6>
On Thu, 2011-11-17 at 16:59 +0530, Deepthi Dharwar wrote:
> This patch makes pseries_idle_driver not to be registered when
> power_save=off kernel boot option is specified. The
> boot_option_idle_override variable used here is similar to
> its usage on x86.
Quick Q. With your changes, the CPU will never get into idle at all
until cpuidle initializes and the driver loads.
That means not only much later in the boot process, but potentially
never if the distro has the driver as a module and fails to load it, or
similar.
Can't that be an issue ? Shouldn't we keep at least one of the basic
idle functions as a fallback ?
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 3/4] cpuidle: (POWER) Enable cpuidle and directly call cpuidle_idle_call() for pSeries
From: Benjamin Herrenschmidt @ 2011-11-27 23:05 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111117112852.9191.34256.stgit@localhost6.localdomain6>
On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
> This patch enables cpuidle for pSeries and cpuidle_idle_call() is
> directly called from the idle loop. As a result pseries_idle cpuidle
> driver registered with cpuidle subsystem comes into action. This patch
> also removes the routines pseries_shared_idle_sleep and
> pseries_dedicated_idle_sleep as they are now implemented as part of
> pseries_idle cpuidle driver.
>
> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
> Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
> ---
> arch/powerpc/platforms/Kconfig | 6 ++
> arch/powerpc/platforms/pseries/setup.c | 86 +-------------------------------
> include/linux/cpuidle.h | 2 -
> 3 files changed, 8 insertions(+), 86 deletions(-)
>
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index e458872..0d2a028 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -211,6 +211,12 @@ config PPC_PASEMI_CPUFREQ
>
> endmenu
>
> +menu "CPUIdle driver"
> +
> +source "drivers/cpuidle/Kconfig"
> +
> +endmenu
> +
> config PPC601_SYNC_FIX
> bool "Workarounds for PPC601 bugs"
> depends on 6xx && (PPC_PREP || PPC_PMAC)
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 9c6716a..f624e74 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -39,6 +39,7 @@
> #include <linux/irq.h>
> #include <linux/seq_file.h>
> #include <linux/root_dev.h>
> +#include <linux/cpuidle.h>
>
> #include <asm/mmu.h>
> #include <asm/processor.h>
> @@ -74,9 +75,6 @@ EXPORT_SYMBOL(CMO_PageSize);
>
> int fwnmi_active; /* TRUE if an FWNMI handler is present */
>
> -static void pseries_shared_idle_sleep(void);
> -static void pseries_dedicated_idle_sleep(void);
> -
> static struct device_node *pSeries_mpic_node;
>
> static void pSeries_show_cpuinfo(struct seq_file *m)
> @@ -374,18 +372,9 @@ static void __init pSeries_setup_arch(void)
>
> pSeries_nvram_init();
>
> - /* Choose an idle loop */
> if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
> vpa_init(boot_cpuid);
> - if (get_lppaca()->shared_proc) {
> - printk(KERN_DEBUG "Using shared processor idle loop\n");
> - ppc_md.power_save = pseries_shared_idle_sleep;
> - } else {
> - printk(KERN_DEBUG "Using dedicated idle loop\n");
> - ppc_md.power_save = pseries_dedicated_idle_sleep;
> - }
> - } else {
> - printk(KERN_DEBUG "Using default idle loop\n");
> + ppc_md.power_save = (void *)cpuidle_idle_call;
> }
I very very much dislike that cast. You should not have to cast a
function pointer ... EVER.
> if (firmware_has_feature(FW_FEATURE_LPAR))
> @@ -586,77 +575,6 @@ static int __init pSeries_probe(void)
> return 1;
> }
>
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 2/4] cpuidle: (POWER) cpuidle driver for pSeries
From: Benjamin Herrenschmidt @ 2011-11-27 23:03 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111117112841.9191.97974.stgit@localhost6.localdomain6>
On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
> This patch implements a backhand cpuidle driver for pSeries
> based on pseries_dedicated_idle_loop and pseries_shared_idle_loop
> routines. The driver is built only if CONFIG_CPU_IDLE is set. This
> cpuidle driver uses global registration of idle states and
> not per-cpu.
.../...
> +#define MAX_IDLE_STATE_COUNT 2
> +
> +static int max_cstate = MAX_IDLE_STATE_COUNT - 1;
Why "cstate" ? This isn't an x86 :-)
> +static struct cpuidle_device __percpu *pseries_idle_cpuidle_devices;
Shorter name please. pseries_cpuidle_devs is fine.
> +static struct cpuidle_state *cpuidle_state_table;
> +
> +void update_smt_snooze_delay(int snooze)
> +{
> + struct cpuidle_driver *drv = cpuidle_get_driver();
> + if (drv)
> + drv->states[0].target_residency = snooze;
> +}
> +
> +static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
> +{
> +
> + *kt_before = ktime_get_real();
> + *in_purr = mfspr(SPRN_PURR);
> + /*
> + * Indicate to the HV that we are idle. Now would be
> + * a good time to find other work to dispatch.
> + */
> + get_lppaca()->idle = 1;
> + get_lppaca()->donate_dedicated_cpu = 1;
> +}
I notice that you call this on shared processors as well. The old ocde
used to not set donate_dedicated_cpu in that case. I assume that's not a
big deal and that the HV will just ignore it in the shared processor
case but please add a comment after you've verified it.
> +static inline s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
> +{
> + get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
> + get_lppaca()->donate_dedicated_cpu = 0;
> + get_lppaca()->idle = 0;
> +
> + return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +}
> +
> +
> +static int snooze_loop(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + unsigned long in_purr;
> + ktime_t kt_before;
> +
> + idle_loop_prolog(&in_purr, &kt_before);
> +
> + 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();
> +
> + dev->last_residency =
> + (int)idle_loop_epilog(in_purr, kt_before);
> + return index;
> +}
So your snooze loop has no timeout, is that handled by the cpuidle
driver using some kind of timer ? That sounds a lot less efficient than
passing a max delay to the snooze loop to handle getting into a deeper
state after a bit of snoozing rather than interrupting etc...
> +static int dedicated_cede_loop(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + unsigned long in_purr;
> + ktime_t kt_before;
> +
> + idle_loop_prolog(&in_purr, &kt_before);
> +
> + ppc64_runlatch_off();
> + HMT_medium();
> + cede_processor();
> +
> + dev->last_residency =
> + (int)idle_loop_epilog(in_purr, kt_before);
> + return index;
> +}
> +
> +static int shared_cede_loop(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + unsigned long in_purr;
> + ktime_t kt_before;
> +
> + idle_loop_prolog(&in_purr, &kt_before);
> +
> + /*
> + * Yield the processor to the hypervisor. We return if
> + * an external interrupt occurs (which are driven prior
> + * to returning here) or if a prod occurs from another
> + * processor. When returning here, external interrupts
> + * are enabled.
> + */
> + cede_processor();
> +
> + dev->last_residency =
> + (int)idle_loop_epilog(in_purr, kt_before);
> + return index;
> +}
> +
> +/*
> + * States for dedicated partition case.
> + */
> +static struct cpuidle_state dedicated_states[MAX_IDLE_STATE_COUNT] = {
> + { /* Snooze */
> + .name = "snooze",
> + .desc = "snooze",
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 0,
> + .target_residency = 0,
> + .enter = &snooze_loop },
> + { /* CEDE */
> + .name = "CEDE",
> + .desc = "CEDE",
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 1,
> + .target_residency = 10,
> + .enter = &dedicated_cede_loop },
> +};
> +
> +/*
> + * States for shared partition case.
> + */
> +static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = {
> + { /* Shared Cede */
> + .name = "Shared Cede",
> + .desc = "Shared Cede",
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 0,
> + .target_residency = 0,
> + .enter = &shared_cede_loop },
> +};
> +
> +int pseries_notify_cpuidle_add_cpu(int cpu)
> +{
> + struct cpuidle_device *dev =
> + per_cpu_ptr(pseries_idle_cpuidle_devices, cpu);
> + if (dev && cpuidle_get_driver()) {
> + cpuidle_disable_device(dev);
> + cpuidle_enable_device(dev);
> + }
> + return 0;
> +}
> +
> +/*
> + * pseries_idle_cpuidle_driver_init()
> + */
> +static int pseries_idle_cpuidle_driver_init(void)
> +{
Drop the "idle", those names are too long.
> + int cstate;
And use something else than 'cstate', we are among civilized people
here ;-)
> + struct cpuidle_driver *drv = &pseries_idle_driver;
> +
> + drv->state_count = 0;
> +
> + for (cstate = 0; cstate < MAX_IDLE_STATE_COUNT; ++cstate) {
> +
> + if (cstate > max_cstate)
> + break;
> +
> + /* is the state not enabled? */
> + if (cpuidle_state_table[cstate].enter == NULL)
> + continue;
> +
> + drv->states[drv->state_count] = /* structure copy */
> + cpuidle_state_table[cstate];
> +
> + if (cpuidle_state_table == dedicated_states)
> + drv->states[drv->state_count].target_residency =
> + __get_cpu_var(smt_snooze_delay);
> +
> + drv->state_count += 1;
> + }
> +
> + return 0;
> +}
> +
> +/* pseries_idle_devices_uninit(void)
> + * unregister cpuidle devices and de-allocate memory
> + */
> +static void pseries_idle_devices_uninit(void)
> +{
> + int i;
> + struct cpuidle_device *dev;
> +
> + for_each_possible_cpu(i) {
> + dev = per_cpu_ptr(pseries_idle_cpuidle_devices, i);
> + cpuidle_unregister_device(dev);
> + }
> +
> + free_percpu(pseries_idle_cpuidle_devices);
> + return;
> +}
> +
> +/* pseries_idle_devices_init()
> + * allocate, initialize and register cpuidle device
> + */
> +static int pseries_idle_devices_init(void)
> +{
> + int i;
> + struct cpuidle_driver *drv = &pseries_idle_driver;
> + struct cpuidle_device *dev;
> +
> + pseries_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> + if (pseries_idle_cpuidle_devices == NULL)
> + return -ENOMEM;
> +
> + for_each_possible_cpu(i) {
> + dev = per_cpu_ptr(pseries_idle_cpuidle_devices, i);
> + dev->state_count = drv->state_count;
> + dev->cpu = i;
> + if (cpuidle_register_device(dev)) {
> + printk(KERN_DEBUG \
> + "cpuidle_register_device %d failed!\n", i);
> + return -EIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * pseries_idle_probe()
> + * Choose state table for shared versus dedicated partition
> + */
> +static int pseries_idle_probe(void)
> +{
> + if (max_cstate == 0) {
> + printk(KERN_DEBUG "pseries processor idle disabled.\n");
> + return -EPERM;
> + }
> +
> + if (!firmware_has_feature(FW_FEATURE_SPLPAR)) {
> + printk(KERN_DEBUG "Using default idle\n");
> + return -ENODEV;
> + }
Don't even printk here, this will happen on all !pseries machines and
the debug output isn't useful. Also move the check of max-cstate to
after the splpar test.
Overall, I quite like these patches, my comments are all pretty minor,
hopefully the next round should be the one.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] cpuidle: (powerpc) Add cpu_idle_wait() to allow switching of idle routines
From: Benjamin Herrenschmidt @ 2011-11-27 22:48 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linuxppc-dev, linux-pm, linux-kernel, linux-pm
In-Reply-To: <20111117112830.9191.1951.stgit@localhost6.localdomain6>
On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
> This patch provides cpu_idle_wait() routine for the powerpc
> platform which is required by the cpuidle subsystem. This
> routine is requied to change the idle handler on SMP systems.
> The equivalent routine for x86 is in arch/x86/kernel/process.c
> but the powerpc implementation is different.
>
> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
> Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
> ---
No, that patch also adds this idle boot override thing (can you pick a
shorter name for boot_option_idle_override btw ?) which seems unrelated
and without any explanation as to what it's supposed to be about.
Additionally, I'm a bit worried (but maybe we already discussed that a
while back, I don't know) but cpu_idle_wait() has "wait" in the name,
which makes me think it might need to actually -wait- for all cpus to
have come out of the function.
Now your implementation doesn't provide that guarantee. It might be
fine, I don't know, but if it is, you'd better document it well in the
comments surrounding the code, because as it is, all you do is shoot an
interrupt which will cause the target CPU to eventually come out of idle
some time in the future.
Cheers,
Ben.
> arch/powerpc/Kconfig | 4 ++++
> arch/powerpc/include/asm/processor.h | 2 ++
> arch/powerpc/include/asm/system.h | 1 +
> arch/powerpc/kernel/idle.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index b177caa..87f8604 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -87,6 +87,10 @@ config ARCH_HAS_ILOG2_U64
> bool
> default y if 64BIT
>
> +config ARCH_HAS_CPU_IDLE_WAIT
> + bool
> + default y
> +
> config GENERIC_HWEIGHT
> bool
> default y
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index eb11a44..811b7e7 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -382,6 +382,8 @@ static inline unsigned long get_clean_sp(struct pt_regs *regs, int is_32)
> }
> #endif
>
> +enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
> +
> #endif /* __KERNEL__ */
> #endif /* __ASSEMBLY__ */
> #endif /* _ASM_POWERPC_PROCESSOR_H */
> diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h
> index e30a13d..ff66680 100644
> --- a/arch/powerpc/include/asm/system.h
> +++ b/arch/powerpc/include/asm/system.h
> @@ -221,6 +221,7 @@ extern unsigned long klimit;
> extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
>
> extern int powersave_nap; /* set if nap mode can be used in idle loop */
> +void cpu_idle_wait(void);
>
> /*
> * Atomic exchange
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> index 39a2baa..b478c72 100644
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -39,9 +39,13 @@
> #define cpu_should_die() 0
> #endif
>
> +unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE;
> +EXPORT_SYMBOL(boot_option_idle_override);
> +
> static int __init powersave_off(char *arg)
> {
> ppc_md.power_save = NULL;
> + boot_option_idle_override = IDLE_POWERSAVE_OFF;
> return 0;
> }
> __setup("powersave=off", powersave_off);
> @@ -102,6 +106,28 @@ void cpu_idle(void)
> }
> }
>
> +
> +/*
> + * cpu_idle_wait - Used to ensure that all the CPUs come out of the old
> + * idle loop and start using the new idle loop.
> + * Required while changing idle handler on SMP systems.
> + * Caller must have changed idle handler to the new value before the call.
> + */
> +void cpu_idle_wait(void)
> +{
> + int cpu;
> + smp_mb();
> +
> + /* kick all the CPUs so that they exit out of old idle routine */
> + get_online_cpus();
> + for_each_online_cpu(cpu) {
> + if (cpu != smp_processor_id())
> + smp_send_reschedule(cpu);
> + }
> + put_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(cpu_idle_wait);
> +
> int powersave_nap;
>
> #ifdef CONFIG_SYSCTL
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-27 21:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: akpm, fweisbec, containers, lizf, linux-kernel, oleg, linux-pm,
paul, kamezawa.hiroyu
In-Reply-To: <20111127192155.GB4266@google.com>
Hello, Linus.
On Sun, Nov 27, 2011 at 11:21:55AM -0800, Tejun Heo wrote:
> On Thu, Nov 24, 2011 at 08:02:18PM -0800, Linus Torvalds wrote:
> > - do you even *need* two separate locks at all? If you extend the
> > threadgroup_fork_lock to cover exec/exit too (and rename it), then why
> > does that separate cred_guard_mutex make sense at all?
>
> That was mostly choosing the path of least resistance as both locks
> already existed and exec exclusion was added later on, but yeah there
> isn't much to lose by merging those two locks (do_exit nesting inside
> exec will need some care tho). I'll try to unify the two locks.
Hmmm... I tried this but it seems more difficult than originally
expected. The biggest obstacle is that we don't have _interruptible
and _killable rwsem ops.
As proposed, what we have is,
* fork and exit paths are read-protected by group_rwsem. ie. forks
and exits don't block each other.
* exec path is protected by cred_guard_mutex. cred_guard_mutex also
synchronizes parallel exec attempts. (It's probably better to
rename it to exec_mutex)
* thread_group_lock() makes the thread group stable by write-locking
group_rwsem and grabbing cred_guard_mutex. The former excludes
forks and exits and the latter exec.
group_rwsem and cred_guard_mutex can be merged by,
* fork and exit paths read-lock group_rwsem as it currently does.
* exec path write-locks group_rwsem. Note that it'll need to unlock
in de_thread() while waiting for other threads to exit as exit path
can't proceed while group_rwsem is write-locked.
prepare_bprm_creds() should be updated to check signal_pending()
after acquiring group_rwsem.
This introduces synchronization between exec and fork/exit paths,
but I don't think this is something we need to worry about as both
fork-exec and exit-exec scalabilities don't matter.
* thread_group_lock() write-locks group_rwsem.
The problem is that cred_guard_mutex uses _interruptible/_killable
operations and rwsem doesn't have them, so cred_guard_mutex can't be
easily replaced with write-locking group_rwsem.
If the two locks can't be merged, under the proposed scheme, while not
exactly pretty, both fork/exit and exec paths go through single
locking and only the ones which want stable threadgroup need to grab
both locks, so IMHO it is at least reasonable.
Any better ideas?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-27 19:30 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: akpm, paul, lizf, linux-kernel, oleg, linux-pm, Linus Torvalds,
containers, kamezawa.hiroyu
In-Reply-To: <20111125140136.GC23307@somewhere.redhat.com>
Hello, Frederic.
On Fri, Nov 25, 2011 at 03:01:39PM +0100, Frederic Weisbecker wrote:
> > + /*
> > + * @tsk's threadgroup is going through changes - lock out users
> > + * which expect stable threadgroup. Do this before actually
> > + * starting tearing down @tsk so that locked threadgroup has either
> > + * alive or dead tasks, not something inbetween.
> > + */
> > + threadgroup_change_begin(tsk);
> > +
>
> I still wonder why there is a so big coverage of this lock. I mean
> why is it called right before exit_irq_thread() and released so late.
> All we want is to lock cgroup_exit() I think, after which tasks can't be
> migrated.
That way, cgroup plugins never see tasks in the process of
deconstruction. Its ->mm, ->sighand, ->signal and so on either don't
exist or continue to exist across all cgroup callbacks. It's simpler
and safer (especially as bugs in this area would only be visible when
migration and exit race), and if we're gonna synchronize exit path at
all, there isn't anything to lose by excluding the whole thing.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-27 19:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: akpm, fweisbec, containers, lizf, linux-kernel, oleg, linux-pm,
paul, kamezawa.hiroyu
In-Reply-To: <CA+55aFxq1wztMzYhKaY5RHazLBDz4pSXUgiGzTj2wA6EJcDbAw@mail.gmail.com>
Hello, Linus.
On Thu, Nov 24, 2011 at 08:02:18PM -0800, Linus Torvalds wrote:
> - do you even *need* two separate locks at all? If you extend the
> threadgroup_fork_lock to cover exec/exit too (and rename it), then why
> does that separate cred_guard_mutex make sense at all?
That was mostly choosing the path of least resistance as both locks
already existed and exec exclusion was added later on, but yeah there
isn't much to lose by merging those two locks (do_exit nesting inside
exec will need some care tho). I'll try to unify the two locks.
> Taking two locks for the common exit case seems disgusting. What do
> the separate locks buy us?
A bit confused. threadgroup_change_begin() only locks
signal->group_rwsem. ie. fork/exit is protected by
signal->group_rwsem. exec is protected by signal->cred_guard_mutex.
Grabbing both locks gives stable thread group.
> - could we possible avoid the lock(s) entirely for the
> single-threaded case? The fact that ptrace wants to serialize makes me
> say "maybe we can't avoid it", but I thought I'd ask. Even if we do
> need/want two separate locks, do we really want to take them both for
> the case that shouldn't really need even a single one?
Maybe we can avoid grabbing a lock on exec and exit if the task
belongs to single task process but optimizations like that can be very
fragile. I think it would be better to merge the two locks and keep
the locking unconditional.
> Hmm? Simplifying the locking rules is always a good idea, and I think
> this seems to make some of it more straightforward, but at the same
> time I shudder when I look at some of the patches in the series that
> nest locking three locks deep. Ugh.
cgroup_root_mutex separation is to avoid lock recursion via
cgroup_show_options and can probably be done in lighter handed way.
As I'm still getting used to cgroup code, I wanted to resolve the
locking order without changing much else.
Anyways, I'll send separated patch series dealing with fork/exec/exit
synchronization.
Thank you.
--
tejun
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox