[PATCH 0/2][V2] cpuidle - remove power from the select equation

[PATCH 0/2][V2] cpuidle - remove power from the select equation

[Daniel Lezcano]

The states could be sorted in the power backward order, we get rid of
some lines of code and by this way that fixes also a bug in the dynamic
c-states.

Changelog:

V2:
 * added the optimization in the select menu governor

Daniel Lezcano (2):
  cpuidle - remove the power_specified field in the driver
  cpuidle - optimize the select function for the 'menu' governor

 drivers/cpuidle/cpuidle.c        |   17 ++++-------------
 drivers/cpuidle/driver.c         |   25 -------------------------
 drivers/cpuidle/governors/menu.c |   20 ++++++++------------
 include/linux/cpuidle.h          |    2 +-
 4 files changed, 13 insertions(+), 51 deletions(-)

-- 
1.7.5.4

[PATCH 0/2][V2] cpuidle - remove power from the select equation

[Daniel Lezcano]

The states could be sorted in the power backward order, we get rid of
some lines of code and by this way that fixes also a bug in the dynamic
c-states.

Changelog:

V2:
 * added the optimization in the select menu governor

Daniel Lezcano (2):
  cpuidle - remove the power_specified field in the driver
  cpuidle - optimize the select function for the 'menu' governor

 drivers/cpuidle/cpuidle.c        |   17 ++++-------------
 drivers/cpuidle/driver.c         |   25 -------------------------
 drivers/cpuidle/governors/menu.c |   20 ++++++++------------
 include/linux/cpuidle.h          |    2 +-
 4 files changed, 13 insertions(+), 51 deletions(-)

-- 
1.7.5.4

[PATCH 1/2][V2] cpuidle - remove the power_specified field in the driver

[Daniel Lezcano]

This patch follows the discussion about reinitializing the power usage
when a C-state is added/removed.

 https://lkml.org/lkml/2012/10/16/518

We realized the power usage field is never filled and when it is
filled for tegra, the power_specified flag is not set making all these
values to be resetted when the driver is initialized with the set_power_state
function.

Julius and I feel this is over-engineered and the power_specified
flag could be simply removed and continue assuming the states are
backward sorted.

The menu governor select function is simplified as the power is ordered.
Actually the condition is always true with the current code.

The cpuidle_play_dead function is also simplified by doing a reverse lookup
on the array.

The set_power_states function is removed as it does no make sense anymore.

As a consequence, this patch fix also the bug where on the dynamic C-state
system, the power fields is not initialized.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Julius Werner <jwerner@chromium.org>
---
 drivers/cpuidle/cpuidle.c        |   17 ++++-------------
 drivers/cpuidle/driver.c         |   25 -------------------------
 drivers/cpuidle/governors/menu.c |    8 ++------
 include/linux/cpuidle.h          |    2 +-
 4 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8df53dd..e1f6860 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -69,24 +69,15 @@ int cpuidle_play_dead(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-	int i, dead_state = -1;
-	int power_usage = -1;
+	int i;
 
 	if (!drv)
 		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];
-
-		if (s->power_usage < power_usage && s->enter_dead) {
-			power_usage = s->power_usage;
-			dead_state = i;
-		}
-	}
-
-	if (dead_state != -1)
-		return drv->states[dead_state].enter_dead(dev, dead_state);
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
+		if (drv->states[i].enter_dead)
+			return drv->states[i].enter_dead(dev, i);
 
 	return -ENODEV;
 }
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 3af841f..bb045f1 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
 static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu);
 static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
 
