[PATCH 0/6] cpuidle : per cpu latencies

[PATCH 0/6] cpuidle : per cpu latencies

[Daniel Lezcano]

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

-- 
1.7.5.4

[PATCH 1/6] acpi : move the acpi_idle_driver variable declaration

[Daniel Lezcano]

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

Re: [PATCH 1/6] acpi : move the acpi_idle_driver variable declaration

[Rafael J. Wysocki]

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

Re: [PATCH 1/6] acpi : move the acpi_idle_driver variable declaration

[Daniel Lezcano]

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


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] acpi : move the acpi_idle_driver variable declaration

[Rafael J. Wysocki]

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

[PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Daniel Lezcano]

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

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Amit Kucheria]

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

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Rafael J. Wysocki]

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

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Rafael J. Wysocki]

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

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Rafael J. Wysocki]

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

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Daniel Lezcano]

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

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] acpi : move cpuidle_device field out of the acpi_processor_power structure

[Rafael J. Wysocki]

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

[PATCH 3/6] acpi : remove pointless cpuidle device state_count init

[Daniel Lezcano]

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

Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init

[Amit Kucheria]

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

Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init

[Rafael J. Wysocki]

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

Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init

[Daniel Lezcano]

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


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] acpi : remove pointless cpuidle device state_count init

[Rafael J. Wysocki]

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

[PATCH 4/6] cpuidle : add a pointer for cpuidle_state in the cpuidle_device

[Daniel Lezcano]

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];
 
-- 
1.7.5.4

[PATCH 5/6] cpuidle : use per cpuidle device cpu states

[Daniel Lezcano]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tested-by: Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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);
 
-- 
1.7.5.4

[PATCH 6/6] cpuidle : add cpuidle_register_states function

[Daniel Lezcano]

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) { }
-- 
1.7.5.4

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Amit Kucheria]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Daniel Lezcano]

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

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Rafael J. Wysocki]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Daniel Lezcano]

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


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Rafael J. Wysocki]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Daniel Lezcano]

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

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Lorenzo Pieralisi]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Rafael J. Wysocki]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Peter De Schrijver]

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

Re: [PATCH 0/6] cpuidle : per cpu latencies

[Rafael J. Wysocki]

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