[PATCH 00/21] CPUIdle: Minor cleanups for 3.13

[PATCH 00/21] CPUIdle: Minor cleanups for 3.13

[Viresh Kumar]

Hi Rafael/Daniel,

This is a small cleanup patchset for CPUIdle which can go in 3.13 if it looks
okay to you guys..

Mostly trivial patches but few are doing good/significant changes. Tested on my
thinkpad with suspend/resume and didn't found any broken stuff with it.

I a not very sure about this patch (As I don't know about all aspects of CPUIdle
framework):
  cpuidle: don't call poll_idle_init() for every cpu

--
viresh

Viresh Kumar (21):
  cpuidle: fix indentation of cpumask
  cpuidle: Fix comments in cpuidle core
  cpuidle: make __cpuidle_get_cpu_driver() inline
  cpuidle: make __cpuidle_device_init() return void
  cpuidle: make __cpuidle_driver_init() return void
  cpuidle: rearrange code in __cpuidle_driver_init()
  cpuidle: rearrange __cpuidle_register_device() to keep minimal exit
    points
  cpuidle: use cpuidle_disabled() instead of "off"
  cpuidle: merge two if() statements for checking error cases
  cpuidle: reduce code duplication inside cpuidle_idle_call()
  cpuidle: replace multiline statements with single line in
    cpuidle_idle_call()
  cpuidle: call cpuidle_get_driver() from after taking
    cpuidle_driver_lock
  cpuidle: use drv instead of cpuidle_driver in show_current_driver()
  cpuidle: coupled: don't compare cpu masks unnecessarily
  cpuidle: free all state kobjects from cpuidle_free_state_kobj()
  cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj
  cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_driver_kobj
  cpuidle: don't call poll_idle_init() for every cpu
  cpuidle: create list of registered drivers
  cpuidle: don't calculate time-diff if entered_state == 0
  cpuidle: change governor from within cpuidle_replace_governor()

 drivers/cpuidle/coupled.c  |   9 +--
 drivers/cpuidle/cpuidle.c  |  95 +++++++------------------
 drivers/cpuidle/driver.c   | 171 ++++++++++++++++++++-------------------------
 drivers/cpuidle/governor.c |  24 +++----
 drivers/cpuidle/sysfs.c    |  74 +++++++-------------
 include/linux/cpuidle.h    |  25 +++++--
 6 files changed, 161 insertions(+), 237 deletions(-)

-- 
1.7.12.rc2.18.g61b472e

[PATCH 01/21] cpuidle: fix indentation of cpumask

[Viresh Kumar]

cpumask is indented using spaces instead of tabs. Fix it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/cpuidle.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 781addc..c082425 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -114,7 +114,7 @@ struct cpuidle_driver {
 	int			safe_state_index;
 
 	/* the driver handles the cpus in cpumask */
-	struct cpumask       *cpumask;
+	struct cpumask		*cpumask;
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.12.rc2.18.g61b472e

[PATCH 02/21] cpuidle: Fix comments in cpuidle core

[Viresh Kumar]

Few comments in cpuidle core files have trivial mistakes. This patch fixes them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/coupled.c | 2 +-
 drivers/cpuidle/cpuidle.c | 2 +-
 drivers/cpuidle/driver.c  | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index f8a8636..e952936 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -147,7 +147,7 @@ static cpumask_t cpuidle_coupled_poked;
  * has returned from this function, the barrier is immediately available for
  * reuse.
  *
- * The atomic variable a must be initialized to 0 before any cpu calls
+ * The atomic variable must be initialized to 0 before any cpu calls
  * this function, will be reset to 0 before any cpu returns from this function.
  *
  * Must only be called from within a coupled idle state handler
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index d75040d..8827c02 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -516,7 +516,7 @@ int cpuidle_register(struct cpuidle_driver *drv,
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
 		/*
-		 * On multiplatform for ARM, the coupled idle states could
+		 * On multiplatform for ARM, the coupled idle states could be
 		 * enabled in the kernel even if the cpuidle driver does not
 		 * use it. Note, coupled_cpus is a struct copy.
 		 */
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 6e11701..ced1df6 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -56,7 +56,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 }
 
 /**
- * __cpuidle_set_driver - set per CPU driver variables the the given driver.
+ * __cpuidle_set_driver - set per CPU driver variables for the given driver.
  * @drv: a valid pointer to a struct cpuidle_driver
  *
  * For each CPU in the driver's cpumask, unset the registered driver per CPU
@@ -132,7 +132,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
  * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
  * @arg: a void pointer used to match the SMP cross call API
  *
- * @arg is used as a value of type 'long' with on of the two values:
+ * @arg is used as a value of type 'long' with one of the two values:
  * - CLOCK_EVT_NOTIFY_BROADCAST_ON
  * - CLOCK_EVT_NOTIFY_BROADCAST_OFF
  *
@@ -169,7 +169,7 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 	/*
 	 * Look for the timer stop flag in the different states, so that we know
 	 * if the broadcast timer has to be set up.  The loop is in the reverse
-	 * order, because usually on of the the deeper states has this flag set.
+	 * order, because usually one of the deeper states have this flag set.
 	 */
 	for (i = drv->state_count - 1; i >= 0 ; i--) {
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 03/21] cpuidle: make __cpuidle_get_cpu_driver() inline

[Viresh Kumar]

__cpuidle_get_cpu_driver() is a single line function and so deserves to be
marked inline.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index ced1df6..25455e8 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
  * Returns a pointer to struct cpuidle_driver or NULL if no driver has been
  * registered for @cpu.
  */
-static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
+static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
 {
 	return per_cpu(cpuidle_drivers, cpu);
 }
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 03/21] cpuidle: make __cpuidle_get_cpu_driver() inline

[Daniel Lezcano]

On 09/22/2013 03:20 AM, Viresh Kumar wrote:
> __cpuidle_get_cpu_driver() is a single line function and so deserves to be
> marked inline.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index ced1df6..25455e8 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
>   * Returns a pointer to struct cpuidle_driver or NULL if no driver has been
>   * registered for @cpu.
>   */
> -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>  {
>  	return per_cpu(cpuidle_drivers, cpu);
>  }
> 


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

[PATCH 04/21] cpuidle: make __cpuidle_device_init() return void

[Viresh Kumar]

__cpuidle_device_init() doesn't return anything except zero and so doesn't
really need a return type other than void.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8827c02..211e504 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -358,12 +358,10 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev)
 	module_put(drv->owner);
 }
 
-static int __cpuidle_device_init(struct cpuidle_device *dev)
+static void __cpuidle_device_init(struct cpuidle_device *dev)
 {
 	memset(dev->states_usage, 0, sizeof(dev->states_usage));
 	dev->last_residency = 0;
-
-	return 0;
 }
 
 /**
@@ -410,9 +408,7 @@ int cpuidle_register_device(struct cpuidle_device *dev)
 	if (dev->registered)
 		goto out_unlock;
 
-	ret = __cpuidle_device_init(dev);
-	if (ret)
-		goto out_unlock;
+	__cpuidle_device_init(dev);
 
 	ret = __cpuidle_register_device(dev);
 	if (ret)
-- 
1.7.12.rc2.18.g61b472e

[PATCH 05/21] cpuidle: make __cpuidle_driver_init() return void

[Viresh Kumar]

__cpuidle_driver_init() doesn't return anything except zero and so doesn't
really need a return type other than void.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/driver.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 25455e8..8df66c8 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg)
  *
  * Returns 0 on success, a negative error code otherwise.
  */
-static int __cpuidle_driver_init(struct cpuidle_driver *drv)
+static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	int i;
 
@@ -179,8 +179,6 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 		drv->bctimer = 1;
 		break;
 	}
-
-	return 0;
 }
 
 /**
@@ -206,9 +204,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 	if (cpuidle_disabled())
 		return -ENODEV;
 
-	ret = __cpuidle_driver_init(drv);
-	if (ret)
-		return ret;
+	__cpuidle_driver_init(drv);
 
 	ret = __cpuidle_set_driver(drv);
 	if (ret)
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 05/21] cpuidle: make __cpuidle_driver_init() return void

[Hongbo Zhang]

On 09/22/2013 09:20 AM, Viresh Kumar wrote:
> __cpuidle_driver_init() doesn't return anything except zero and so doesn't
> really need a return type other than void.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpuidle/driver.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 25455e8..8df66c8 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg)
>    *
>    * Returns 0 on success, a negative error code otherwise.
If you want to make it return void, you should also update the above 
comment.
>    */
> -static int __cpuidle_driver_init(struct cpuidle_driver *drv)
> +static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>   {
>   	int i;
>   
> @@ -179,8 +179,6 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>   		drv->bctimer = 1;
>   		break;
>   	}
> -
> -	return 0;
>   }
>   
>   /**
> @@ -206,9 +204,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>   	if (cpuidle_disabled())
>   		return -ENODEV;
>   
> -	ret = __cpuidle_driver_init(drv);
> -	if (ret)
> -		return ret;
> +	__cpuidle_driver_init(drv);
>   
>   	ret = __cpuidle_set_driver(drv);
>   	if (ret)

Re: [PATCH 05/21] cpuidle: make __cpuidle_driver_init() return void

[Viresh Kumar]

On 23 September 2013 15:28, Hongbo Zhang <hongbo.zhang@freescale.com> wrote:
> On 09/22/2013 09:20 AM, Viresh Kumar wrote:
>>
>> __cpuidle_driver_init() doesn't return anything except zero and so doesn't
>> really need a return type other than void.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   drivers/cpuidle/driver.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
>> index 25455e8..8df66c8 100644
>> --- a/drivers/cpuidle/driver.c
>> +++ b/drivers/cpuidle/driver.c
>> @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg)
>>    *
>>    * Returns 0 on success, a negative error code otherwise.
>
> If you want to make it return void, you should also update the above
> comment.

Thanks for the correction..

[PATCH 06/21] cpuidle: rearrange code in __cpuidle_driver_init()

[Viresh Kumar]

This is trivial patch that just reorders few statements in
__cpuidle_driver_init() routine, so that we don't need both 'continue' and
'break' in the for loop. Functionally it shouldn't change anything.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/driver.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 8df66c8..6279e1c 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -172,12 +172,10 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 	 * order, because usually one of the deeper states have this flag set.
 	 */
 	for (i = drv->state_count - 1; i >= 0 ; i--) {
-
-		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
-			continue;
-
-		drv->bctimer = 1;
-		break;
+		if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) {
+			drv->bctimer = 1;
+			break;
+		}
 	}
 }
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 06/21] cpuidle: rearrange code in __cpuidle_driver_init()

[Daniel Lezcano]

On 09/22/2013 03:20 AM, Viresh Kumar wrote:
> This is trivial patch that just reorders few statements in
> __cpuidle_driver_init() routine, so that we don't need both 'continue' and
> 'break' in the for loop. Functionally it shouldn't change anything.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/driver.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 8df66c8..6279e1c 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -172,12 +172,10 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>  	 * order, because usually one of the deeper states have this flag set.
>  	 */
>  	for (i = drv->state_count - 1; i >= 0 ; i--) {
> -
> -		if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))
> -			continue;
> -
> -		drv->bctimer = 1;
> -		break;
> +		if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) {
> +			drv->bctimer = 1;
> +			break;
> +		}
>  	}
>  }

