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

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

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

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

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.

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

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.