-static void set_power_states(struct cpuidle_driver *drv)
-{
-	int i;
-
-	/*
-	 * cpuidle driver should set the drv->power_specified bit
-	 * before registering if the driver provides
-	 * power_usage numbers.
-	 *
-	 * If power_specified is not set,
-	 * we fill in power_usage with decreasing values as the
-	 * cpuidle code has an implicit assumption that state Cn
-	 * uses less power than C(n-1).
-	 *
-	 * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
-	 * an power value of -1.  So we use -2, -3, etc, for other
-	 * c-states.
-	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
-		drv->states[i].power_usage = -1 - i;
-}
-
 static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	drv->refcnt = 0;
-
-	if (!drv->power_specified)
-		set_power_states(drv);
 }
 
 static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bd40b94..fe343a0 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
 	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
-	int power_usage = -1;
 	int i;
 	int multiplier;
 	struct timespec t;
@@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
-		if (s->power_usage < power_usage) {
-			power_usage = s->power_usage;
-			data->last_state_idx = i;
-			data->exit_us = s->exit_latency;
-		}
+		data->last_state_idx = i;
+		data->exit_us = s->exit_latency;
 	}
 
 	/* not deepest C-state chosen for low predicted residency */
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 3711b34..24cd103 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -126,9 +126,9 @@ struct cpuidle_driver {
 	struct module 		*owner;
 	int                     refcnt;
 
-	unsigned int		power_specified:1;
 	/* set to 1 to use the core cpuidle time keeping (for all states). */
 	unsigned int		en_core_tk_irqen:1;
+	/* states array must be ordered in decreasing power consumption */
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
-- 
1.7.5.4

[PATCH 1/2][V2] cpuidle - remove the power_specified field in the driver

[Daniel Lezcano]

This patch follows the discussion about reinitializing the power usage
when a C-state is added/removed.

 https://lkml.org/lkml/2012/10/16/518

We realized the power usage field is never filled and when it is
filled for tegra, the power_specified flag is not set making all these
values to be resetted when the driver is initialized with the set_power_state
function.

Julius and I feel this is over-engineered and the power_specified
flag could be simply removed and continue assuming the states are
backward sorted.

The menu governor select function is simplified as the power is ordered.
Actually the condition is always true with the current code.

The cpuidle_play_dead function is also simplified by doing a reverse lookup
on the array.

The set_power_states function is removed as it does no make sense anymore.

As a consequence, this patch fix also the bug where on the dynamic C-state
system, the power fields is not initialized.

Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Julius Werner <jwerner-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/cpuidle/cpuidle.c        |   17 ++++-------------
 drivers/cpuidle/driver.c         |   25 -------------------------
 drivers/cpuidle/governors/menu.c |    8 ++------
 include/linux/cpuidle.h          |    2 +-
 4 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8df53dd..e1f6860 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -69,24 +69,15 @@ int cpuidle_play_dead(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-	int i, dead_state = -1;
-	int power_usage = -1;
+	int i;
 
 	if (!drv)
 		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];
-
-		if (s->power_usage < power_usage && s->enter_dead) {
-			power_usage = s->power_usage;
-			dead_state = i;
-		}
-	}
-
-	if (dead_state != -1)
-		return drv->states[dead_state].enter_dead(dev, dead_state);
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
+		if (drv->states[i].enter_dead)
+			return drv->states[i].enter_dead(dev, i);
 
 	return -ENODEV;
 }
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 3af841f..bb045f1 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
 static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu);
 static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
 