Well, I don't have a strong opinion on that, it is "Schtroumpf Vert et
Vert Schtroumpf" :)  [1]

  -- Daniel

[1] http://en.wikipedia.org/wiki/Schtroumpf_Vert_et_Vert_Schtroumpf


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

Re: [PATCH 06/21] cpuidle: rearrange code in __cpuidle_driver_init()

[Viresh Kumar]

On 26 September 2013 03:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Well, I don't have a strong opinion on that, it is "Schtroumpf Vert et
> Vert Schtroumpf" :)  [1]

:)

[PATCH 07/21] cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points

[Viresh Kumar]

This patch rearranges __cpuidle_register_device() a bit in order to reduce the
number of exit points of this function.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 211e504..8c91bad 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -383,13 +383,12 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 	list_add(&dev->device_list, &cpuidle_detected_devices);
 
 	ret = cpuidle_coupled_register_device(dev);
-	if (ret) {
+	if (ret)
 		__cpuidle_unregister_device(dev);
-		return ret;
-	}
+	else
+		dev->registered = 1;
 
-	dev->registered = 1;
-	return 0;
+	return ret;
 }
 
 /**
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 07/21] cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points

[Daniel Lezcano]

On 09/22/2013 03:20 AM, Viresh Kumar wrote:
> This patch rearranges __cpuidle_register_device() a bit in order to reduce the
> number of exit points of this function.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/cpuidle.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 211e504..8c91bad 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -383,13 +383,12 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
>  	list_add(&dev->device_list, &cpuidle_detected_devices);
>  
>  	ret = cpuidle_coupled_register_device(dev);
> -	if (ret) {
> +	if (ret)
>  		__cpuidle_unregister_device(dev);
> -		return ret;
> -	}
> +	else
> +		dev->registered = 1;
>  
> -	dev->registered = 1;
> -	return 0;
> +	return ret;
>  }

There is no accounting for taste :)

I agree the patch concentrates more the return statement into a single
place which conforms better to the kernel coding style.

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

[PATCH 08/21] cpuidle: use cpuidle_disabled() instead of "off"

[Viresh Kumar]

We have a routine for getting value of "off", better call that instead of using
"off" directly.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8c91bad..aec9029 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -119,7 +119,7 @@ int cpuidle_idle_call(void)
 	struct cpuidle_driver *drv;
 	int next_state, entered_state;
 
-	if (off)
+	if (cpuidle_disabled())
 		return -ENODEV;
 
 	if (!initialized)
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 08/21] cpuidle: use cpuidle_disabled() instead of "off"

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> We have a routine for getting value of "off", better call that instead of using
> "off" directly.

We are in the fast path, I am not sure invoking a function here is
better than using directly the static variable.

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/cpuidle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 8c91bad..aec9029 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -119,7 +119,7 @@ int cpuidle_idle_call(void)
>  	struct cpuidle_driver *drv;
>  	int next_state, entered_state;
>  
> -	if (off)
> +	if (cpuidle_disabled())
>  		return -ENODEV;
>  
>  	if (!initialized)
> 


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

Re: [PATCH 08/21] cpuidle: use cpuidle_disabled() instead of "off"

[Viresh Kumar]

On 26 September 2013 03:22, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>> We have a routine for getting value of "off", better call that instead of using
>> "off" directly.
>
> We are in the fast path, I am not sure invoking a function here is
> better than using directly the static variable.

I only did it for consistency as we have this routine specifically for reading
value of "off" and so we better don't use off directly..

Probably we can make it static inline and move it into
drivers/cpuidle/cpuidle.h?

Re: [PATCH 08/21] cpuidle: use cpuidle_disabled() instead of "off"

[Daniel Lezcano]

On 09/26/2013 07:06 AM, Viresh Kumar wrote:
> On 26 September 2013 03:22, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>>> We have a routine for getting value of "off", better call that instead of using
>>> "off" directly.
>>
>> We are in the fast path, I am not sure invoking a function here is
>> better than using directly the static variable.
>
> I only did it for consistency as we have this routine specifically for reading
> value of "off" and so we better don't use off directly..
>
> Probably we can make it static inline and move it into
> drivers/cpuidle/cpuidle.h?

If you move it to cpuidle.h, you will have to move the 'off' variable in 
the header hence increasing the scope of it.



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

[PATCH 09/21] cpuidle: merge two if() statements for checking error cases

[Viresh Kumar]

Both return same error message and so better write them in a single line.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index aec9029..b8c63cb 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -119,10 +119,7 @@ int cpuidle_idle_call(void)
 	struct cpuidle_driver *drv;
 	int next_state, entered_state;
 
-	if (cpuidle_disabled())
-		return -ENODEV;
-
-	if (!initialized)
+	if (cpuidle_disabled() || !initialized)
 		return -ENODEV;
 
 	/* check if the device is ready */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 09/21] cpuidle: merge two if() statements for checking error cases

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Both return same error message and so better write them in a single line.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

modulo the comment on patch 08/21:

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/cpuidle.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index aec9029..b8c63cb 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -119,10 +119,7 @@ int cpuidle_idle_call(void)
>  	struct cpuidle_driver *drv;
>  	int next_state, entered_state;
>  
> -	if (cpuidle_disabled())
> -		return -ENODEV;
> -
> -	if (!initialized)
> +	if (cpuidle_disabled() || !initialized)
>  		return -ENODEV;
>  
>  	/* check if the device is ready */
> 


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

[PATCH 10/21] cpuidle: reduce code duplication inside cpuidle_idle_call()

[Viresh Kumar]

We are doing this twice in cpuidle_idle_call() routine:
	drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP

Would be better if we actually store this in a local variable and use that. That
would remove code duplication as well as make this piece of code run fast (in
case compiler wasn't able to optimize it earlier)

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index b8c63cb..ed67e3c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -118,6 +118,7 @@ int cpuidle_idle_call(void)
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv;
 	int next_state, entered_state;
+	bool broadcast;
 
 	if (cpuidle_disabled() || !initialized)
 		return -ENODEV;
@@ -141,7 +142,9 @@ int cpuidle_idle_call(void)
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
-	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
+	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
+
+	if (broadcast)
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
 				   &dev->cpu);
 
@@ -151,7 +154,7 @@ int cpuidle_idle_call(void)
 	else
 		entered_state = cpuidle_enter_state(dev, drv, next_state);
 
-	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
+	if (broadcast)
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
 				   &dev->cpu);
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 10/21] cpuidle: reduce code duplication inside cpuidle_idle_call()

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> We are doing this twice in cpuidle_idle_call() routine:
> 	drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP
> 
> Would be better if we actually store this in a local variable and use that. That
> would remove code duplication as well as make this piece of code run fast (in
> case compiler wasn't able to optimize it earlier)
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/cpuidle.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index b8c63cb..ed67e3c 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -118,6 +118,7 @@ int cpuidle_idle_call(void)
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>  	struct cpuidle_driver *drv;
>  	int next_state, entered_state;
> +	bool broadcast;
>  
>  	if (cpuidle_disabled() || !initialized)
>  		return -ENODEV;
> @@ -141,7 +142,9 @@ int cpuidle_idle_call(void)
>  
>  	trace_cpu_idle_rcuidle(next_state, dev->cpu);
>  
> -	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
> +	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
> +
> +	if (broadcast)
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
>  				   &dev->cpu);
>  
> @@ -151,7 +154,7 @@ int cpuidle_idle_call(void)
>  	else
>  		entered_state = cpuidle_enter_state(dev, drv, next_state);
>  
> -	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
> +	if (broadcast)
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
>  				   &dev->cpu);
>  
> 


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

[PATCH 11/21] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Viresh Kumar]

Few statements in cpuidle_idle_call() are broken into multiple lines, whereas
they can actually come in a single line. Convert those to single line.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index ed67e3c..43d5836 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,8 +145,7 @@ int cpuidle_idle_call(void)
 	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
 
 	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
-				   &dev->cpu);
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
 
 	if (cpuidle_state_is_coupled(dev, drv, next_state))
 		entered_state = cpuidle_enter_state_coupled(dev, drv,
@@ -155,8 +154,7 @@ int cpuidle_idle_call(void)
 		entered_state = cpuidle_enter_state(dev, drv, next_state);
 
 	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
-				   &dev->cpu);
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
 
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 11/21] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Few statements in cpuidle_idle_call() are broken into multiple lines, whereas
> they can actually come in a single line. Convert those to single line.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I splitted these lines because they have 81 cols.

> ---
>  drivers/cpuidle/cpuidle.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index ed67e3c..43d5836 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -145,8 +145,7 @@ int cpuidle_idle_call(void)
>  	broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
>  
>  	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> -				   &dev->cpu);
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>  
>  	if (cpuidle_state_is_coupled(dev, drv, next_state))
>  		entered_state = cpuidle_enter_state_coupled(dev, drv,
> @@ -155,8 +154,7 @@ int cpuidle_idle_call(void)
>  		entered_state = cpuidle_enter_state(dev, drv, next_state);
>  
>  	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
> -				   &dev->cpu);
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>  
>  	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>  
> 


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

Re: [PATCH 11/21] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Viresh Kumar]

On 26 September 2013 03:33, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> I splitted these lines because they have 81 cols.

>> -             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
>> -                                &dev->cpu);
>> +             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);

This one comes in 80 columns

>> -             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
>> -                                &dev->cpu);
>> +             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);

And this one in 79..

And so I did this change.. I have checked it again now..

Re: [PATCH 11/21] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Daniel Lezcano]

On 09/26/2013 07:51 AM, Viresh Kumar wrote:
> On 26 September 2013 03:33, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> I splitted these lines because they have 81 cols.
> 
>>> -             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
>>> -                                &dev->cpu);
>>> +             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> 
> This one comes in 80 columns
> 
>>> -             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
>>> -                                &dev->cpu);
>>> +             clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> 
> And this one in 79..
> 
> And so I did this change.. I have checked it again now..

Well I have 81 with Vi and 80 with Emacs :)

May be some tabs converted to spaces ...

Anyway if it fits 80 cols then it is ok.

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

Re: [PATCH 11/21] cpuidle: replace multiline statements with single line in cpuidle_idle_call()

[Viresh Kumar]

On 26 September 2013 13:25, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Well I have 81 with Vi and 80 with Emacs :)

Hmm.. vim gave me 80 :) .. I am looking at bottom right corner where it gives:
148, 66-80 22%

[PATCH 12/21] cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock

[Viresh Kumar]

cpuidle_driver_lock is taken correctly at most of the places but at few places
calls to cpuidle_get_driver() are done from outside of this lock.

