Messages in this thread | | | Date | Thu, 26 Sep 2013 00:50:55 +0200 | From | Daniel Lezcano <> | Subject | 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); > } >
-- <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 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/
| |