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.ku...@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
--
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/