Fix them by calling cpuidle_get_driver() after taking cpuidle_driver_lock.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/driver.c | 3 ++-
 drivers/cpuidle/sysfs.c  | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 6279e1c..7b2510a 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -340,10 +340,11 @@ struct cpuidle_driver *cpuidle_driver_ref(void)
  */
 void cpuidle_driver_unref(void)
 {
-	struct cpuidle_driver *drv = cpuidle_get_driver();
+	struct cpuidle_driver *drv;
 
 	spin_lock(&cpuidle_driver_lock);
 
+	drv = cpuidle_get_driver();
 	if (drv && !WARN_ON(drv->refcnt <= 0))
 		drv->refcnt--;
 
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 8739cc0..a022393 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -52,9 +52,10 @@ static ssize_t show_current_driver(struct device *dev,
 				   char *buf)
 {
 	ssize_t ret;
-	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
+	struct cpuidle_driver *cpuidle_driver;
 
 	spin_lock(&cpuidle_driver_lock);
+	cpuidle_driver = cpuidle_get_driver();
 	if (cpuidle_driver)
 		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
 	else
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 12/21] cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> cpuidle_driver_lock is taken correctly at most of the places but at few places
> calls to cpuidle_get_driver() are done from outside of this lock.
> 
> Fix them by calling cpuidle_get_driver() after taking cpuidle_driver_lock.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/driver.c | 3 ++-
>  drivers/cpuidle/sysfs.c  | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 6279e1c..7b2510a 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -340,10 +340,11 @@ struct cpuidle_driver *cpuidle_driver_ref(void)
>   */
>  void cpuidle_driver_unref(void)
>  {
> -	struct cpuidle_driver *drv = cpuidle_get_driver();
> +	struct cpuidle_driver *drv;
>  
>  	spin_lock(&cpuidle_driver_lock);
>  
> +	drv = cpuidle_get_driver();
>  	if (drv && !WARN_ON(drv->refcnt <= 0))
>  		drv->refcnt--;
>  
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 8739cc0..a022393 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -52,9 +52,10 @@ static ssize_t show_current_driver(struct device *dev,
>  				   char *buf)
>  {
>  	ssize_t ret;
> -	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
> +	struct cpuidle_driver *cpuidle_driver;
>  
>  	spin_lock(&cpuidle_driver_lock);
> +	cpuidle_driver = cpuidle_get_driver();
>  	if (cpuidle_driver)
>  		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
>  	else
> 


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

[PATCH 13/21] cpuidle: use drv instead of cpuidle_driver in show_current_driver()

[Viresh Kumar]

Instances of "struct cpuidle_driver *" are consistently named as "drv" in
cpuidle core. Its broken only at one place: show_current_driver().

Fix it for consistency.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index a022393..e918b6d 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -52,12 +52,12 @@ static ssize_t show_current_driver(struct device *dev,
 				   char *buf)
 {
 	ssize_t ret;
-	struct cpuidle_driver *cpuidle_driver;
+	struct cpuidle_driver *drv;
 
 	spin_lock(&cpuidle_driver_lock);
-	cpuidle_driver = cpuidle_get_driver();
-	if (cpuidle_driver)
-		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
+	drv = cpuidle_get_driver();
+	if (drv)
+		ret = sprintf(buf, "%s\n", drv->name);
 	else
 		ret = sprintf(buf, "none\n");
 	spin_unlock(&cpuidle_driver_lock);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 13/21] cpuidle: use drv instead of cpuidle_driver in show_current_driver()

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Instances of "struct cpuidle_driver *" are consistently named as "drv" in
> cpuidle core. Its broken only at one place: show_current_driver().
> 
> Fix it for consistency.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/sysfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index a022393..e918b6d 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -52,12 +52,12 @@ static ssize_t show_current_driver(struct device *dev,
>  				   char *buf)
>  {
>  	ssize_t ret;
> -	struct cpuidle_driver *cpuidle_driver;
> +	struct cpuidle_driver *drv;
>  
>  	spin_lock(&cpuidle_driver_lock);
> -	cpuidle_driver = cpuidle_get_driver();
> -	if (cpuidle_driver)
> -		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
> +	drv = cpuidle_get_driver();
> +	if (drv)
> +		ret = sprintf(buf, "%s\n", drv->name);
>  	else
>  		ret = sprintf(buf, "none\n");
>  	spin_unlock(&cpuidle_driver_lock);
> 


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

[PATCH 14/21] cpuidle: coupled: don't compare cpu masks unnecessarily

[Viresh Kumar]

In cpuidle_coupled_register_device() we do following:
	if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
		coupled->prevent++;

This is only required to be done when we are using 'coupled' from an existing
cpuidle_device and not when we have just done this:

	coupled->coupled_cpus = dev->coupled_cpus

So, move this compare statement to the right place.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/coupled.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index e952936..19a89eb 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
 		other_dev = per_cpu(cpuidle_devices, cpu);
 		if (other_dev && other_dev->coupled) {
 			coupled = other_dev->coupled;
+
+			if (WARN_ON(!cpumask_equal(&dev->coupled_cpus,
+						&coupled->coupled_cpus)))
+				coupled->prevent++;
 			goto have_coupled;
 		}
 	}
@@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
 
 have_coupled:
 	dev->coupled = coupled;
-	if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
-		coupled->prevent++;
-
 	cpuidle_coupled_update_online_cpus(coupled);
 
 	coupled->refcnt++;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 14/21] cpuidle: coupled: don't compare cpu masks unnecessarily

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> In cpuidle_coupled_register_device() we do following:
> 	if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
> 		coupled->prevent++;
> 
> This is only required to be done when we are using 'coupled' from an existing
> cpuidle_device and not when we have just done this:
> 
> 	coupled->coupled_cpus = dev->coupled_cpus
> 
> So, move this compare statement to the right place.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Cc'ed Colin Cross.

> ---
>  drivers/cpuidle/coupled.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index e952936..19a89eb 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
>  		other_dev = per_cpu(cpuidle_devices, cpu);
>  		if (other_dev && other_dev->coupled) {
>  			coupled = other_dev->coupled;
> +
> +			if (WARN_ON(!cpumask_equal(&dev->coupled_cpus,
> +						&coupled->coupled_cpus)))
> +				coupled->prevent++;
>  			goto have_coupled;
>  		}
>  	}
> @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
>  
>  have_coupled:
>  	dev->coupled = coupled;
> -	if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
> -		coupled->prevent++;
> -
>  	cpuidle_coupled_update_online_cpus(coupled);
>  
>  	coupled->refcnt++;
> 


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

Re: [PATCH 14/21] cpuidle: coupled: don't compare cpu masks unnecessarily

[Colin Cross]

On Sat, Sep 21, 2013 at 6:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> In cpuidle_coupled_register_device() we do following:
>         if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
>                 coupled->prevent++;
>
> This is only required to be done when we are using 'coupled' from an existing
> cpuidle_device and not when we have just done this:
>
>         coupled->coupled_cpus = dev->coupled_cpus
>
> So, move this compare statement to the right place.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I don't agree with this.  This patch is a tiny optimization in code
that is rarely called, and it moves a final sanity check somewhere
that it might get missed if the code were later refactored.

> ---
>  drivers/cpuidle/coupled.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index e952936..19a89eb 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
>                 other_dev = per_cpu(cpuidle_devices, cpu);
>                 if (other_dev && other_dev->coupled) {
>                         coupled = other_dev->coupled;
> +
> +                       if (WARN_ON(!cpumask_equal(&dev->coupled_cpus,
> +                                               &coupled->coupled_cpus)))
> +                               coupled->prevent++;
>                         goto have_coupled;
>                 }
>         }
> @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
>
>  have_coupled:
>         dev->coupled = coupled;
> -       if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
> -               coupled->prevent++;
> -
>         cpuidle_coupled_update_online_cpus(coupled);
>
>         coupled->refcnt++;
> --
> 1.7.12.rc2.18.g61b472e
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 14/21] cpuidle: coupled: don't compare cpu masks unnecessarily

[Viresh Kumar]

On 26 September 2013 05:55, Colin Cross <ccross@google.com> wrote:
> I don't agree with this.  This patch is a tiny optimization in code
> that is rarely called, and it moves a final sanity check somewhere
> that it might get missed if the code were later refactored.

This is what we are doing for the first cpu of coupled-cpus:
if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus)))
    coupled->prevent++;

i.e. comparing a variable to itself :)

And I believe my patch puts the sanity check at the right place (where
we are using coupled from existing CPUs.. And that is where it should
have been since the beginning)..

If people miss this during code re-factoring, then it would be even more
stupid on the part of Author and Reviewer.. And if it still gets missed
then this is not the only place where we need to worry about such stuff..

This is present everywhere in our code.. You can't really some part of
code to some place and leave the other as-is.. The change is supposed
to be more logical and so funny mistakes must be caught during reviews.

Re: [PATCH 14/21] cpuidle: coupled: don't compare cpu masks unnecessarily

[Colin Cross]

On Thu, Sep 26, 2013 at 1:36 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26 September 2013 05:55, Colin Cross <ccross@google.com> wrote:
>> I don't agree with this.  This patch is a tiny optimization in code
>> that is rarely called, and it moves a final sanity check somewhere
>> that it might get missed if the code were later refactored.
>
> This is what we are doing for the first cpu of coupled-cpus:
> if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus)))
>     coupled->prevent++;
>
> i.e. comparing a variable to itself :)
>
> And I believe my patch puts the sanity check at the right place (where
> we are using coupled from existing CPUs.. And that is where it should
> have been since the beginning)..
>
> If people miss this during code re-factoring, then it would be even more
> stupid on the part of Author and Reviewer.. And if it still gets missed
> then this is not the only place where we need to worry about such stuff..
>
> This is present everywhere in our code.. You can't really some part of
> code to some place and leave the other as-is.. The change is supposed
> to be more logical and so funny mistakes must be caught during reviews.

It's fine where it is.  Once dev and coupled are both known,
regardless of how they were found, it performs a final sanity check
that nothing went wrong.  Moving into a specific branch of the finding
code defeats the purpose.  Yes, it performs a useless check on the
first cpu, but it keeps the code readable and maintainable, so it
stays where it is.  NAK.

[PATCH 15/21] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Viresh Kumar]

Loop for states is currently present on callers side and so is replicated at
several places. It would be better to move that inside cpuidle_free_state_kobj()
instead.

