From: Arun R Bharadwaj <hidden> Date: 2009-08-27 11:49:38
Hi,
Changes from previous iteration:
--------------------------------
* Remove the EXPORT_SYMBOL(pm_idle) from
arch/powerpc/platform/pseries/processor_idle.c and introduce a
generic cpuidle_pm_idle in cpuidle.c which was earlier assuming pm_idle
to be the default idle routine. (As suggested by Peter and Ben).
* Move the cpu_idle_wait function from arch/powerpc/platforms/pseries/setup.c
to arch/powerpc/kernel/idle.c which would prevent breaking the build of
other platforms. (As suggested by Ben).
---------------------------------------
"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/26/233
Patches included in this set:
------------------------------
PATCH 1/4 - Enable cpuidle for pSeries.
PATCH 2/4 - Introduce architecture independent cpuidle_pm_idle in
drivers/cpuidle/cpuidle.c
PATCH 3/4 - Register for cpuidle_pm_idle in drivers/acpi/processor_idle.c
and arch/arm/mach-kirkwood/cpuidle.c
PATCH 4/4 - Implement Pseries Processor Idle idle module
--arun
From: Arun R Bharadwaj <hidden> Date: 2009-08-27 11:52:03
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
This patch enables the cpuidle option in Kconfig for pSeries.
Currently cpuidle infrastructure is enabled only for x86 and ARM.
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/kernel/idle.c | 19 +++++++++++++++++++
3 files changed, 38 insertions(+)
Index: linux.trees.git/arch/powerpc/Kconfig
===================================================================
@@ -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"++configPSERIES_PROCESSOR_IDLE+bool"Idle Power Management Support for pSeries"+depends onPPC_PSERIES&&CPU_IDLE+defaulty+help+IdlePowerManagementSupportforpSeries.Thishooksontocpuidle+infrastructuretohelpinidlecpupowermanagement.++endmenu+menu"Kernel options"configHIGHMEM
@@ -102,6 +102,25 @@ void cpu_idle(void)}}+staticvoiddo_nothing(void*unused)+{+}++/*+*cpu_idle_wait-UsedtoensurethatalltheCPUsdiscardoldvalueof+*ppc_md.power_saveandupdatetonewvalue.+*Requiredwhilechangingppc_md.power_savehandleronSMPsystems.+*Callermusthavechangedppc_md.power_savetothenewvaluebeforethecall.+*/+voidcpu_idle_wait(void)+{+/* Ensure that new value of ppc_md.power_save is set */+smp_mb();+/* kick all the CPUs so that they exit out of ppc_md.power_save */+smp_call_function(do_nothing,NULL,1);+}+EXPORT_SYMBOL_GPL(cpu_idle_wait);+intpowersave_nap;#ifdef CONFIG_SYSCTL
From: Arun R Bharadwaj <hidden> Date: 2009-08-27 11:54:05
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
Cpuidle infrastructure assumes pm_idle as the default idle routine.
But, ppc_md.power_save is the default idle callback in case of pSeries.
So, create a more generic, architecture independent cpuidle_pm_idle
function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
of architectures to be set to cpuidle_pm_idle.
Signed-off-by: Arun R Bharadwaj <redacted>
---
drivers/cpuidle/cpuidle.c | 12 +++++++-----
include/linux/cpuidle.h | 7 +++++++
2 files changed, 14 insertions(+), 5 deletions(-)
Index: linux.trees.git/drivers/cpuidle/cpuidle.c
===================================================================
@@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)*/voidcpuidle_install_idle_handler(void){-if(enabled_devices&&(pm_idle!=cpuidle_idle_call)){+if(enabled_devices&&(cpuidle_pm_idle!=cpuidle_idle_call)){/* Make sure all changes finished before we switch to new idle */smp_wmb();-pm_idle=cpuidle_idle_call;+cpuidle_pm_idle=cpuidle_idle_call;}}
From: Arun R Bharadwaj <hidden> Date: 2009-08-27 11:55:42
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
Set the idle routine to cpuidle_pm_idle after registering cpuidle
devices. Earlier pm_idle was assumed as the defualt idle loop by
cpuidle infrastructure. This is changed to an architecture independent
cpuidle_pm_idle.
There are 2 instances which are using cpuidle infrastructure currently.
This patch makes the change in both the places.
Signed-off-by: Arun R Bharadwaj <redacted>
---
arch/arm/mach-kirkwood/cpuidle.c | 6 ++++++
drivers/acpi/processor_idle.c | 5 +++++
2 files changed, 11 insertions(+)
Index: linux.trees.git/arch/arm/mach-kirkwood/cpuidle.c
===================================================================
From: Arun R Bharadwaj <hidden> Date: 2009-08-27 11:57:30
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
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 | 178 ++++++++++++++++++++++++
arch/powerpc/platforms/pseries/pseries.h | 14 +
arch/powerpc/platforms/pseries/setup.c | 3
4 files changed, 193 insertions(+), 3 deletions(-)
Index: linux.trees.git/arch/powerpc/platforms/pseries/Makefile
===================================================================
From: Peter Zijlstra <hidden> Date: 2009-08-27 12:54:44
On Thu, 2009-08-27 at 17:23 +0530, Arun R Bharadwaj wrote:
quoted hunk
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
Cpuidle infrastructure assumes pm_idle as the default idle routine.
But, ppc_md.power_save is the default idle callback in case of pSeries.
So, create a more generic, architecture independent cpuidle_pm_idle
function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
of architectures to be set to cpuidle_pm_idle.
Signed-off-by: Arun R Bharadwaj <redacted>
---
drivers/cpuidle/cpuidle.c | 12 +++++++-----
include/linux/cpuidle.h | 7 +++++++
2 files changed, 14 insertions(+), 5 deletions(-)
Index: linux.trees.git/drivers/cpuidle/cpuidle.c
===================================================================
@@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)*/voidcpuidle_install_idle_handler(void){-if(enabled_devices&&(pm_idle!=cpuidle_idle_call)){+if(enabled_devices&&(cpuidle_pm_idle!=cpuidle_idle_call)){/* Make sure all changes finished before we switch to new idle */smp_wmb();-pm_idle=cpuidle_idle_call;+cpuidle_pm_idle=cpuidle_idle_call;}}
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-08-27 21:30:46
On Thu, 2009-08-27 at 14:53 +0200, Peter Zijlstra wrote:
I'm not quite seeing how this makes anything any better. Not we have 3
function pointers, where 1 should suffice.
There's also the question of us having different "idle" vs.
"power_save", the former being the entire idle loop, the later being the
part that does put the processor into power.
At what level are we trying to change the loop here ?
There are some requirements of things to do in our idle loop that really
don't have their place in generic drivers/* code.
Ben.
/me wonders what's wrong with something like:
struct idle_func_desc {
int power;
int latency;
void (*idle)(void);
struct list_head list;
};
static void spin_idle(void)
{
for (;;)
cpu_relax();
}
static idle_func_desc default_idle_func = {
power = 0, /* doesn't safe any power */
latency = INT_MAX, /* has max latency */
idle = spin_idle,
list = INIT_LIST_HEAD(default_idle_func.list),
};
void (*idle_func)(void);
static struct list_head idle_func_list;
static void pick_idle_func(void)
{
struct idle_func_desc *desc, *idle = &default_idle_desc;
list_for_each_entry(desc, &idle_func_list, list) {
if (desc->power < idle->power)
continue;
if (desc->latency > target_latency);
continue;
idle = desc;
}
pm_idle = idle->idle;
}
void register_idle_func(struct idle_func_desc *desc)
{
WARN_ON_ONCE(!list_empty(&desc->list));
list_add_tail(&idle_func_list, &desc->list);
pick_idle_func();
}
void unregister_idle_func(struct idle_func_desc *desc)
{
WARN_ON_ONCE(list_empty(&desc->list));
list_del_init(&desc->list);
if (idle_func == desc->idle)
pick_idle_func();
}
From: Arun R Bharadwaj <hidden> Date: 2009-08-28 04:49:08
* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:
On Thu, 2009-08-27 at 17:23 +0530, Arun R Bharadwaj wrote:
quoted
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
Cpuidle infrastructure assumes pm_idle as the default idle routine.
But, ppc_md.power_save is the default idle callback in case of pSeries.
So, create a more generic, architecture independent cpuidle_pm_idle
function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
of architectures to be set to cpuidle_pm_idle.
Signed-off-by: Arun R Bharadwaj <redacted>
---
drivers/cpuidle/cpuidle.c | 12 +++++++-----
include/linux/cpuidle.h | 7 +++++++
2 files changed, 14 insertions(+), 5 deletions(-)
Index: linux.trees.git/drivers/cpuidle/cpuidle.c
===================================================================
@@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)*/voidcpuidle_install_idle_handler(void){-if(enabled_devices&&(pm_idle!=cpuidle_idle_call)){+if(enabled_devices&&(cpuidle_pm_idle!=cpuidle_idle_call)){/* Make sure all changes finished before we switch to new idle */smp_wmb();-pm_idle=cpuidle_idle_call;+cpuidle_pm_idle=cpuidle_idle_call;}}
I'm not quite seeing how this makes anything any better. Not we have 3
function pointers, where 1 should suffice.
Not really. We already do have pm_idle in case of x86 and
ppc_md.power_save in case of POWER. So here I'm only introducing
cpuidle_pm_idle which can be used by doing a
ppc_md.power_save = cpuidle_pm_idle;
/me wonders what's wrong with something like:
struct idle_func_desc {
int power;
int latency;
void (*idle)(void);
struct list_head list;
};
static void spin_idle(void)
{
for (;;)
cpu_relax();
}
static idle_func_desc default_idle_func = {
power = 0, /* doesn't safe any power */
latency = INT_MAX, /* has max latency */
idle = spin_idle,
list = INIT_LIST_HEAD(default_idle_func.list),
};
void (*idle_func)(void);
static struct list_head idle_func_list;
static void pick_idle_func(void)
{
struct idle_func_desc *desc, *idle = &default_idle_desc;
list_for_each_entry(desc, &idle_func_list, list) {
if (desc->power < idle->power)
continue;
if (desc->latency > target_latency);
continue;
idle = desc;
}
pm_idle = idle->idle;
}
This only does the job of picking the right idle loop for current
latency and power requirement. This is already done in ladder/menu
governors under the routines menu_select()/ladder_select().
I'm not sure whats the purpose of it here.
Here we are only concerned about the main idle loop, which is
pm_idle/ppc_md.power_save. After setting the main idle loop to
cpuidle_pm_idle, that would call cpuidle_idle_call() which would do
the job of picking the right low level idle loop based on latency and
other requirements.
From: Arun R Bharadwaj <hidden> Date: 2009-08-28 06:14:39
* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:
Hi Peter, Ben,
I've put the whole thing in a sort of a block diagram. Hope it
explains things more clearly.
----------------
| CPUIDLE | (Select idle states like
| GOVERNORS | C1, C1e, C6 etc in case
| (Menu/Ladder)| x86 & nap, snooze in
| | case of POWER - based on
---------------- latency & power req)
^
|
|
|
|
|
---------- ----------------- -------------
| | | | | PSERIES |
| ACPI |------------------> | CPUIDLE | <--------------| IDLE |
| | | | | |
---------- ----------------- -------------
Main idle routine- pm_idle() Main idle routine-
ppc_md.power_save()
pm_idle = cpuidle_pm_idle; ppc_md.power_save =
(start using cpuidle's idle cpuidle_pm_idle();
loop, which internally calls
governor to select the right
state to go into).
Relavent code snippet from drivers/cpuidle/cpuidle.c
-------------------------------------
static void cpuidle_idle_call(void)
{
............
............
/* Call the menu_select() to select the idle state to enter. */
next_state = cpuidle_curr_governor->select(dev);
............
............
/*
* Enter the idle state previously selected. target_state->enter
* would call pseries_cpuidle_loop() which selects nap/snooze
* /
dev->last_residency = target_state->enter(dev, target_state);
}
void cpuidle_install_idle_handler(void)
{
.........
.........
cpuidle_pm_idle = cpuidle_idle_call;
}
--arun
From: Peter Zijlstra <hidden> Date: 2009-08-28 06:41:35
On Fri, 2009-08-28 at 10:19 +0530, Arun R Bharadwaj wrote:
This only does the job of picking the right idle loop for current
latency and power requirement. This is already done in ladder/menu
governors under the routines menu_select()/ladder_select().
I'm not sure whats the purpose of it here.
I can't seem to find ladder_select() but menu_select() doesn't manage
pm_idle and its not clear what it does manage.
Here we are only concerned about the main idle loop, which is
pm_idle/ppc_md.power_save. After setting the main idle loop to
cpuidle_pm_idle, that would call cpuidle_idle_call() which would do
the job of picking the right low level idle loop based on latency and
other requirements.
It also gets pm_idle unexported and avoids anybody directly tinkering
with the function pointer, _that_ is the whole goal.
pm_idle is it exists today, and the whole cpuidle_{un,}install*() is
utter crap. It relies on unmanaged access to this function pointer.
/me stop looking at drivers/cpuidle/, convoluted mess that is, shame on
you for wanting to have anything to do with it.
From: Arun R Bharadwaj <hidden> Date: 2009-08-28 06:43:35
* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:
On Thu, 2009-08-27 at 17:23 +0530, Arun R Bharadwaj wrote:
quoted
* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:
Cpuidle infrastructure assumes pm_idle as the default idle routine.
But, ppc_md.power_save is the default idle callback in case of pSeries.
So, create a more generic, architecture independent cpuidle_pm_idle
function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
of architectures to be set to cpuidle_pm_idle.
Signed-off-by: Arun R Bharadwaj <redacted>
---
drivers/cpuidle/cpuidle.c | 12 +++++++-----
include/linux/cpuidle.h | 7 +++++++
2 files changed, 14 insertions(+), 5 deletions(-)
Index: linux.trees.git/drivers/cpuidle/cpuidle.c
===================================================================
@@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)*/voidcpuidle_install_idle_handler(void){-if(enabled_devices&&(pm_idle!=cpuidle_idle_call)){+if(enabled_devices&&(cpuidle_pm_idle!=cpuidle_idle_call)){/* Make sure all changes finished before we switch to new idle */smp_wmb();-pm_idle=cpuidle_idle_call;+cpuidle_pm_idle=cpuidle_idle_call;}}
I'm not quite seeing how this makes anything any better. Not we have 3
function pointers, where 1 should suffice.
Or, can we have something like:
(if exporting a function is ok, instead of exporting a function
pointer).
in drivers/cpuidle/cpuidle.c
void (*return_cpuidle_handler(void))(void)
{
return cpuidle_pm_idle;
}
EXPORT_SYMBOL(return_cpuidle_handler);
and from pseries/processor_idle.c,
ppc_md.power_save = return_cpuidle_handler;
--arun
/me wonders what's wrong with something like:
struct idle_func_desc {
int power;
int latency;
void (*idle)(void);
struct list_head list;
};
static void spin_idle(void)
{
for (;;)
cpu_relax();
}
static idle_func_desc default_idle_func = {
power = 0, /* doesn't safe any power */
latency = INT_MAX, /* has max latency */
idle = spin_idle,
list = INIT_LIST_HEAD(default_idle_func.list),
};
void (*idle_func)(void);
static struct list_head idle_func_list;
static void pick_idle_func(void)
{
struct idle_func_desc *desc, *idle = &default_idle_desc;
list_for_each_entry(desc, &idle_func_list, list) {
if (desc->power < idle->power)
continue;
if (desc->latency > target_latency);
continue;
idle = desc;
}
pm_idle = idle->idle;
}
void register_idle_func(struct idle_func_desc *desc)
{
WARN_ON_ONCE(!list_empty(&desc->list));
list_add_tail(&idle_func_list, &desc->list);
pick_idle_func();
}
void unregister_idle_func(struct idle_func_desc *desc)
{
WARN_ON_ONCE(list_empty(&desc->list));
list_del_init(&desc->list);
if (idle_func == desc->idle)
pick_idle_func();
}
From: Peter Zijlstra <hidden> Date: 2009-08-28 06:49:29
On Fri, 2009-08-28 at 11:44 +0530, Arun R Bharadwaj wrote:
* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:
Hi Peter, Ben,
I've put the whole thing in a sort of a block diagram. Hope it
explains things more clearly.
----------------
| CPUIDLE | (Select idle states like
| GOVERNORS | C1, C1e, C6 etc in case
| (Menu/Ladder)| x86 & nap, snooze in
| | case of POWER - based on
---------------- latency & power req)
^
|
|
|
|
|
---------- ----------------- -------------
| | | | | PSERIES |
| ACPI |------------------> | CPUIDLE | <--------------| IDLE |
| | | | | |
---------- ----------------- -------------
Main idle routine- pm_idle() Main idle routine-
ppc_md.power_save()
pm_idle = cpuidle_pm_idle; ppc_md.power_save =
(start using cpuidle's idle cpuidle_pm_idle();
loop, which internally calls
governor to select the right
state to go into).
Relavent code snippet from drivers/cpuidle/cpuidle.c
-------------------------------------
static void cpuidle_idle_call(void)
{
............
............
/* Call the menu_select() to select the idle state to enter. */
next_state = cpuidle_curr_governor->select(dev);
............
............
/*
* Enter the idle state previously selected. target_state->enter
* would call pseries_cpuidle_loop() which selects nap/snooze
* /
dev->last_residency = target_state->enter(dev, target_state);
}
void cpuidle_install_idle_handler(void)
{
.........
.........
cpuidle_pm_idle = cpuidle_idle_call;
}
All I'm seeing here is a frigging mess.
How on earths can something called: cpuidle_install_idle_handler() have
a void argument, _WHAT_ handler is it going to install?
So somehow you added to the ACPI mess by now having 3 wild function
pointers, that's _NOT_ progress.
* Peter Zijlstra [off-list ref] [2009-08-28 08:48:05]:
On Fri, 2009-08-28 at 11:44 +0530, Arun R Bharadwaj wrote:
quoted
* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:
Hi Peter, Ben,
I've put the whole thing in a sort of a block diagram. Hope it
explains things more clearly.
----------------
| CPUIDLE | (Select idle states like
| GOVERNORS | C1, C1e, C6 etc in case
| (Menu/Ladder)| x86 & nap, snooze in
| | case of POWER - based on
---------------- latency & power req)
^
|
|
|
|
|
---------- ----------------- -------------
| | | | | PSERIES |
| ACPI |------------------> | CPUIDLE | <--------------| IDLE |
| | | | | |
---------- ----------------- -------------
Main idle routine- pm_idle() Main idle routine-
ppc_md.power_save()
pm_idle = cpuidle_pm_idle; ppc_md.power_save =
(start using cpuidle's idle cpuidle_pm_idle();
loop, which internally calls
governor to select the right
state to go into).
Relavent code snippet from drivers/cpuidle/cpuidle.c
-------------------------------------
static void cpuidle_idle_call(void)
{
............
............
/* Call the menu_select() to select the idle state to enter. */
next_state = cpuidle_curr_governor->select(dev);
............
............
/*
* Enter the idle state previously selected. target_state->enter
* would call pseries_cpuidle_loop() which selects nap/snooze
* /
dev->last_residency = target_state->enter(dev, target_state);
}
void cpuidle_install_idle_handler(void)
{
.........
.........
cpuidle_pm_idle = cpuidle_idle_call;
}
All I'm seeing here is a frigging mess.
How on earths can something called: cpuidle_install_idle_handler() have
a void argument, _WHAT_ handler is it going to install?
Peter, I think that is a typo, we need a function pointer like your
snippet of code showed
So somehow you added to the ACPI mess by now having 3 wild function
pointers, that's _NOT_ progress.
The goal, IIUC is to integrate three modules
1. CPUIdle - for idle CPU management
2. Architecture specific code for idle detection
3. CPUIdle governor
Again, if IIUC
The architecture specific idle code would call an idle loop that would
call into the CPUIdle framework, which inturn would depend on the
governor selected.
Passing void* is a mess, we need function pointer registration
and a framework so that we can query what is registered.
--
Balbir
All I'm seeing here is a frigging mess.
How on earths can something called: cpuidle_install_idle_handler() have
a void argument, _WHAT_ handler is it going to install?
Argh, now I see, it installs itself as the platform idle handler.
so cpuidle_install_idle_handler() pokes at the unmanaged pm_idle pointer
to make cpuidle take control.
On module load it does:
pm_idle_old = pm_idle;
then in the actual idle loop it does:
if (!dev || !dev->enabled) {
if (pm_idle_old)
pm_idle_old();
who is to say that the pointer stored at module init time is still
around at that time?
So cpuidle recognised the pm_idle stuff was a flaky, but instead of
fixing it, they build a whole new layer on top of it. Brilliant.
/me goes mark this whole thread read, I've got enough things to do.
All I'm seeing here is a frigging mess.
How on earths can something called: cpuidle_install_idle_handler() have
a void argument, _WHAT_ handler is it going to install?
Argh, now I see, it installs itself as the platform idle handler.
so cpuidle_install_idle_handler() pokes at the unmanaged pm_idle pointer
to make cpuidle take control.
On module load it does:
pm_idle_old = pm_idle;
then in the actual idle loop it does:
if (!dev || !dev->enabled) {
if (pm_idle_old)
pm_idle_old();
who is to say that the pointer stored at module init time is still
around at that time?
So cpuidle recognised the pm_idle stuff was a flaky, but instead of
fixing it, they build a whole new layer on top of it. Brilliant.
/me goes mark this whole thread read, I've got enough things to do.
Hi Peter,
I understand that you are frustrated with the mess. We are willing
to clean up the pm_idle pointer at the moment before the cpuidle
framework is exploited my more archs.
At this moment we need your suggestions on what interface should we
call 'clean' and safe.
cpuidle.c and the arch independent cpuidle subsystem is not a module
and its cpuidle_idle_call() routine is valid and can be safely called
from arch dependent process.c
The fragile part is how cpuidle_idle_call() is hooked onto arch
specific cpu_idle() function at runtime. x86 has the pm_idle pointer
exported while powerpc has ppc_md.power_save pointer being called.
At cpuidle init time we can override the platform idle function, but
that will mean we are including arch specific code in cpuidle.c
Do you think having an exported function in cpuidle.c to give us the
correct pointer to arch code be better than the current situation:
in drivers/cpuidle/cpuidle.c
void (*get_cpuidle_handler(void))(void)
{
return cpuidle_pm_idle;
}
EXPORT_SYMBOL(get_cpuidle_handler);
and from pseries/processor_idle.c,
ppc_md.power_save = get_cpuidle_handler();
--Vaidy