-static void set_power_states(struct cpuidle_driver *drv)
-{
-	int i;
-
-	/*
-	 * cpuidle driver should set the drv->power_specified bit
-	 * before registering if the driver provides
-	 * power_usage numbers.
-	 *
-	 * If power_specified is not set,
-	 * we fill in power_usage with decreasing values as the
-	 * cpuidle code has an implicit assumption that state Cn
-	 * uses less power than C(n-1).
-	 *
-	 * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
-	 * an power value of -1.  So we use -2, -3, etc, for other
-	 * c-states.
-	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
-		drv->states[i].power_usage = -1 - i;
-}
-
 static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	drv->refcnt = 0;
-
-	if (!drv->power_specified)
-		set_power_states(drv);
 }
 
 static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bd40b94..fe343a0 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
 	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
-	int power_usage = -1;
 	int i;
 	int multiplier;
 	struct timespec t;
@@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
-		if (s->power_usage < power_usage) {
-			power_usage = s->power_usage;
-			data->last_state_idx = i;
-			data->exit_us = s->exit_latency;
-		}
+		data->last_state_idx = i;
+		data->exit_us = s->exit_latency;
 	}
 
 	/* not deepest C-state chosen for low predicted residency */
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 3711b34..24cd103 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -126,9 +126,9 @@ struct cpuidle_driver {
 	struct module 		*owner;
 	int                     refcnt;
 
-	unsigned int		power_specified:1;
 	/* set to 1 to use the core cpuidle time keeping (for all states). */
 	unsigned int		en_core_tk_irqen:1;
+	/* states array must be ordered in decreasing power consumption */
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
-- 
1.7.5.4

[PATCH 2/2][V2] cpuidle - optimize the select function for the 'menu' governor

[Daniel Lezcano]

As the power is backward sorted in the states array and we are looking for
the state consuming the little power as possible, instead of looking from
the beginning of the array, we look from the end. That should save us some
iterations in the loop each time we select a state at idle time.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/menu.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index fe343a0..05b8998 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -367,24 +367,24 @@ 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++) {
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 
 		if (s->disabled || su->disable)
 			continue;
-		if (s->target_residency > data->predicted_us) {
-			low_predicted = 1;
-			continue;
-		}
 		if (s->exit_latency > latency_req)
 			continue;
+		if (s->target_residency > data->predicted_us)
+			continue;
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
+		low_predicted = i - CPUIDLE_DRIVER_STATE_START;
 		data->last_state_idx = i;
 		data->exit_us = s->exit_latency;
-	}
+		break;
+       }
 
 	/* not deepest C-state chosen for low predicted residency */
 	if (low_predicted) {
-- 
1.7.5.4

[PATCH 2/2][V2] cpuidle - optimize the select function for the 'menu' governor

[Daniel Lezcano]

As the power is backward sorted in the states array and we are looking for
the state consuming the little power as possible, instead of looking from
the beginning of the array, we look from the end. That should save us some
iterations in the loop each time we select a state at idle time.

Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpuidle/governors/menu.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index fe343a0..05b8998 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -367,24 +367,24 @@ 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++) {
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 
 		if (s->disabled || su->disable)
 			continue;
-		if (s->target_residency > data->predicted_us) {
-			low_predicted = 1;
-			continue;
-		}
 		if (s->exit_latency > latency_req)
 			continue;
+		if (s->target_residency > data->predicted_us)
+			continue;
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
+		low_predicted = i - CPUIDLE_DRIVER_STATE_START;
 		data->last_state_idx = i;
 		data->exit_us = s->exit_latency;
-	}
+		break;
+       }
 
 	/* not deepest C-state chosen for low predicted residency */
 	if (low_predicted) {
-- 
1.7.5.4

Re: [PATCH 2/2][V2] cpuidle - optimize the select function for the 'menu' governor

[Francesco Lavra]

Hi Daniel,

On 12/14/2012 02:57 PM, Daniel Lezcano wrote:
> As the power is backward sorted in the states array and we are looking for
> the state consuming the little power as possible, instead of looking from
> the beginning of the array, we look from the end. That should save us some
> iterations in the loop each time we select a state at idle time.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/governors/menu.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index fe343a0..05b8998 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -367,24 +367,24 @@ 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++) {
> +	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) {
>  		struct cpuidle_state *s = &drv->states[i];
>  		struct cpuidle_state_usage *su = &dev->states_usage[i];
>  
>  		if (s->disabled || su->disable)
>  			continue;
> -		if (s->target_residency > data->predicted_us) {
> -			low_predicted = 1;
> -			continue;
> -		}
>  		if (s->exit_latency > latency_req)
>  			continue;
> +		if (s->target_residency > data->predicted_us)
> +			continue;
>  		if (s->exit_latency * multiplier > data->predicted_us)
>  			continue;
>  
> +		low_predicted = i - CPUIDLE_DRIVER_STATE_START;

You are altering the semantics of low_predicted, which is supposed to be
non-zero when the deepest C-state has not been chosen because its target
residency is more than the predicted residency.
It seems to me that the if block where target_residency is compared with
the predicted residency should be left as is.

>  		data->last_state_idx = i;
>  		data->exit_us = s->exit_latency;
> -	}
> +		break;
> +       }
>  
>  	/* not deepest C-state chosen for low predicted residency */
>  	if (low_predicted) {

--
Francesco

Re: [PATCH 2/2][V2] cpuidle - optimize the select function for the 'menu' governor

[Julius Werner]

*Ping!*

Happy New Year everyone. Is there any update on this? I think Francesco already pointed out how to solve the last remaining issue with this, so I hope we can now resubmit these patches and finally get them merged. Daniel?