This patch does it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/sysfs.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index e918b6d..ade31a9 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = {
 	.release = cpuidle_state_sysfs_release,
 };
 
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i)
+static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
+		int count)
 {
-	kobject_put(&device->kobjs[i]->kobj);
-	wait_for_completion(&device->kobjs[i]->kobj_unregister);
-	kfree(device->kobjs[i]);
-	device->kobjs[i] = NULL;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		kobject_put(&device->kobjs[i]->kobj);
+		wait_for_completion(&device->kobjs[i]->kobj_unregister);
+		kfree(device->kobjs[i]);
+		device->kobjs[i] = NULL;
+	}
 }
 
 /**
@@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
 	return 0;
 
 error_state:
-	for (i = i - 1; i >= 0; i--)
-		cpuidle_free_state_kobj(device, i);
+	cpuidle_free_state_kobj(device, i);
 	return ret;
 }
 
@@ -430,10 +434,7 @@ error_state:
  */
 static void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
 {
-	int i;
-
-	for (i = 0; i < device->state_count; i++)
-		cpuidle_free_state_kobj(device, i);
+	cpuidle_free_state_kobj(device, device->state_count);
 }
 
 #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 15/21] cpuidle: free all state kobjects from cpuidle_free_state_kobj()

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Loop for states is currently present on callers side and so is replicated at
> several places. It would be better to move that inside cpuidle_free_state_kobj()
> instead.
> 
> This patch does it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  drivers/cpuidle/sysfs.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index e918b6d..ade31a9 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = {
>  	.release = cpuidle_state_sysfs_release,
>  };
>  
> -static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i)
> +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
> +		int count)
>  {
> -	kobject_put(&device->kobjs[i]->kobj);
> -	wait_for_completion(&device->kobjs[i]->kobj_unregister);
> -	kfree(device->kobjs[i]);
> -	device->kobjs[i] = NULL;
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		kobject_put(&device->kobjs[i]->kobj);
> +		wait_for_completion(&device->kobjs[i]->kobj_unregister);
> +		kfree(device->kobjs[i]);
> +		device->kobjs[i] = NULL;
> +	}
>  }
>  
>  /**
> @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
>  	return 0;
>  
>  error_state:
> -	for (i = i - 1; i >= 0; i--)
> -		cpuidle_free_state_kobj(device, i);
> +	cpuidle_free_state_kobj(device, i);
>  	return ret;
>  }
>  
> @@ -430,10 +434,7 @@ error_state:
>   */
>  static void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
>  {
> -	int i;
> -
> -	for (i = 0; i < device->state_count; i++)
> -		cpuidle_free_state_kobj(device, i);
> +	cpuidle_free_state_kobj(device, device->state_count);
>  }
>  
>  #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> 


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

