Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
* Tested on intel core 2 duo T9500 * Tested on vexpress by Lorenzo Pieralsi * Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com --- drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index bfc31cb..e1f6330 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = {
DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors); - +extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly;
/* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver;
/* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
On Friday, September 07, 2012, Daniel Lezcano wrote:
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
Well, the changelog is wrong (because the scope of the variable is not reduced by moving it out of the header) and I don't see the point.
Is there any _real_ problem with that definition in processor.h?
Rafael
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index bfc31cb..e1f6330 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = { DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors);
+extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly; /* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver; /* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
On 09/07/2012 11:19 PM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
Well, the changelog is wrong (because the scope of the variable is not reduced by moving it out of the header) and I don't see the point.
Yes, you are right.
Is there any _real_ problem with that definition in processor.h?
It is not a real problem. There is no issue fixed by this patch. It is just reorganizing the code little by little. The intent is to group what is related to cpuidle to the C files here processor_driver.c.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index bfc31cb..e1f6330 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = { DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors);
+extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly; /* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver; /* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
On Tuesday, September 11, 2012, Daniel Lezcano wrote:
On 09/07/2012 11:19 PM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
Well, the changelog is wrong (because the scope of the variable is not reduced by moving it out of the header) and I don't see the point.
Yes, you are right.
Is there any _real_ problem with that definition in processor.h?
It is not a real problem. There is no issue fixed by this patch. It is just reorganizing the code little by little. The intent is to group what is related to cpuidle to the C files here processor_driver.c.
However, it is not recommended to put "extern something" type of declarations into *.c files. All of them should go into headers (although not necessarily in include/linux for that matter).
Thanks, Rafael
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com --- drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644);
+static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device); + static int disabled_by_idle_boot_param(void) { return boot_option_idle_override == IDLE_POLL || @@ -998,7 +1000,7 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) int i, count = CPUIDLE_DRIVER_STATE_START; struct acpi_processor_cx *cx; struct cpuidle_state_usage *state_usage; - struct cpuidle_device *dev = &pr->power.dev; + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id);
if (!pr->flags.power_setup_done) return -EINVAL; @@ -1130,6 +1132,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) int acpi_processor_hotplug(struct acpi_processor *pr) { int ret = 0; + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id);
if (disabled_by_idle_boot_param()) return 0; @@ -1145,11 +1148,11 @@ int acpi_processor_hotplug(struct acpi_processor *pr) return -ENODEV;
cpuidle_pause_and_lock(); - cpuidle_disable_device(&pr->power.dev); + cpuidle_disable_device(dev); acpi_processor_get_power_info(pr); if (pr->flags.power) { acpi_processor_setup_cpuidle_cx(pr); - ret = cpuidle_enable_device(&pr->power.dev); + ret = cpuidle_enable_device(dev); } cpuidle_resume_and_unlock();
@@ -1160,6 +1163,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) { int cpu; struct acpi_processor *_pr; + struct cpuidle_device *dev;
if (disabled_by_idle_boot_param()) return 0; @@ -1190,7 +1194,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) _pr = per_cpu(processors, cpu); if (!_pr || !_pr->flags.power_setup_done) continue; - cpuidle_disable_device(&_pr->power.dev); + dev = &per_cpu(acpi_cpuidle_device, cpu); + cpuidle_disable_device(dev); }
/* Populate Updated C-state information */ @@ -1204,7 +1209,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) acpi_processor_get_power_info(_pr); if (_pr->flags.power) { acpi_processor_setup_cpuidle_cx(_pr); - cpuidle_enable_device(&_pr->power.dev); + dev = &per_cpu(acpi_cpuidle_device, cpu); + cpuidle_enable_device(dev); } } put_online_cpus(); @@ -1221,6 +1227,7 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, { acpi_status status = 0; int retval; + struct cpuidle_device *dev; static int first_run;
if (disabled_by_idle_boot_param()) @@ -1270,7 +1277,9 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, * must already be registered before registering device */ acpi_processor_setup_cpuidle_cx(pr); - retval = cpuidle_register_device(&pr->power.dev); + + dev = &per_cpu(acpi_cpuidle_device, pr->id); + retval = cpuidle_register_device(dev); if (retval) { if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver); @@ -1284,11 +1293,13 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device) { + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id); + if (disabled_by_idle_boot_param()) return 0;
if (pr->flags.power) { - cpuidle_unregister_device(&pr->power.dev); + cpuidle_unregister_device(dev); acpi_processor_registered--; if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver); diff --git a/include/acpi/processor.h b/include/acpi/processor.h index 8b2c39a..4d98ec8 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -3,7 +3,6 @@
#include <linux/kernel.h> #include <linux/cpu.h> -#include <linux/cpuidle.h> #include <linux/thermal.h> #include <asm/acpi.h>
@@ -64,7 +63,6 @@ struct acpi_processor_cx { };
struct acpi_processor_power { - struct cpuidle_device dev; struct acpi_processor_cx *state; unsigned long bm_check_timestamp; u32 default_state;
On Fri, Sep 7, 2012 at 3:49 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Reword a little to make it easier to read:
In order to be consistent with the rest of the drivers and for the per-cpu states coming after this patch, this patch moves the cpuidle_device field out of the acpi_processor_power structure.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644);
+static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
static int disabled_by_idle_boot_param(void) { return boot_option_idle_override == IDLE_POLL || @@ -998,7 +1000,7 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) int i, count = CPUIDLE_DRIVER_STATE_START; struct acpi_processor_cx *cx; struct cpuidle_state_usage *state_usage;
struct cpuidle_device *dev = &pr->power.dev;
struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id); if (!pr->flags.power_setup_done) return -EINVAL;@@ -1130,6 +1132,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) int acpi_processor_hotplug(struct acpi_processor *pr) { int ret = 0;
struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id); if (disabled_by_idle_boot_param()) return 0;@@ -1145,11 +1148,11 @@ int acpi_processor_hotplug(struct acpi_processor *pr) return -ENODEV;
cpuidle_pause_and_lock();
cpuidle_disable_device(&pr->power.dev);
cpuidle_disable_device(dev); acpi_processor_get_power_info(pr); if (pr->flags.power) { acpi_processor_setup_cpuidle_cx(pr);
ret = cpuidle_enable_device(&pr->power.dev);
ret = cpuidle_enable_device(dev); } cpuidle_resume_and_unlock();@@ -1160,6 +1163,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) { int cpu; struct acpi_processor *_pr;
struct cpuidle_device *dev; if (disabled_by_idle_boot_param()) return 0;@@ -1190,7 +1194,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) _pr = per_cpu(processors, cpu); if (!_pr || !_pr->flags.power_setup_done) continue;
cpuidle_disable_device(&_pr->power.dev);
dev = &per_cpu(acpi_cpuidle_device, cpu);cpuidle_disable_device(dev); } /* Populate Updated C-state information */@@ -1204,7 +1209,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) acpi_processor_get_power_info(_pr); if (_pr->flags.power) { acpi_processor_setup_cpuidle_cx(_pr);
cpuidle_enable_device(&_pr->power.dev);
dev = &per_cpu(acpi_cpuidle_device, cpu);cpuidle_enable_device(dev); } } put_online_cpus();@@ -1221,6 +1227,7 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, { acpi_status status = 0; int retval;
struct cpuidle_device *dev; static int first_run; if (disabled_by_idle_boot_param())@@ -1270,7 +1277,9 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, * must already be registered before registering device */ acpi_processor_setup_cpuidle_cx(pr);
retval = cpuidle_register_device(&pr->power.dev);
dev = &per_cpu(acpi_cpuidle_device, pr->id);retval = cpuidle_register_device(dev); if (retval) { if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver);@@ -1284,11 +1293,13 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device) {
struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id);if (disabled_by_idle_boot_param()) return 0; if (pr->flags.power) {
cpuidle_unregister_device(&pr->power.dev);
cpuidle_unregister_device(dev); acpi_processor_registered--; if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver);diff --git a/include/acpi/processor.h b/include/acpi/processor.h index 8b2c39a..4d98ec8 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -3,7 +3,6 @@
#include <linux/kernel.h> #include <linux/cpu.h> -#include <linux/cpuidle.h> #include <linux/thermal.h> #include <asm/acpi.h>
@@ -64,7 +63,6 @@ struct acpi_processor_cx { };
struct acpi_processor_power {
struct cpuidle_device dev; struct acpi_processor_cx *state; unsigned long bm_check_timestamp; u32 default_state;-- 1.7.5.4
linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
On Friday, September 07, 2012, Daniel Lezcano wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644); +static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
Well. Why are you moving that thing into the percpu memory? It doesn't have to be per-CPU and storing it there just wastes the room.
Thanks, Rafael
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644); +static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
Well. Why are you moving that thing into the percpu memory? It doesn't have to be per-CPU and storing it there just wastes the room.
Sorry, it is per-CPU already, scratch that.
Thanks, Rafael
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644); +static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
Well. Why are you moving that thing into the percpu memory? It doesn't have to be per-CPU and storing it there just wastes the room.
Sorry, it is per-CPU already, scratch that.
Well, no, it isn't. So I was right originally (boy, that code _is_ confusing).
So originally you had per-CPU pointers called 'processors' that each pointed to a struct acpi_processor object created by acpi_processor_add() in slab memory. Your patch doesn't touch those pointers, so they are still there.
In addition to them it creates a number of static per-CPU objects that previously were stored in those struct acpi_processor object mentioned above. These things need not be stored in percpu memory.
Thanks, Rafael
On 09/08/2012 12:06 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644); +static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
Well. Why are you moving that thing into the percpu memory? It doesn't have to be per-CPU and storing it there just wastes the room.
Sorry, it is per-CPU already, scratch that.
Well, no, it isn't. So I was right originally (boy, that code _is_ confusing).
So originally you had per-CPU pointers called 'processors' that each pointed to a struct acpi_processor object created by acpi_processor_add() in slab memory. Your patch doesn't touch those pointers, so they are still there.
Yes, the purpose of this patch is the same as the other patches which is separate the cpuidle code from the rest of the acpi code. It is part of the cleanup.
In addition to them it creates a number of static per-CPU objects that previously were stored in those struct acpi_processor object mentioned above. These things need not be stored in percpu memory.
Agreed, this patch makes the cpuidle driver to consume more per cpu memory. Is it acceptable to create a per cpu pointer for the cpuidle devices which will be allocated in the processor_driver init function like the intel_idle driver ? We keep the same memory consumption while we are moving the cpuidle specific code the C file.
By the way, most of the cpuidle drivers for ARM are defining a static cpuidle device structure per cpu. I guess your remark for acpi applies to them too. Not easy to make all these drivers to converge to the same code scheme ...
Thanks -- Daniel
On Tuesday, September 11, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:06 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com
drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index de89624..084b1d2 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644); +static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device);
Well. Why are you moving that thing into the percpu memory? It doesn't have to be per-CPU and storing it there just wastes the room.
Sorry, it is per-CPU already, scratch that.
Well, no, it isn't. So I was right originally (boy, that code _is_ confusing).
So originally you had per-CPU pointers called 'processors' that each pointed to a struct acpi_processor object created by acpi_processor_add() in slab memory. Your patch doesn't touch those pointers, so they are still there.
Yes, the purpose of this patch is the same as the other patches which is separate the cpuidle code from the rest of the acpi code. It is part of the cleanup.
In addition to them it creates a number of static per-CPU objects that previously were stored in those struct acpi_processor object mentioned above. These things need not be stored in percpu memory.
Agreed, this patch makes the cpuidle driver to consume more per cpu memory. Is it acceptable to create a per cpu pointer for the cpuidle devices which will be allocated in the processor_driver init function like the intel_idle driver ? We keep the same memory consumption while we are moving the cpuidle specific code the C file.
I'm not really sure what you mean hear, care to elaborate?
By the way, most of the cpuidle drivers for ARM are defining a static cpuidle device structure per cpu. I guess your remark for acpi applies to them too.
Yes, it does. Percpu memory is limited and should only be used for things that really need to be stored there.
Not easy to make all these drivers to converge to the same code scheme ...
I agree, but then I'm not sure it's a problem if they are slightly different.
Thanks, Rafael
The cpuidle core takes care of filling this field from drv->state_count.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/acpi/processor_idle.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 084b1d2..fc4757e 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1035,8 +1035,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) break; }
- dev->state_count = count; - if (!count) return -EINVAL;
Patch 1 and 3 are independent cleanups. Perhaps you can separate them out from the per-cpu latency series in case you have to repost.
On Fri, Sep 7, 2012 at 3:49 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The cpuidle core takes care of filling this field from drv->state_count.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/acpi/processor_idle.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 084b1d2..fc4757e 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1035,8 +1035,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) break; }
dev->state_count = count;if (!count) return -EINVAL;-- 1.7.5.4
linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
On Friday, September 07, 2012, Daniel Lezcano wrote:
The cpuidle core takes care of filling this field from drv->state_count.
I'm not quite sure this is always valid. If dev has already been initialized and dev->state_count is different from 0, cpuidle_enable_device() doesn't actually change it.
Have you considered all of the possible scenarios?
Rafael
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/acpi/processor_idle.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 084b1d2..fc4757e 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1035,8 +1035,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) break; }
- dev->state_count = count;
- if (!count) return -EINVAL;
On 09/07/2012 11:50 PM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
The cpuidle core takes care of filling this field from drv->state_count.
I'm not quite sure this is always valid. If dev has already been initialized and dev->state_count is different from 0, cpuidle_enable_device() doesn't actually change it.
Have you considered all of the possible scenarios?
Ok, there is the scenario where the ACPI supports _CST. At runtime, ACPI may notify the OS a C-state changed for a specific cpu and this is done through 'acpi_processor_cst_has_changed' followed by 'acpi_processor_setup_cpuidle_cx'.
So at the end, we could have different cpus with one cpu with less C-states than the other, if I understood correctly ACPIspec30.pdf => page 262 :)
In conclusion, we should keep this variable filled from there and keep this in mind in cpuidle.c in order to not break acpi in the future. Maybe a comment in cpuidle.c would help ... especially with a single C-state registration.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/acpi/processor_idle.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 084b1d2..fc4757e 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1035,8 +1035,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) break; }
- dev->state_count = count;
- if (!count) return -EINVAL;
On Sunday, September 16, 2012, Daniel Lezcano wrote:
On 09/07/2012 11:50 PM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
The cpuidle core takes care of filling this field from drv->state_count.
I'm not quite sure this is always valid. If dev has already been initialized and dev->state_count is different from 0, cpuidle_enable_device() doesn't actually change it.
Have you considered all of the possible scenarios?
Ok, there is the scenario where the ACPI supports _CST. At runtime, ACPI may notify the OS a C-state changed for a specific cpu and this is done through 'acpi_processor_cst_has_changed' followed by 'acpi_processor_setup_cpuidle_cx'.
So at the end, we could have different cpus with one cpu with less C-states than the other, if I understood correctly ACPIspec30.pdf => page 262 :)
In conclusion, we should keep this variable filled from there and keep this in mind in cpuidle.c in order to not break acpi in the future. Maybe a comment in cpuidle.c would help ... especially with a single C-state registration.
Sure, adding a comment in there sounds like a good idea.
Thanks, Rafael
This patch adds a pointer to the cpuidle_state array in the cpuidle_device structure. When the cpuidle_device is initialized, the pointer is assigned from the driver's cpuidle states array.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com --- drivers/cpuidle/cpuidle.c | 4 +++- include/linux/cpuidle.h | 1 + 2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index e28f6ea..ef0e936 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -317,8 +317,10 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return 0; if (!drv || !cpuidle_curr_governor) return -EIO; - if (!dev->state_count) + if (!dev->state_count) { dev->state_count = drv->state_count; + dev->states = drv->states; + }
if (dev->registered == 0) { ret = __cpuidle_register_device(dev); diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 279b1ea..5cf18b5 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -96,6 +96,7 @@ struct cpuidle_device {
int last_residency; int state_count; + struct cpuidle_state *states; struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX]; struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
We have the cpuidle states pointer stored in the cpuidle device structure. This patch use this pointer instead of the driver's one.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com --- drivers/cpuidle/cpuidle.c | 17 +++++++++-------- drivers/cpuidle/governors/ladder.c | 22 +++++++++++----------- drivers/cpuidle/governors/menu.c | 8 ++++---- drivers/cpuidle/sysfs.c | 3 +-- 4 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ef0e936..062f54e 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -45,7 +45,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev); static inline int cpuidle_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct cpuidle_state *target_state = &drv->states[index]; + struct cpuidle_state *target_state = &dev->states[index]; return target_state->enter(dev, drv, index); }
@@ -76,8 +76,8 @@ int cpuidle_play_dead(void) return -ENODEV;
/* Find lowest-power state that supports long-term idle */ - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { - struct cpuidle_state *s = &drv->states[i]; + for (i = CPUIDLE_DRIVER_STATE_START; i < dev->state_count; i++) { + struct cpuidle_state *s = &dev->states[i];
if (s->power_usage < power_usage && s->enter_dead) { power_usage = s->power_usage; @@ -86,7 +86,7 @@ int cpuidle_play_dead(void) }
if (dead_state != -1) - return drv->states[dead_state].enter_dead(dev, dead_state); + return dev->states[dead_state].enter_dead(dev, dead_state);
return -ENODEV; } @@ -281,9 +281,9 @@ static int poll_idle(struct cpuidle_device *dev, return index; }
-static void poll_idle_init(struct cpuidle_driver *drv) +static void poll_idle_init(struct cpuidle_device *dev) { - struct cpuidle_state *state = &drv->states[0]; + struct cpuidle_state *state = &dev->states[0];
snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); @@ -295,7 +295,7 @@ static void poll_idle_init(struct cpuidle_driver *drv) state->disabled = false; } #else -static void poll_idle_init(struct cpuidle_driver *drv) {} +static void poll_idle_init(struct cpuidle_device *dev) {} #endif /* CONFIG_ARCH_HAS_CPU_RELAX */
/** @@ -317,6 +317,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return 0; if (!drv || !cpuidle_curr_governor) return -EIO; + if (!dev->state_count) { dev->state_count = drv->state_count; dev->states = drv->states; @@ -331,7 +332,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) cpuidle_enter_ops = drv->en_core_tk_irqen ? cpuidle_enter_tk : cpuidle_enter;
- poll_idle_init(drv); + poll_idle_init(dev);
if ((ret = cpuidle_add_state_sysfs(dev))) return ret; diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c index 9b78405..0783f84 100644 --- a/drivers/cpuidle/governors/ladder.c +++ b/drivers/cpuidle/governors/ladder.c @@ -79,19 +79,19 @@ static int ladder_select_state(struct cpuidle_driver *drv,
last_state = &ldev->states[last_idx];
- if (drv->states[last_idx].flags & CPUIDLE_FLAG_TIME_VALID) { + if (dev->states[last_idx].flags & CPUIDLE_FLAG_TIME_VALID) { last_residency = cpuidle_get_last_residency(dev) - \ - drv->states[last_idx].exit_latency; + dev->states[last_idx].exit_latency; } else last_residency = last_state->threshold.promotion_time + 1;
/* consider promotion */ - if (last_idx < drv->state_count - 1 && - !drv->states[last_idx + 1].disabled && + if (last_idx < dev->state_count - 1 && + !dev->states[last_idx + 1].disabled && !dev->states_usage[last_idx + 1].disable && last_residency > last_state->threshold.promotion_time && - drv->states[last_idx + 1].exit_latency <= latency_req) { + dev->states[last_idx + 1].exit_latency <= latency_req) { last_state->stats.promotion_count++; last_state->stats.demotion_count = 0; if (last_state->stats.promotion_count >= last_state->threshold.promotion_count) { @@ -102,13 +102,13 @@ static int ladder_select_state(struct cpuidle_driver *drv,
/* consider demotion */ if (last_idx > CPUIDLE_DRIVER_STATE_START && - (drv->states[last_idx].disabled || + (dev->states[last_idx].disabled || dev->states_usage[last_idx].disable || - drv->states[last_idx].exit_latency > latency_req)) { + dev->states[last_idx].exit_latency > latency_req)) { int i;
for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) { - if (drv->states[i].exit_latency <= latency_req) + if (dev->states[i].exit_latency <= latency_req) break; } ladder_do_selection(ldev, last_idx, i); @@ -144,8 +144,8 @@ static int ladder_enable_device(struct cpuidle_driver *drv,
ldev->last_state_idx = CPUIDLE_DRIVER_STATE_START;
- for (i = 0; i < drv->state_count; i++) { - state = &drv->states[i]; + for (i = 0; i < dev->state_count; i++) { + state = &dev->states[i]; lstate = &ldev->states[i];
lstate->stats.promotion_count = 0; @@ -154,7 +154,7 @@ static int ladder_enable_device(struct cpuidle_driver *drv, lstate->threshold.promotion_count = PROMOTION_COUNT; lstate->threshold.demotion_count = DEMOTION_COUNT;
- if (i < drv->state_count - 1) + if (i < dev->state_count - 1) lstate->threshold.promotion_time = state->exit_latency; if (i > 0) lstate->threshold.demotion_time = state->exit_latency; diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index 5b1f2c3..3a9a9bd 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) * unless the timer is happening really really soon. */ if (data->expected_us > 5 && - !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && + !dev->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
@@ -289,8 +289,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) * Find the idle state with the lowest power while satisfying * our constraints. */ - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { - struct cpuidle_state *s = &drv->states[i]; + for (i = CPUIDLE_DRIVER_STATE_START; i < dev->state_count; i++) { + struct cpuidle_state *s = &dev->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i];
if (s->disabled || su->disable) @@ -338,7 +338,7 @@ static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev) struct menu_device *data = &__get_cpu_var(menu_devices); int last_idx = data->last_state_idx; unsigned int last_idle_us = cpuidle_get_last_residency(dev); - struct cpuidle_state *target = &drv->states[last_idx]; + struct cpuidle_state *target = &dev->states[last_idx]; unsigned int measured_us; u64 new_factor;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 5f809e3..6b20929 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -363,14 +363,13 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) { int i, ret = -ENOMEM; struct cpuidle_state_kobj *kobj; - struct cpuidle_driver *drv = cpuidle_get_driver();
/* state statistics */ for (i = 0; i < device->state_count; i++) { kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL); if (!kobj) goto error_state; - kobj->state = &drv->states[i]; + kobj->state = &device->states[i]; kobj->state_usage = &device->states_usage[i]; init_completion(&kobj->kobj_unregister);
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Deepthi Dharwar deepthi@linux.vnet.ibm.com Acked-by: Peter De Schrijver pdeschrijver@nvidia.com Tested-by: Peter De Schrijver pdeschrijver@nvidia.com --- drivers/cpuidle/cpuidle.c | 21 +++++++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 062f54e..ebb10c9 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -487,6 +487,27 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
+int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ + if (!dev || !states) + return -EINVAL; + + if (state_count <= 0) + return -EINVAL; + + cpuidle_pause_and_lock(); + + dev->states = states; + dev->state_count = state_count; + + cpuidle_resume_and_unlock(); + + return 0; +} +EXPORT_SYMBOL_GPL(cpuidle_register_states); + #ifdef CONFIG_SMP
static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 5cf18b5..85cabfd 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -151,7 +151,9 @@ extern void cpuidle_driver_unref(void); extern void cpuidle_unregister_driver(struct cpuidle_driver *drv); extern int cpuidle_register_device(struct cpuidle_device *dev); extern void cpuidle_unregister_device(struct cpuidle_device *dev); - +extern int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count); extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -163,7 +165,6 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, int (*enter)(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void); - #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -176,7 +177,10 @@ static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { } static inline int cpuidle_register_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_unregister_device(struct cpuidle_device *dev) { } - +static inline int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ return -ENODEV; } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
On Fri, Sep 7, 2012 at 3:49 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
Feel free to add my Reviewed-by for this series.
/Amit
On 09/07/2012 01:04 PM, Amit Kucheria wrote:
On Fri, Sep 7, 2012 at 3:49 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
Feel free to add my Reviewed-by for this series.
Thanks Amit for reviewing the patches.
-- Daniel
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
Thanks, Rafael
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ? We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
eg.
int cpuidle_register_driver(struct cpuidle_driver *drv) { if (!drv || !drv->state_count) return -EINVAL;
if (cpuidle_disabled()) return -ENODEV;
spin_lock(&cpuidle_driver_lock);
if (cpuidle_curr_driver) { spin_unlock(&cpuidle_driver_lock); return -EBUSY; }
__cpuidle_register_driver(drv); cpuidle_curr_driver = drv; spin_unlock(&cpuidle_driver_lock);
return 0; }
cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
Thanks, Rafael
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
OK, but what if there are two (or more) sets of cores, where all cores in one set are identical, but two cores from different sets differ?
In that case it would be good to have one array of states per set, but the patch doesn't seem to do that, does it?
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
Well that's required. :-)
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ?
Uniform handling of all the CPUs of the same kind without data duplication and less code complexity, I think.
We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
I don't seem to understand how things are supposed to work, then.
What _exactly_ do you mean by "the bL switcher", for instance?
Rafael
On 09/17/2012 10:50 PM, Rafael J. Wysocki wrote:
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
OK, but what if there are two (or more) sets of cores, where all cores in one set are identical, but two cores from different sets differ?
A second array is defined and registered for these cores with the cpuidle_register_states function.
Let's pick an example with the big.LITTLE architecture.
There are two A7 and two A15, resulting in the code on 4 cpuidle_device structure (eg. dev_A7_1, dev_A7_2, dev_A15_1, dev_A15_2). Then the driver registers a different cpu states array for the A7s and the A15s
At the end,
dev_A7_1->states points to the array states 1 dev_A7_2->states points to the array states 1
dev_A15_1->states points to the array states 2 dev_A15_2->states points to the array states 2
It is similar with Tegra3.
I think Peter and Lorenzo already wrote a driver based on this approach. Peter, Lorenzo any comments ?
The single registration mechanism introduced by Deepthi is kept and we have a way to specify different idle states for different cpus.
In that case it would be good to have one array of states per set, but the patch doesn't seem to do that, does it?
Yes, this is what does the patch.
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
Well that's required. :-)
Yes :)
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ?
Uniform handling of all the CPUs of the same kind without data duplication and less code complexity, I think.
We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
I don't seem to understand how things are supposed to work, then.
Sorry, I did not suggest that. I am wondering how several cpuidle drivers can co-exist together in the state of the code. Maybe I misunderstood your idea.
The patchset I sent is pretty simple and do not duplicate the array states.
That would be nice if Len could react to this patchset (4/6,5/6, and 6/6). Cc'ing him to its intel address.
What _exactly_ do you mean by "the bL switcher", for instance?
The switcher is in charge of migrating tasks from the A7 to A15 (and vice versa) depending on the system load and make the one cluster up and visible while the other is not visible [1].
[1] www.arm.com/files/downloads/big.LITTLE_Final.pdf
On Mon, Sep 17, 2012 at 10:35:00PM +0100, Daniel Lezcano wrote:
On 09/17/2012 10:50 PM, Rafael J. Wysocki wrote:
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
[...]
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
OK, but what if there are two (or more) sets of cores, where all cores in one set are identical, but two cores from different sets differ?
A second array is defined and registered for these cores with the cpuidle_register_states function.
Let's pick an example with the big.LITTLE architecture.
There are two A7 and two A15, resulting in the code on 4 cpuidle_device structure (eg. dev_A7_1, dev_A7_2, dev_A15_1, dev_A15_2). Then the driver registers a different cpu states array for the A7s and the A15s
At the end,
dev_A7_1->states points to the array states 1 dev_A7_2->states points to the array states 1
dev_A15_1->states points to the array states 2 dev_A15_2->states points to the array states 2
It is similar with Tegra3.
I think Peter and Lorenzo already wrote a driver based on this approach. Peter, Lorenzo any comments ?
Well, I guess the question is about *where* the array of states should belong. With the current approach we end up having an array of states in the cpuidle_driver, but also array(s) of states in platform code and we override the newly added pointer in cpuidle_device to point to those array(s) for CPUs whose idle states differ from the ones registered (and copied) in the driver.
Data is NOT duplicated but now I understand Rafael's remark.
If we have a driver per-[set of cpus] (that is impossible with the current framework as you higlighted) code would be cleaner since all idle states data would live in cpuidle_driver(s), not possibly in platform code. Then the problem becomes: what cpuidle_driver should be used and how to choose that at run-time within the governors.
The single registration mechanism introduced by Deepthi is kept and we have a way to specify different idle states for different cpus.
In that case it would be good to have one array of states per set, but the patch doesn't seem to do that, does it?
Yes, this is what does the patch.
The patch adds a pointer to idle states in cpuidle_device, the platform driver defines the array(s).
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
Well that's required. :-)
Yes :)
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ?
Uniform handling of all the CPUs of the same kind without data duplication and less code complexity, I think.
We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
I don't seem to understand how things are supposed to work, then.
Sorry, I did not suggest that. I am wondering how several cpuidle drivers can co-exist together in the state of the code. Maybe I misunderstood your idea.
The patchset I sent is pretty simple and do not duplicate the array states.
That would be nice if Len could react to this patchset (4/6,5/6, and 6/6). Cc'ing him to its intel address.
As the idle infrastructure stands I do not see how multiple cpuidle drivers can co-exist, that's the problem, and Daniel already mentioned that.
Lorenzo
On Tuesday, September 18, 2012, Lorenzo Pieralisi wrote:
On Mon, Sep 17, 2012 at 10:35:00PM +0100, Daniel Lezcano wrote:
On 09/17/2012 10:50 PM, Rafael J. Wysocki wrote:
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
[...]
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
OK, but what if there are two (or more) sets of cores, where all cores in one set are identical, but two cores from different sets differ?
A second array is defined and registered for these cores with the cpuidle_register_states function.
Let's pick an example with the big.LITTLE architecture.
There are two A7 and two A15, resulting in the code on 4 cpuidle_device structure (eg. dev_A7_1, dev_A7_2, dev_A15_1, dev_A15_2). Then the driver registers a different cpu states array for the A7s and the A15s
At the end,
dev_A7_1->states points to the array states 1 dev_A7_2->states points to the array states 1
dev_A15_1->states points to the array states 2 dev_A15_2->states points to the array states 2
It is similar with Tegra3.
I think Peter and Lorenzo already wrote a driver based on this approach. Peter, Lorenzo any comments ?
Well, I guess the question is about *where* the array of states should belong. With the current approach we end up having an array of states in the cpuidle_driver, but also array(s) of states in platform code and we override the newly added pointer in cpuidle_device to point to those array(s) for CPUs whose idle states differ from the ones registered (and copied) in the driver.
Data is NOT duplicated but now I understand Rafael's remark.
If we have a driver per-[set of cpus] (that is impossible with the current framework as you higlighted) code would be cleaner since all idle states data would live in cpuidle_driver(s), not possibly in platform code. Then the problem becomes: what cpuidle_driver should be used and how to choose that at run-time within the governors.
For that to work, the cpuidle core would have to be modified so that it didn't make the "there may be only on driver in the system" assumption and there would have to be a way to associate the given CPU core with the matching driver.
Thanks, Rafael
It is similar with Tegra3.
In our case CPU0 has different latencies for 1 C state compared to the other CPUs
I think Peter and Lorenzo already wrote a driver based on this approach. Peter, Lorenzo any comments ?
Yes. My implementation doesn't provide a state table in the cpuidle device at all. I always use cpuidle_register_states() to register the state tables.
Cheers,
Peter.
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/17/2012 10:50 PM, Rafael J. Wysocki wrote:
On Monday, September 17, 2012, Daniel Lezcano wrote:
On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
OK, but what if there are two (or more) sets of cores, where all cores in one set are identical, but two cores from different sets differ?
A second array is defined and registered for these cores with the cpuidle_register_states function.
Let's pick an example with the big.LITTLE architecture.
There are two A7 and two A15, resulting in the code on 4 cpuidle_device structure (eg. dev_A7_1, dev_A7_2, dev_A15_1, dev_A15_2). Then the driver registers a different cpu states array for the A7s and the A15s
At the end,
dev_A7_1->states points to the array states 1 dev_A7_2->states points to the array states 1
dev_A15_1->states points to the array states 2 dev_A15_2->states points to the array states 2
It is similar with Tegra3.
I think Peter and Lorenzo already wrote a driver based on this approach. Peter, Lorenzo any comments ?
The single registration mechanism introduced by Deepthi is kept and we have a way to specify different idle states for different cpus.
In that case it would be good to have one array of states per set, but the patch doesn't seem to do that, does it?
Yes, this is what does the patch.
OK
Now, if you look at struct cpuidle_driver, it is not much more than the array of struct cpuidle_state objects. Yes, there are more fields in there, but they are all secondary.
This means that by adding a new array of states you effectively add a different cpuidle driver for those CPU cores.
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
Well that's required. :-)
Yes :)
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ?
Uniform handling of all the CPUs of the same kind without data duplication and less code complexity, I think.
We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
I don't seem to understand how things are supposed to work, then.
Sorry, I did not suggest that.
No, but that's what happened, actually. :-)
I didn't realize that you wanted to address the "bL switcher" use case and now the changes make more sense to me than before, but still I'd prefer this to be done a bit differently.
I am wondering how several cpuidle drivers can co-exist together in the state of the code. Maybe I misunderstood your idea.
Well, we have the assumption that there will be only one cpuidle driver in the system, but I don't think it would be a big deal to change that.
The patchset I sent is pretty simple and do not duplicate the array states.
That would be nice if Len could react to this patchset (4/6,5/6, and 6/6). Cc'ing him to its intel address.
Well, I'm afraid you'll need to deal with me instead. :-)
What _exactly_ do you mean by "the bL switcher", for instance?
The switcher is in charge of migrating tasks from the A7 to A15 (and vice versa) depending on the system load and make the one cluster up and visible while the other is not visible [1].
[1] www.arm.com/files/downloads/big.LITTLE_Final.pdf
Yeah. So for that use case I'd just add a secondary_states pointer to struct cpuidle_driver containing the address of an array of states for "secondary" CPU cores. It probably would need its own counterparts of state_count and safe_state_index.
Next, I'd add a "secondary" flag to struct cpuidle_device which, when set, would indicate that for this particular CPU the core should use the states from the "secondary_states" array.
However, for the use case in which all cores are normally visible to the scheduler, I'd just go for making it possible to use more than one cpuidle driver at a time.
We do that for all other kinds of devices already. Consider Ethernet, for one example. There is no reason to have a single Ethernet driver trying to handle all of the adapters in the system, if they are different. If they are identical, then yes, one driver should handle all of them, but if they are different, we use different drivers.
I don't see why CPU cores should be treated in any special way in that respect (except for the "bL switcher" use case with is kind of unusual).
Thanks, Rafael
-
Amit Kucheria -
Daniel Lezcano -
Lorenzo Pieralisi -
Peter De Schrijver -
Rafael J. Wysocki