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

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

}