[PATCH 16/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj

[Viresh Kumar]

We always need to allocate struct cpuidle_device_kobj for all CPUs and so there
is no real need to have a pointer to it inside struct cpuidle_device.

This patch makes a object instance of struct cpuidle_device_kobj inside struct
cpuidle_device instead of a pointer.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/sysfs.c | 24 +++++-------------------
 include/linux/cpuidle.h | 11 +++++++++--
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index ade31a9..db0aac3 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -172,12 +172,6 @@ struct cpuidle_attr {
 
 #define attr_to_cpuidleattr(a) container_of(a, struct cpuidle_attr, attr)
 
-struct cpuidle_device_kobj {
-	struct cpuidle_device *dev;
-	struct completion kobj_unregister;
-	struct kobject kobj;
-};
-
 static inline struct cpuidle_device *to_cpuidle_device(struct kobject *kobj)
 {
 	struct cpuidle_device_kobj *kdev =
@@ -399,7 +393,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device)
 {
 	int i, ret = -ENOMEM;
 	struct cpuidle_state_kobj *kobj;
-	struct cpuidle_device_kobj *kdev = device->kobj_dev;
+	struct cpuidle_device_kobj *kdev = &device->kobj_dev;
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device);
 
 	/* state statistics */
@@ -525,7 +519,7 @@ static struct kobj_type ktype_driver_cpuidle = {
 static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
 {
 	struct cpuidle_driver_kobj *kdrv;
-	struct cpuidle_device_kobj *kdev = dev->kobj_dev;
+	struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int ret;
 
@@ -606,24 +600,17 @@ void cpuidle_remove_device_sysfs(struct cpuidle_device *device)
  */
 int cpuidle_add_sysfs(struct cpuidle_device *dev)
 {
-	struct cpuidle_device_kobj *kdev;
+	struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
 	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
 	int error;
 
-	kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
-	if (!kdev)
-		return -ENOMEM;
 	kdev->dev = dev;
-	dev->kobj_dev = kdev;
-
 	init_completion(&kdev->kobj_unregister);
 
 	error = kobject_init_and_add(&kdev->kobj, &ktype_cpuidle, &cpu_dev->kobj,
 				   "cpuidle");
-	if (error) {
-		kfree(kdev);
+	if (error)
 		return error;
-	}
 
 	kobject_uevent(&kdev->kobj, KOBJ_ADD);
 
@@ -636,9 +623,8 @@ int cpuidle_add_sysfs(struct cpuidle_device *dev)
  */
 void cpuidle_remove_sysfs(struct cpuidle_device *dev)
 {
-	struct cpuidle_device_kobj *kdev = dev->kobj_dev;
+	struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
 
 	kobject_put(&kdev->kobj);
 	wait_for_completion(&kdev->kobj_unregister);
-	kfree(kdev);
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index c082425..1fc5cb5 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -11,6 +11,8 @@
 #ifndef _LINUX_CPUIDLE_H
 #define _LINUX_CPUIDLE_H
 
+#include <linux/completion.h>
+#include <linux/kobject.h>
 #include <linux/percpu.h>
 #include <linux/list.h>
 #include <linux/hrtimer.h>
@@ -59,10 +61,15 @@ struct cpuidle_state {
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
-struct cpuidle_device_kobj;
 struct cpuidle_state_kobj;
 struct cpuidle_driver_kobj;
 
+struct cpuidle_device_kobj {
+	struct cpuidle_device *dev;
+	struct completion kobj_unregister;
+	struct kobject kobj;
+};
+
 struct cpuidle_device {
 	unsigned int		registered:1;
 	unsigned int		enabled:1;
@@ -73,7 +80,7 @@ struct cpuidle_device {
 	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];
 	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
 	struct cpuidle_driver_kobj *kobj_driver;
-	struct cpuidle_device_kobj *kobj_dev;
+	struct cpuidle_device_kobj kobj_dev;
 	struct list_head 	device_list;
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 16/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> We always need to allocate struct cpuidle_device_kobj for all CPUs and so there
> is no real need to have a pointer to it inside struct cpuidle_device.
> 
> This patch makes a object instance of struct cpuidle_device_kobj inside struct
> cpuidle_device instead of a pointer.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

nack, it was made in purpose for kobject_init_and_add.

see commit 728ce22b696f9f1404a74d7b2279a65933553a1b



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

Re: [PATCH 16/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj

[Viresh Kumar]

On 26 September 2013 03:42, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>> We always need to allocate struct cpuidle_device_kobj for all CPUs and so there
>> is no real need to have a pointer to it inside struct cpuidle_device.
>>
>> This patch makes a object instance of struct cpuidle_device_kobj inside struct
>> cpuidle_device instead of a pointer.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> nack, it was made in purpose for kobject_init_and_add.
>
> see commit 728ce22b696f9f1404a74d7b2279a65933553a1b

Ahh.. sorry for missing that one :(

Now that I understand why it was introduced, I am thinking if
we can make hotplug path a bit fast? By not freeing sysfs stuff
and only hiding it for time being? And so we wouldn't be required
to do unnecessary initialisations while coming back?

Would that be worth it?

Re: [PATCH 16/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj

[Daniel Lezcano]

On 09/26/2013 08:05 AM, Viresh Kumar wrote:
> On 26 September 2013 03:42, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>>> We always need to allocate struct cpuidle_device_kobj for all CPUs and so there
>>> is no real need to have a pointer to it inside struct cpuidle_device.
>>>
>>> This patch makes a object instance of struct cpuidle_device_kobj inside struct
>>> cpuidle_device instead of a pointer.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> nack, it was made in purpose for kobject_init_and_add.
>>
>> see commit 728ce22b696f9f1404a74d7b2279a65933553a1b
>
> Ahh.. sorry for missing that one :(
>
> Now that I understand why it was introduced, I am thinking if
> we can make hotplug path a bit fast? By not freeing sysfs stuff
> and only hiding it for time being? And so we wouldn't be required
> to do unnecessary initialisations while coming back?
>
> Would that be worth it?

Yes if it possible.


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

[PATCH 17/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_driver_kobj

[Viresh Kumar]

For CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, struct cpuidle_device always needs to
allocate struct cpuidle_driver_kobj for all CPUs and so there is no real need to
have a pointer to it inside struct cpuidle_device.

This patch makes a object instance of struct cpuidle_driver_kobj inside struct
cpuidle_device instead of a pointer.

It also makes kobj_driver dependent on CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/sysfs.c | 20 +++-----------------
 include/linux/cpuidle.h | 11 +++++++++--
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index db0aac3..3386d64 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -439,12 +439,6 @@ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
 	static struct cpuidle_driver_attr attr_driver_##_name = \
 		__ATTR(_name, 0644, show, NULL)
 
-struct cpuidle_driver_kobj {
-	struct cpuidle_driver *drv;
-	struct completion kobj_unregister;
-	struct kobject kobj;
-};
-
 struct cpuidle_driver_attr {
 	struct attribute attr;
 	ssize_t (*show)(struct cpuidle_driver *, char *);
@@ -518,27 +512,20 @@ static struct kobj_type ktype_driver_cpuidle = {
  */
 static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
 {
-	struct cpuidle_driver_kobj *kdrv;
+	struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver;
 	struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int ret;
 
-	kdrv = kzalloc(sizeof(*kdrv), GFP_KERNEL);
-	if (!kdrv)
-		return -ENOMEM;
-
 	kdrv->drv = drv;
 	init_completion(&kdrv->kobj_unregister);
 
 	ret = kobject_init_and_add(&kdrv->kobj, &ktype_driver_cpuidle,
 				   &kdev->kobj, "driver");
-	if (ret) {
-		kfree(kdrv);
+	if (ret)
 		return ret;
-	}
 
 	kobject_uevent(&kdrv->kobj, KOBJ_ADD);
-	dev->kobj_driver = kdrv;
 
 	return ret;
 }
@@ -549,10 +536,9 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
  */
 static void cpuidle_remove_driver_sysfs(struct cpuidle_device *dev)
 {
-	struct cpuidle_driver_kobj *kdrv = dev->kobj_driver;
+	struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver;
 	kobject_put(&kdrv->kobj);
 	wait_for_completion(&kdrv->kobj_unregister);
-	kfree(kdrv);
 }
 #else
 static inline int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1fc5cb5..0f0da17 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -62,7 +62,6 @@ struct cpuidle_state {
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
 struct cpuidle_state_kobj;
-struct cpuidle_driver_kobj;
 
 struct cpuidle_device_kobj {
 	struct cpuidle_device *dev;
@@ -70,6 +69,12 @@ struct cpuidle_device_kobj {
 	struct kobject kobj;
 };
 
+struct cpuidle_driver_kobj {
+	struct cpuidle_driver *drv;
+	struct completion kobj_unregister;
+	struct kobject kobj;
+};
+
 struct cpuidle_device {
 	unsigned int		registered:1;
 	unsigned int		enabled:1;
@@ -79,7 +84,9 @@ struct cpuidle_device {
 	int			state_count;
 	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];
 	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
-	struct cpuidle_driver_kobj *kobj_driver;
+#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
+	struct cpuidle_driver_kobj kobj_driver;
+#endif
 	struct cpuidle_device_kobj kobj_dev;
 	struct list_head 	device_list;
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 17/21] cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_driver_kobj

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> For CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, struct cpuidle_device always needs to
> allocate struct cpuidle_driver_kobj for all CPUs and so there is no real need to
> have a pointer to it inside struct cpuidle_device.
> 
> This patch makes a object instance of struct cpuidle_driver_kobj inside struct
> cpuidle_device instead of a pointer.
> 
> It also makes kobj_driver dependent on CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Comment similar to patch 16/21. If you try to offline/online a cpu, that
should lead to a kernel warning.

> ---
>  drivers/cpuidle/sysfs.c | 20 +++-----------------
>  include/linux/cpuidle.h | 11 +++++++++--
>  2 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index db0aac3..3386d64 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -439,12 +439,6 @@ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device)
>  	static struct cpuidle_driver_attr attr_driver_##_name = \
>  		__ATTR(_name, 0644, show, NULL)
>  
> -struct cpuidle_driver_kobj {
> -	struct cpuidle_driver *drv;
> -	struct completion kobj_unregister;
> -	struct kobject kobj;
> -};
> -
>  struct cpuidle_driver_attr {
>  	struct attribute attr;
>  	ssize_t (*show)(struct cpuidle_driver *, char *);
> @@ -518,27 +512,20 @@ static struct kobj_type ktype_driver_cpuidle = {
>   */
>  static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
>  {
> -	struct cpuidle_driver_kobj *kdrv;
> +	struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver;
>  	struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int ret;
>  
> -	kdrv = kzalloc(sizeof(*kdrv), GFP_KERNEL);
> -	if (!kdrv)
> -		return -ENOMEM;
> -
>  	kdrv->drv = drv;
>  	init_completion(&kdrv->kobj_unregister);
>  
>  	ret = kobject_init_and_add(&kdrv->kobj, &ktype_driver_cpuidle,
>  				   &kdev->kobj, "driver");
> -	if (ret) {
> -		kfree(kdrv);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	kobject_uevent(&kdrv->kobj, KOBJ_ADD);
> -	dev->kobj_driver = kdrv;
>  
>  	return ret;
>  }
> @@ -549,10 +536,9 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
>   */
>  static void cpuidle_remove_driver_sysfs(struct cpuidle_device *dev)
>  {
> -	struct cpuidle_driver_kobj *kdrv = dev->kobj_driver;
> +	struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver;
>  	kobject_put(&kdrv->kobj);
>  	wait_for_completion(&kdrv->kobj_unregister);
> -	kfree(kdrv);
>  }
>  #else
>  static inline int cpuidle_add_driver_sysfs(struct cpuidle_device *dev)
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 1fc5cb5..0f0da17 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -62,7 +62,6 @@ struct cpuidle_state {
>  #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>  
>  struct cpuidle_state_kobj;
> -struct cpuidle_driver_kobj;
>  
>  struct cpuidle_device_kobj {
>  	struct cpuidle_device *dev;
> @@ -70,6 +69,12 @@ struct cpuidle_device_kobj {
>  	struct kobject kobj;
>  };
>  
> +struct cpuidle_driver_kobj {
> +	struct cpuidle_driver *drv;
> +	struct completion kobj_unregister;
> +	struct kobject kobj;
> +};
> +
>  struct cpuidle_device {
>  	unsigned int		registered:1;
>  	unsigned int		enabled:1;
> @@ -79,7 +84,9 @@ struct cpuidle_device {
>  	int			state_count;
>  	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];
>  	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
> -	struct cpuidle_driver_kobj *kobj_driver;
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +	struct cpuidle_driver_kobj kobj_driver;
> +#endif
>  	struct cpuidle_device_kobj kobj_dev;
>  	struct list_head 	device_list;
>  
> 


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

[PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Viresh Kumar]

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 41 -----------------------------------------
 drivers/cpuidle/driver.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 43d5836..bf80236 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -226,45 +226,6 @@ void cpuidle_resume(void)
 	mutex_unlock(&cpuidle_lock);
 }
 
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX
-static int poll_idle(struct cpuidle_device *dev,
-		struct cpuidle_driver *drv, int index)
-{
-	ktime_t	t1, t2;
-	s64 diff;
-
-	t1 = ktime_get();
-	local_irq_enable();
-	while (!need_resched())
-		cpu_relax();
-
-	t2 = ktime_get();
-	diff = ktime_to_us(ktime_sub(t2, t1));
-	if (diff > INT_MAX)
-		diff = INT_MAX;
-
-	dev->last_residency = (int) diff;
-
-	return index;
-}
-
-static void poll_idle_init(struct cpuidle_driver *drv)
-{
-	struct cpuidle_state *state = &drv->states[0];
-
-	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
-	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
-	state->exit_latency = 0;
-	state->target_residency = 0;
-	state->power_usage = -1;
-	state->flags = 0;
-	state->enter = poll_idle;
-	state->disabled = false;
-}
-#else
-static void poll_idle_init(struct cpuidle_driver *drv) {}
-#endif /* CONFIG_ARCH_HAS_CPU_RELAX */
-
 /**
  * cpuidle_enable_device - enables idle PM for a CPU
  * @dev: the CPU
@@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 	if (!dev->state_count)
 		dev->state_count = drv->state_count;
 
-	poll_idle_init(drv);
-
 	ret = cpuidle_add_device_sysfs(dev);
 	if (ret)
 		return ret;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 7b2510a..a4a93b4 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -10,6 +10,7 @@
 
 #include <linux/mutex.h>
 #include <linux/module.h>
+#include <linux/sched.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/clockchips.h>
@@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 	}
 }
 
+#ifdef CONFIG_ARCH_HAS_CPU_RELAX
+static int poll_idle(struct cpuidle_device *dev,
+		struct cpuidle_driver *drv, int index)
+{
+	ktime_t	t1, t2;
+	s64 diff;
+
+	t1 = ktime_get();
+	local_irq_enable();
+	while (!need_resched())
+		cpu_relax();
+
+	t2 = ktime_get();
+	diff = ktime_to_us(ktime_sub(t2, t1));
+	if (diff > INT_MAX)
+		diff = INT_MAX;
+
+	dev->last_residency = (int) diff;
+
+	return index;
+}
+
+static void poll_idle_init(struct cpuidle_driver *drv)
+{
+	struct cpuidle_state *state = &drv->states[0];
+
+	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
+	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
+	state->exit_latency = 0;
+	state->target_residency = 0;
+	state->power_usage = -1;
+	state->flags = 0;
+	state->enter = poll_idle;
+	state->disabled = false;
+}
+#else
+static void poll_idle_init(struct cpuidle_driver *drv) {}
+#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */
+
 /**
  * __cpuidle_register_driver: register the driver
  * @drv: a valid pointer to a struct cpuidle_driver
@@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
 				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
 
+	poll_idle_init(drv);
+
 	return 0;
 }
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

The optimization sounds good but IMHO if we can move this state out of
the cpuidle common framework that would be nicer.

The poll_idle is only applicable for x86 (acpi_driver and intel_idle),
hence I suggest we move this state to these drivers, that will cleanup
the framework code and will remove index shift macro
CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.

> ---
>  drivers/cpuidle/cpuidle.c | 41 -----------------------------------------
>  drivers/cpuidle/driver.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 43d5836..bf80236 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -226,45 +226,6 @@ void cpuidle_resume(void)
>  	mutex_unlock(&cpuidle_lock);
>  }
>  
> -#ifdef CONFIG_ARCH_HAS_CPU_RELAX
> -static int poll_idle(struct cpuidle_device *dev,
> -		struct cpuidle_driver *drv, int index)
> -{
> -	ktime_t	t1, t2;
> -	s64 diff;
> -
> -	t1 = ktime_get();
> -	local_irq_enable();
> -	while (!need_resched())
> -		cpu_relax();
> -
> -	t2 = ktime_get();
> -	diff = ktime_to_us(ktime_sub(t2, t1));
> -	if (diff > INT_MAX)
> -		diff = INT_MAX;
> -
> -	dev->last_residency = (int) diff;
> -
> -	return index;
> -}
> -
> -static void poll_idle_init(struct cpuidle_driver *drv)
> -{
> -	struct cpuidle_state *state = &drv->states[0];
> -
> -	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
> -	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
> -	state->exit_latency = 0;
> -	state->target_residency = 0;
> -	state->power_usage = -1;
> -	state->flags = 0;
> -	state->enter = poll_idle;
> -	state->disabled = false;
> -}
> -#else
> -static void poll_idle_init(struct cpuidle_driver *drv) {}
> -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */
> -
>  /**
>   * cpuidle_enable_device - enables idle PM for a CPU
>   * @dev: the CPU
> @@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
>  	if (!dev->state_count)
>  		dev->state_count = drv->state_count;
>  
> -	poll_idle_init(drv);
> -
>  	ret = cpuidle_add_device_sysfs(dev);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 7b2510a..a4a93b4 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/mutex.h>
>  #include <linux/module.h>
> +#include <linux/sched.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpumask.h>
>  #include <linux/clockchips.h>
> @@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>  	}
>  }
>  
> +#ifdef CONFIG_ARCH_HAS_CPU_RELAX
> +static int poll_idle(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t	t1, t2;
> +	s64 diff;
> +
> +	t1 = ktime_get();
> +	local_irq_enable();
> +	while (!need_resched())
> +		cpu_relax();
> +
> +	t2 = ktime_get();
> +	diff = ktime_to_us(ktime_sub(t2, t1));
> +	if (diff > INT_MAX)
> +		diff = INT_MAX;
> +
> +	dev->last_residency = (int) diff;
> +
> +	return index;
> +}
> +
> +static void poll_idle_init(struct cpuidle_driver *drv)
> +{
> +	struct cpuidle_state *state = &drv->states[0];
> +
> +	snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
> +	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
> +	state->exit_latency = 0;
> +	state->target_residency = 0;
> +	state->power_usage = -1;
> +	state->flags = 0;
> +	state->enter = poll_idle;
> +	state->disabled = false;
> +}
> +#else
> +static void poll_idle_init(struct cpuidle_driver *drv) {}
> +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */
> +
>  /**
>   * __cpuidle_register_driver: register the driver
>   * @drv: a valid pointer to a struct cpuidle_driver
> @@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  		on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer,
>  				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
>  
> +	poll_idle_init(drv);
> +
>  	return 0;
>  }
>  
> 


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

Re: [PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Viresh Kumar]

On 26 September 2013 03:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This deserved a log, sorry for missing that :(

> The optimization sounds good but IMHO if we can move this state out of
> the cpuidle common framework that would be nicer.
>
> The poll_idle is only applicable for x86 (acpi_driver and intel_idle),
> hence I suggest we move this state to these drivers, that will cleanup
> the framework code and will remove index shift macro
> CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.

Lets see what X86 folks have to say about it and then we can do it..
Btw, wouldn't that add some code duplication in those two drivers?

Re: [PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Daniel Lezcano]

On 09/26/2013 08:09 AM, Viresh Kumar wrote:
> On 26 September 2013 03:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> This deserved a log, sorry for missing that :(
>
>> The optimization sounds good but IMHO if we can move this state out of
>> the cpuidle common framework that would be nicer.
>>
>> The poll_idle is only applicable for x86 (acpi_driver and intel_idle),
>> hence I suggest we move this state to these drivers, that will cleanup
>> the framework code and will remove index shift macro
>> CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.
>
> Lets see what X86 folks have to say about it and then we can do it..
> Btw, wouldn't that add some code duplication in those two drivers?

Yes, certainly and that will impact also the menu select governor function:

  ...

         /*
          * We want to default to C1 (hlt), not to busy polling
          * unless the timer is happening really really soon.
          */
         if (data->expected_us > 5 &&
             !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
                 dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
                 data->last_state_idx = CPUIDLE_DRIVER_STATE_START;

         /*
          * 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];
                 struct cpuidle_state_usage *su = &dev->states_usage[i];

                 if (s->disabled || su->disable)
                         continue;
                 if (s->target_residency > data->predicted_us)
                         continue;
                 if (s->exit_latency > latency_req)
                         continue;
                 if (s->exit_latency * multiplier > data->predicted_us)
                         continue;

                 data->last_state_idx = i;
                 data->exit_us = s->exit_latency;
         }

....

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

Re: [PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Viresh Kumar]

On 26 September 2013 13:58, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Yes, certainly and that will impact also the menu select governor function:
>
>  ...
>
>         /*
>          * We want to default to C1 (hlt), not to busy polling
>          * unless the timer is happening really really soon.
>          */
>         if (data->expected_us > 5 &&
>             !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
>                 dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
>                 data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>
>         /*
>          * 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];
>                 struct cpuidle_state_usage *su = &dev->states_usage[i];
>
>                 if (s->disabled || su->disable)
>                         continue;
>                 if (s->target_residency > data->predicted_us)
>                         continue;
>                 if (s->exit_latency > latency_req)
>                         continue;
>                 if (s->exit_latency * multiplier > data->predicted_us)
>                         continue;
>
>                 data->last_state_idx = i;
>                 data->exit_us = s->exit_latency;
>         }

Hmm.. For now I will repost this patch as is and then you can go ahead
for this bigger change.. I wouldn't be able to do this change now, as I
would be rushing for my 2 weeks vacations :)

If this patch looked okay to you, can you please Ack it ?

Re: [PATCH 18/21] cpuidle: don't call poll_idle_init() for every cpu

[Daniel Lezcano]

On 10/03/2013 12:33 PM, Viresh Kumar wrote:
> On 26 September 2013 13:58, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Yes, certainly and that will impact also the menu select governor function:
>>
>>   ...
>>
>>          /*
>>           * We want to default to C1 (hlt), not to busy polling
>>           * unless the timer is happening really really soon.
>>           */
>>          if (data->expected_us > 5 &&
>>              !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
>>                  dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
>>                  data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>>
>>          /*
>>           * 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];
>>                  struct cpuidle_state_usage *su = &dev->states_usage[i];
>>
>>                  if (s->disabled || su->disable)
>>                          continue;
>>                  if (s->target_residency > data->predicted_us)
>>                          continue;
>>                  if (s->exit_latency > latency_req)
>>                          continue;
>>                  if (s->exit_latency * multiplier > data->predicted_us)
>>                          continue;
>>
>>                  data->last_state_idx = i;
>>                  data->exit_us = s->exit_latency;
>>          }
>
> Hmm.. For now I will repost this patch as is and then you can go ahead
> for this bigger change.. I wouldn't be able to do this change now, as I
> would be rushing for my 2 weeks vacations :)
>
> If this patch looked okay to you, can you please Ack it ?

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


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

[PATCH 19/21] cpuidle: create list of registered drivers

[Viresh Kumar]

Currently we have multiple definitions of few routines based on following config
option: CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.

These are present to save space by not creating per-cpu variable for platforms
which need only one cpuidle driver to be registered for all CPUs.

But this setup has a problem. For ARM multi-platform kernel use case this option
will get enabled and so we will have per-cpu variables even for platforms that
don't need it.

The bigger problem is two separate code paths for such platforms for single &
multi platform kernels. Which doesn't sound good.

A better way of solving this problem would be to create cpuidle driver's list
that can be used to manage all information we need. Then we don't really have to
write any special code for handling platforms with
CONFIG_CPU_IDLE_MULTIPLE_DRIVERS option set.

This patch does it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/driver.c | 106 ++++++++++++-----------------------------------
 include/linux/cpuidle.h  |   1 +
 2 files changed, 27 insertions(+), 80 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index a4a93b4..320b4ec 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -18,10 +18,19 @@
 #include "cpuidle.h"
 
 DEFINE_SPINLOCK(cpuidle_driver_lock);
+static LIST_HEAD(cpuidle_detected_drivers);
 
-#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
+static inline struct cpuidle_driver *
+__cpuidle_get_driver(const struct cpumask *cpumask)
+{
+	struct cpuidle_driver *drv;
+
+	list_for_each_entry(drv, &cpuidle_detected_drivers, driver_list)
+		if (cpumask_intersects(drv->cpumask, cpumask))
+			return drv;
 
-static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
+	return NULL;
+}
 
 /**
  * __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
@@ -32,103 +41,39 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
  */
 static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
 {
-	return per_cpu(cpuidle_drivers, cpu);
+	return __cpuidle_get_driver(cpumask_of(cpu));
 }
 
 /**
- * __cpuidle_unset_driver - unset per CPU driver variables.
+ * __cpuidle_add_driver - adds a cpuidle driver to list.
  * @drv: a valid pointer to a struct cpuidle_driver
  *
- * For each CPU in the driver's CPU mask, unset the registered driver per CPU
- * variable. If @drv is different from the registered driver, the corresponding
- * variable is not cleared.
- */
-static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
-{
-	int cpu;
-
-	for_each_cpu(cpu, drv->cpumask) {
-
-		if (drv != __cpuidle_get_cpu_driver(cpu))
-			continue;
-
-		per_cpu(cpuidle_drivers, cpu) = NULL;
-	}
-}
-
-/**
- * __cpuidle_set_driver - set per CPU driver variables for the given driver.
- * @drv: a valid pointer to a struct cpuidle_driver
- *
- * For each CPU in the driver's cpumask, unset the registered driver per CPU
- * to @drv.
+ * Adds cpuidle driver to cpuidle_detected_drivers list if no driver is already
+ * registered for any CPUs present in drv->cpumask.
  *
  * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
  */
-static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
-{
-	int cpu;
-
-	for_each_cpu(cpu, drv->cpumask) {
-
-		if (__cpuidle_get_cpu_driver(cpu)) {
-			__cpuidle_unset_driver(drv);
-			return -EBUSY;
-		}
-
-		per_cpu(cpuidle_drivers, cpu) = drv;
-	}
-
-	return 0;
-}
-
-#else
-
-static struct cpuidle_driver *cpuidle_curr_driver;
-
-/**
- * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
- * @cpu: ignored without the multiple driver support
- *
- * Return a pointer to a struct cpuidle_driver object or NULL if no driver was
- * previously registered.
- */
-static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
-{
-	return cpuidle_curr_driver;
-}
-
-/**
- * __cpuidle_set_driver - assign the global cpuidle driver variable.
- * @drv: pointer to a struct cpuidle_driver object
- *
- * Returns 0 on success, -EBUSY if the driver is already registered.
- */
-static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
+static inline int __cpuidle_add_driver(struct cpuidle_driver *drv)
 {
-	if (cpuidle_curr_driver)
+	if (__cpuidle_get_driver(drv->cpumask))
 		return -EBUSY;
 
-	cpuidle_curr_driver = drv;
+	list_add(&drv->driver_list, &cpuidle_detected_drivers);
 
 	return 0;
 }
 
 /**
- * __cpuidle_unset_driver - unset the global cpuidle driver variable.
- * @drv: a pointer to a struct cpuidle_driver
+ * __cpuidle_remove_driver - remove cpuidle driver from list.
+ * @drv: a valid pointer to a struct cpuidle_driver
  *
- * Reset the global cpuidle variable to NULL.  If @drv does not match the
- * registered driver, do nothing.
+ * Removes cpuidle driver from cpuidle_detected_drivers list.
  */
-static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
+static inline void __cpuidle_remove_driver(struct cpuidle_driver *drv)
 {
-	if (drv == cpuidle_curr_driver)
-		cpuidle_curr_driver = NULL;
+	list_del(&drv->driver_list);
 }
 
-#endif
-
 /**
  * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
  * @arg: a void pointer used to match the SMP cross call API
@@ -158,6 +103,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 	int i;
 
 	drv->refcnt = 0;
+	INIT_LIST_HEAD(&drv->driver_list);
 
 	/*
 	 * Use all possible CPUs as the default, because if the kernel boots
@@ -244,7 +190,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 
 	__cpuidle_driver_init(drv);
 
-	ret = __cpuidle_set_driver(drv);
+	ret = __cpuidle_add_driver(drv);
 	if (ret)
 		return ret;
 
@@ -277,7 +223,7 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
 				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
 	}
 
-	__cpuidle_unset_driver(drv);
+	__cpuidle_remove_driver(drv);
 }
 
 /**
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 0f0da17..81b74d2 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -129,6 +129,7 @@ struct cpuidle_driver {
 
 	/* the driver handles the cpus in cpumask */
 	struct cpumask		*cpumask;
+	struct list_head	driver_list;
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> Currently we have multiple definitions of few routines based on following config
> option: CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
> 
> These are present to save space by not creating per-cpu variable for platforms
> which need only one cpuidle driver to be registered for all CPUs.
> 
> But this setup has a problem. For ARM multi-platform kernel use case this option
> will get enabled and so we will have per-cpu variables even for platforms that
> don't need it.
> 
> The bigger problem is two separate code paths for such platforms for single &
> multi platform kernels. Which doesn't sound good.
> 
> A better way of solving this problem would be to create cpuidle driver's list
> that can be used to manage all information we need. Then we don't really have to
> write any special code for handling platforms with
> CONFIG_CPU_IDLE_MULTIPLE_DRIVERS option set.
> 
> This patch does it.

If you introduce a list, you will have to introduce a lock to protect
it. This lock will be in the fast path cpuidle_idle_call with the
get_driver function and conforming to the comment: "NOTE: no locks or
semaphores should be used here".

A lock has been introduced in this function already and the system hangs
with 1024 cpus.

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/driver.c | 106 ++++++++++++-----------------------------------
>  include/linux/cpuidle.h  |   1 +
>  2 files changed, 27 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index a4a93b4..320b4ec 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -18,10 +18,19 @@
>  #include "cpuidle.h"
>  
>  DEFINE_SPINLOCK(cpuidle_driver_lock);
> +static LIST_HEAD(cpuidle_detected_drivers);
>  
> -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +static inline struct cpuidle_driver *
> +__cpuidle_get_driver(const struct cpumask *cpumask)
> +{
> +	struct cpuidle_driver *drv;
> +
> +	list_for_each_entry(drv, &cpuidle_detected_drivers, driver_list)
> +		if (cpumask_intersects(drv->cpumask, cpumask))
> +			return drv;
>  
> -static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
> +	return NULL;
> +}
>  
>  /**
>   * __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
> @@ -32,103 +41,39 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
>   */
>  static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>  {
> -	return per_cpu(cpuidle_drivers, cpu);
> +	return __cpuidle_get_driver(cpumask_of(cpu));
>  }
>  
>  /**
> - * __cpuidle_unset_driver - unset per CPU driver variables.
> + * __cpuidle_add_driver - adds a cpuidle driver to list.
>   * @drv: a valid pointer to a struct cpuidle_driver
>   *
> - * For each CPU in the driver's CPU mask, unset the registered driver per CPU
> - * variable. If @drv is different from the registered driver, the corresponding
> - * variable is not cleared.
> - */
> -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
> -{
> -	int cpu;
> -
> -	for_each_cpu(cpu, drv->cpumask) {
> -
> -		if (drv != __cpuidle_get_cpu_driver(cpu))
> -			continue;
> -
> -		per_cpu(cpuidle_drivers, cpu) = NULL;
> -	}
> -}
> -
> -/**
> - * __cpuidle_set_driver - set per CPU driver variables for the given driver.
> - * @drv: a valid pointer to a struct cpuidle_driver
> - *
> - * For each CPU in the driver's cpumask, unset the registered driver per CPU
> - * to @drv.
> + * Adds cpuidle driver to cpuidle_detected_drivers list if no driver is already
> + * registered for any CPUs present in drv->cpumask.
>   *
>   * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
>   */
> -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> -{
> -	int cpu;
> -
> -	for_each_cpu(cpu, drv->cpumask) {
> -
> -		if (__cpuidle_get_cpu_driver(cpu)) {
> -			__cpuidle_unset_driver(drv);
> -			return -EBUSY;
> -		}
> -
> -		per_cpu(cpuidle_drivers, cpu) = drv;
> -	}
> -
> -	return 0;
> -}
> -
> -#else
> -
> -static struct cpuidle_driver *cpuidle_curr_driver;
> -
> -/**
> - * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
> - * @cpu: ignored without the multiple driver support
> - *
> - * Return a pointer to a struct cpuidle_driver object or NULL if no driver was
> - * previously registered.
> - */
> -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> -{
> -	return cpuidle_curr_driver;
> -}
> -
> -/**
> - * __cpuidle_set_driver - assign the global cpuidle driver variable.
> - * @drv: pointer to a struct cpuidle_driver object
> - *
> - * Returns 0 on success, -EBUSY if the driver is already registered.
> - */
> -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> +static inline int __cpuidle_add_driver(struct cpuidle_driver *drv)
>  {
> -	if (cpuidle_curr_driver)
> +	if (__cpuidle_get_driver(drv->cpumask))
>  		return -EBUSY;
>  
> -	cpuidle_curr_driver = drv;
> +	list_add(&drv->driver_list, &cpuidle_detected_drivers);
>  
>  	return 0;
>  }
>  
>  /**
> - * __cpuidle_unset_driver - unset the global cpuidle driver variable.
> - * @drv: a pointer to a struct cpuidle_driver
> + * __cpuidle_remove_driver - remove cpuidle driver from list.
> + * @drv: a valid pointer to a struct cpuidle_driver
>   *
> - * Reset the global cpuidle variable to NULL.  If @drv does not match the
> - * registered driver, do nothing.
> + * Removes cpuidle driver from cpuidle_detected_drivers list.
>   */
> -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
> +static inline void __cpuidle_remove_driver(struct cpuidle_driver *drv)
>  {
> -	if (drv == cpuidle_curr_driver)
> -		cpuidle_curr_driver = NULL;
> +	list_del(&drv->driver_list);
>  }
>  
> -#endif
> -
>  /**
>   * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
>   * @arg: a void pointer used to match the SMP cross call API
> @@ -158,6 +103,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>  	int i;
>  
>  	drv->refcnt = 0;
> +	INIT_LIST_HEAD(&drv->driver_list);
>  
>  	/*
>  	 * Use all possible CPUs as the default, because if the kernel boots
> @@ -244,7 +190,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  
>  	__cpuidle_driver_init(drv);
>  
> -	ret = __cpuidle_set_driver(drv);
> +	ret = __cpuidle_add_driver(drv);
>  	if (ret)
>  		return ret;
>  
> @@ -277,7 +223,7 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  				 (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
>  	}
>  
> -	__cpuidle_unset_driver(drv);
> +	__cpuidle_remove_driver(drv);
>  }
>  
>  /**
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 0f0da17..81b74d2 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -129,6 +129,7 @@ struct cpuidle_driver {
>  
>  	/* the driver handles the cpus in cpumask */
>  	struct cpumask		*cpumask;
> +	struct list_head	driver_list;
>  };
>  
>  #ifdef CONFIG_CPU_IDLE
> 


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

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Viresh Kumar]

On 26 September 2013 04:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> If you introduce a list, you will have to introduce a lock to protect
> it.

I missed it, should have added that :)

> This lock will be in the fast path cpuidle_idle_call with the
> get_driver function and conforming to the comment: "NOTE: no locks or
> semaphores should be used here".
>
> A lock has been introduced in this function already and the system hangs
> with 1024 cpus.

Hmm... I see.. I didn't knew about this expectation.. What about a rcu
read/write lock? As far as I know its too lightweight... Can we have that
in fast path?

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Daniel Lezcano]

On 09/26/2013 08:17 AM, Viresh Kumar wrote:
> On 26 September 2013 04:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> If you introduce a list, you will have to introduce a lock to protect
>> it.
>
> I missed it, should have added that :)
>
>> This lock will be in the fast path cpuidle_idle_call with the
>> get_driver function and conforming to the comment: "NOTE: no locks or
>> semaphores should be used here".
>>
>> A lock has been introduced in this function already and the system hangs
>> with 1024 cpus.
>
> Hmm... I see.. I didn't knew about this expectation.. What about a rcu
> read/write lock? As far as I know its too lightweight... Can we have that
> in fast path?

Nope, we can't use rcu in the idle path :)

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.html


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

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Paul E. McKenney]

On Thu, Sep 26, 2013 at 10:19:14AM +0200, Daniel Lezcano wrote:
> On 09/26/2013 08:17 AM, Viresh Kumar wrote:
> >On 26 September 2013 04:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>If you introduce a list, you will have to introduce a lock to protect
> >>it.
> >
> >I missed it, should have added that :)
> >
> >>This lock will be in the fast path cpuidle_idle_call with the
> >>get_driver function and conforming to the comment: "NOTE: no locks or
> >>semaphores should be used here".
> >>
> >>A lock has been introduced in this function already and the system hangs
> >>with 1024 cpus.
> >
> >Hmm... I see.. I didn't knew about this expectation.. What about a rcu
> >read/write lock? As far as I know its too lightweight... Can we have that
> >in fast path?
> 
> Nope, we can't use rcu in the idle path :)
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.html

But you should be able to use SRCU in the idle path, if that helps.

							Thanx, Paul

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Daniel Lezcano]

On 09/28/2013 11:33 PM, Paul E. McKenney wrote:
> On Thu, Sep 26, 2013 at 10:19:14AM +0200, Daniel Lezcano wrote:
>> On 09/26/2013 08:17 AM, Viresh Kumar wrote:
>>> On 26 September 2013 04:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>> If you introduce a list, you will have to introduce a lock to protect
>>>> it.
>>>
>>> I missed it, should have added that :)
>>>
>>>> This lock will be in the fast path cpuidle_idle_call with the
>>>> get_driver function and conforming to the comment: "NOTE: no locks or
>>>> semaphores should be used here".
>>>>
>>>> A lock has been introduced in this function already and the system hangs
>>>> with 1024 cpus.
>>>
>>> Hmm... I see.. I didn't knew about this expectation.. What about a rcu
>>> read/write lock? As far as I know its too lightweight... Can we have that
>>> in fast path?
>>
>> Nope, we can't use rcu in the idle path :)
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.html
>
> But you should be able to use SRCU in the idle path, if that helps.

Interesting, thanks for the pointer.

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

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Viresh Kumar]

On 1 October 2013 00:07, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Interesting, thanks for the pointer.

So, should I keep this patch with SRCU?

Re: [PATCH 19/21] cpuidle: create list of registered drivers

[Daniel Lezcano]

On 10/03/2013 06:38 AM, Viresh Kumar wrote:
> On 1 October 2013 00:07, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Interesting, thanks for the pointer.
>
> So, should I keep this patch with SRCU?

IMHO, we should, for now, keep the code as it is and then focus on the 
lock/refcount for drivers in a separate series.


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

[PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Viresh Kumar]

If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we
will be setting it to zero without using its new value.

And so move calculation of diff also inside the "if" statement.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index bf80236..cb81689 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 
 	struct cpuidle_state *target_state = &drv->states[index];
 	ktime_t time_start, time_end;
-	s64 diff;
 
 	time_start = ktime_get();
 
 	entered_state = target_state->enter(dev, drv, index);
 
-	time_end = ktime_get();
+	if (entered_state >= 0) {
+		s64 diff;
 
-	local_irq_enable();
+		time_end = ktime_get();
+		diff = ktime_to_us(ktime_sub(time_end, time_start));
 
-	diff = ktime_to_us(ktime_sub(time_end, time_start));
-	if (diff > INT_MAX)
-		diff = INT_MAX;
+		if (diff > INT_MAX)
+			diff = INT_MAX;
 
-	dev->last_residency = (int) diff;
+		dev->last_residency = (int) diff;
 
-	if (entered_state >= 0) {
 		/* Update cpuidle counters */
 		/* This can be moved to within driver enter routine
 		 * but that results in multiple copies of same code.
@@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		dev->last_residency = 0;
 	}
 
+	local_irq_enable();
+
 	return entered_state;
 }
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we
> will be setting it to zero without using its new value.

I don't get it, can you elaborate. We can be a long time in this state
(eg. if the prediction is false).

> And so move calculation of diff also inside the "if" statement.
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/cpuidle.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index bf80236..cb81689 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  
>  	struct cpuidle_state *target_state = &drv->states[index];
>  	ktime_t time_start, time_end;
> -	s64 diff;
>  
>  	time_start = ktime_get();
>  
>  	entered_state = target_state->enter(dev, drv, index);
>  
> -	time_end = ktime_get();
> +	if (entered_state >= 0) {
> +		s64 diff;
>  
> -	local_irq_enable();
> +		time_end = ktime_get();
> +		diff = ktime_to_us(ktime_sub(time_end, time_start));
>  
> -	diff = ktime_to_us(ktime_sub(time_end, time_start));
> -	if (diff > INT_MAX)
> -		diff = INT_MAX;
> +		if (diff > INT_MAX)
> +			diff = INT_MAX;
>  
> -	dev->last_residency = (int) diff;
> +		dev->last_residency = (int) diff;
>  
> -	if (entered_state >= 0) {
>  		/* Update cpuidle counters */
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
> @@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		dev->last_residency = 0;
>  	}
>  
> +	local_irq_enable();
> +
>  	return entered_state;
>  }
>  
> 


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

Re: [PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Viresh Kumar]

On 26 September 2013 04:08, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>> If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we
>> will be setting it to zero without using its new value.
>
> I don't get it, can you elaborate.

Sure.. We have following code in cpuidle_enter_state():

---------
        diff = ktime_to_us(ktime_sub(time_end, time_start));
        if (diff > INT_MAX)
                diff = INT_MAX;

        dev->last_residency = (int) diff;

        if (entered_state >= 0) {
                /* Update cpuidle counters */
                /* This can be moved to within driver enter routine
                 * but that results in multiple copies of same code.
                 */
                dev->states_usage[entered_state].time += dev->last_residency;
                dev->states_usage[entered_state].usage++;
        } else {
                dev->last_residency = 0;
        }
-------

We are setting last_residency to 0 when (entered_state < 0) and aren't using
the value of "diff". So, we can actually skip calculations of time_end, diff and
last_residency when (entered_state < 0).. Makes sense?


> We can be a long time in this state
> (eg. if the prediction is false).

Okay.. I didn't get it :)

Re: [PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Daniel Lezcano]

On 09/26/2013 08:24 AM, Viresh Kumar wrote:
> On 26 September 2013 04:08, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 09/22/2013 03:21 AM, Viresh Kumar wrote:
>>> If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we
>>> will be setting it to zero without using its new value.
>>
>> I don't get it, can you elaborate.
>
> Sure.. We have following code in cpuidle_enter_state():
>
> ---------
>          diff = ktime_to_us(ktime_sub(time_end, time_start));
>          if (diff > INT_MAX)
>                  diff = INT_MAX;
>
>          dev->last_residency = (int) diff;
>
>          if (entered_state >= 0) {
>                  /* Update cpuidle counters */
>                  /* This can be moved to within driver enter routine
>                   * but that results in multiple copies of same code.
>                   */
>                  dev->states_usage[entered_state].time += dev->last_residency;
>                  dev->states_usage[entered_state].usage++;
>          } else {
>                  dev->last_residency = 0;
>          }
> -------
>
> We are setting last_residency to 0 when (entered_state < 0) and aren't using
> the value of "diff". So, we can actually skip calculations of time_end, diff and
> last_residency when (entered_state < 0).. Makes sense?

Yes I agree, but why are you saying "If entered_state == 0, we don't 
need to ..." ?

>> We can be a long time in this state
>> (eg. if the prediction is false).
>
> Okay.. I didn't get it :)
>


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

Re: [PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Viresh Kumar]

On 26 September 2013 13:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Yes I agree, but why are you saying "If entered_state == 0, we don't need to
> ..." ?

Ahh.. that's a mistake.. s/==/< :)

Re: [PATCH 20/21] cpuidle: don't calculate time-diff if entered_state == 0

[Daniel Lezcano]

On 09/26/2013 10:28 AM, Viresh Kumar wrote:
> On 26 September 2013 13:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Yes I agree, but why are you saying "If entered_state == 0, we don't need to
>> ..." ?
>
> Ahh.. that's a mistake.. s/==/< :)

Ah ok.



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

[PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Viresh Kumar]

When I first read cpuidle_replace_governor()'s name I thought it will replace
the governor (as per its name) but then found that it just returns the next best
governor. And cpuidle_unregister_governor() actually replaces it.

We always replace current governor with the next best and this information is
already present with cpuidle_replace_governor() and so we don't really need to
send an additional argument for it.

Also, it makes sense to actually call cpuidle_switch_governor() from within
cpuidle_replace_governor() instead.

Along with this ret_gov is now renamed as new_gov to better suit its purpose.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpuidle/governor.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index ea2f8e7..deb6e50 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -98,26 +98,27 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
 }
 
 /**
- * cpuidle_replace_governor - find a replacement governor
- * @exclude_rating: the rating that will be skipped while looking for
- * new governor.
+ * cpuidle_replace_governor - replace governor with highest rating one
+ *
+ * Finds governor (excluding cpuidle_curr_governor) with highest rating and
+ * replaces cpuidle_curr_governor with it.
  */
-static struct cpuidle_governor *cpuidle_replace_governor(int exclude_rating)
+static inline void cpuidle_replace_governor(void)
 {
 	struct cpuidle_governor *gov;
-	struct cpuidle_governor *ret_gov = NULL;
+	struct cpuidle_governor *new_gov = NULL;
 	unsigned int max_rating = 0;
 
 	list_for_each_entry(gov, &cpuidle_governors, governor_list) {
-		if (gov->rating == exclude_rating)
+		if (gov == cpuidle_curr_governor)
 			continue;
 		if (gov->rating > max_rating) {
 			max_rating = gov->rating;
-			ret_gov = gov;
+			new_gov = gov;
 		}
 	}
 
-	return ret_gov;
+	cpuidle_switch_governor(new_gov);
 }
 
 /**
@@ -130,11 +131,8 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov)
 		return;
 
 	mutex_lock(&cpuidle_lock);
-	if (gov == cpuidle_curr_governor) {
-		struct cpuidle_governor *new_gov;
-		new_gov = cpuidle_replace_governor(gov->rating);
-		cpuidle_switch_governor(new_gov);
-	}
+	if (gov == cpuidle_curr_governor)
+		cpuidle_replace_governor();
 	list_del(&gov->governor_list);
 	mutex_unlock(&cpuidle_lock);
 }
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Daniel Lezcano]

On 09/22/2013 03:21 AM, Viresh Kumar wrote:
> When I first read cpuidle_replace_governor()'s name I thought it will replace
> the governor (as per its name) but then found that it just returns the next best
> governor. And cpuidle_unregister_governor() actually replaces it.
> 
> We always replace current governor with the next best and this information is
> already present with cpuidle_replace_governor() and so we don't really need to
> send an additional argument for it.
> 
> Also, it makes sense to actually call cpuidle_switch_governor() from within
> cpuidle_replace_governor() instead.
> 
> Along with this ret_gov is now renamed as new_gov to better suit its purpose.

Actually I am wondering if all this stuff is not over-engineered.

There are 2 governors, each of them suits for a specific kernel config,
periodic tick or tickless system.

menu     => tickless system
periodic => periodic tick system

IMHO, all the code with rating checking and so do not really makes
sense. Wouldn't make sense to remove this code ?

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpuidle/governor.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
> index ea2f8e7..deb6e50 100644
> --- a/drivers/cpuidle/governor.c
> +++ b/drivers/cpuidle/governor.c
> @@ -98,26 +98,27 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
>  }
>  
>  /**
> - * cpuidle_replace_governor - find a replacement governor
> - * @exclude_rating: the rating that will be skipped while looking for
> - * new governor.
> + * cpuidle_replace_governor - replace governor with highest rating one
> + *
> + * Finds governor (excluding cpuidle_curr_governor) with highest rating and
> + * replaces cpuidle_curr_governor with it.
>   */
> -static struct cpuidle_governor *cpuidle_replace_governor(int exclude_rating)
> +static inline void cpuidle_replace_governor(void)
>  {
>  	struct cpuidle_governor *gov;
> -	struct cpuidle_governor *ret_gov = NULL;
> +	struct cpuidle_governor *new_gov = NULL;
>  	unsigned int max_rating = 0;
>  
>  	list_for_each_entry(gov, &cpuidle_governors, governor_list) {
> -		if (gov->rating == exclude_rating)
> +		if (gov == cpuidle_curr_governor)
>  			continue;
>  		if (gov->rating > max_rating) {
>  			max_rating = gov->rating;
> -			ret_gov = gov;
> +			new_gov = gov;
>  		}
>  	}
>  
> -	return ret_gov;
> +	cpuidle_switch_governor(new_gov);
>  }
>  
>  /**
> @@ -130,11 +131,8 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov)
>  		return;
>  
>  	mutex_lock(&cpuidle_lock);
> -	if (gov == cpuidle_curr_governor) {
> -		struct cpuidle_governor *new_gov;
> -		new_gov = cpuidle_replace_governor(gov->rating);
> -		cpuidle_switch_governor(new_gov);
> -	}
> +	if (gov == cpuidle_curr_governor)
> +		cpuidle_replace_governor();
>  	list_del(&gov->governor_list);
>  	mutex_unlock(&cpuidle_lock);
>  }
> 


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

Re: [PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Viresh Kumar]

On 26 September 2013 04:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Actually I am wondering if all this stuff is not over-engineered.
>
> There are 2 governors, each of them suits for a specific kernel config,
> periodic tick or tickless system.
>
> menu     => tickless system
> periodic => periodic tick system
>
> IMHO, all the code with rating checking and so do not really makes
> sense. Wouldn't make sense to remove this code ?

I am a newbie here, really can't think of all side effects of this :)

Re: [PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Daniel Lezcano]

On 09/26/2013 08:37 AM, Viresh Kumar wrote:
> On 26 September 2013 04:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Actually I am wondering if all this stuff is not over-engineered.
>>
>> There are 2 governors, each of them suits for a specific kernel config,
>> periodic tick or tickless system.
>>
>> menu     => tickless system
>> periodic => periodic tick system
>>
>> IMHO, all the code with rating checking and so do not really makes
>> sense. Wouldn't make sense to remove this code ?
>
> I am a newbie here, really can't think of all side effects of this :)

Rafael is pretty busy ATM but may be he can give his feedback on this later.


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

Re: [PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Viresh Kumar]

On 26 September 2013 13:50, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Rafael is pretty busy ATM but may be he can give his feedback on this later.

For now I will resend it and maybe later you can get it cleaned up even more..
Or maybe I will do it once I have better hold on cpuidle core :)

Can I have your Ack for now? (As discussed on IRC) :)

--
viresh

Re: [PATCH 21/21] cpuidle: change governor from within cpuidle_replace_governor()

[Daniel Lezcano]

On 10/03/2013 12:36 PM, Viresh Kumar wrote:
> On 26 September 2013 13:50, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> Rafael is pretty busy ATM but may be he can give his feedback on this later.
>
> For now I will resend it and maybe later you can get it cleaned up even more..
> Or maybe I will do it once I have better hold on cpuidle core :)
>
> Can I have your Ack for now? (As discussed on IRC) :)

Actually the functions cpuidle_unregister_governor and 
cpuidle_replace_governor are dead code since the governors are no longer 
modules (commit 137b944e100278d696826cf25c83014ac17473fe), so you can 
remove the code instead.


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