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

2013-10-03 Thread Daniel Lezcano 

On 10/03/2013 12:36 PM, Viresh Kumar wrote:

On 26 September 2013 13:50, Daniel Lezcano  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.



--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 21/21] cpuidle: change governor from within cpuidle_replace_governor()

2013-10-03 Thread Viresh Kumar 
On 26 September 2013 13:50, Daniel Lezcano  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 21/21] cpuidle: change governor from within cpuidle_replace_governor()

2013-09-26 Thread Daniel Lezcano 

On 09/26/2013 08:37 AM, Viresh Kumar wrote:

On 26 September 2013 04:20, Daniel Lezcano  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.


--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 21/21] cpuidle: change governor from within cpuidle_replace_governor()

2013-09-25 Thread Viresh Kumar 
On 26 September 2013 04:20, Daniel Lezcano  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 :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 21/21] cpuidle: change governor from within cpuidle_replace_governor()

2013-09-25 Thread 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 
> ---
>  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);
>  }
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2013-09-21 Thread 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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/