| Message ID | bc7154266269beb8c6b9b3d56b529b64db33d2e2.1422346933.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 12 February 2015 at 11:03, Saravana Kannan <skannan@codeaurora.org> wrote: >> - gov = find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu)); >> + gov = find_governor(policy->last_governor); > > Just change this to: > > gov = policy->governor; > > No need to search for the governor again. It should already be valid for > policy that's being restored. For new policies, it would be NULL and would > get defaulted correctly. No. Apart from the fact that one of my patches is making it NULL intentionally, here are the problems that I see with reusing the pointer: - All CPUs of a policy are down - The last used governor is removed (was built in as a module) - Now if we online the CPUs again, it wouldn't work as the pointer insn't usable. - Or if we insert the governor again, pointers would change and then onlining the CPUs wouldn't work.. > 2. For physical hotplug of CPUs, the policies are getting freed (I assume > and hope so). So, the "last_governor" field is going to be empty. Its not about it being empty. Its more about when we don't remove them physically.. > 3. Even if we continue storing the last_governor outside of the policy, for > physical hotplug of CPUs where the policy is getting recreated, I'm not sure > restoring the governor is the right thing to do anyway. I'll explain various > possible configurations below for the physical hotplug case: We do it today as this is part of the per-cpu stuff now and my patch would fix it then :) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 February 2015 at 16:00, <skannan@codeaurora.org> wrote: > I think for logical hotplug we should lock the governor from being Nack. > removed. Or, if you don't want that, go set the pointer to NULL lazily > when/if that happens. That would be another band-aid .. > I'm not sure if you read rest of my email. Unless you added some patches I did read them.. > recently, restoring the last_governor is definitely functionally broken as > of at least 3.12. Probably 3.14 too (I need to check again). It just > restores the governor with the default tunables. So, it's kinda useless in > the real world. You might as well set it to default governor because it's > just as helpful as using the last governor with different tunables. I agree that the tunables aren't restored, but this is useful atleast for the case where the governor tunables are shared across policies. And so we don't have to change the tunables on policy-re-init.. This patch wasn't about changing the functionality but about saving some memory and simple code flow.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f253cf45f910..4ad1e46891b5 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -57,9 +57,6 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_RWLOCK(cpufreq_driver_lock); DEFINE_MUTEX(cpufreq_governor_lock); -/* This one keeps track of the previously set governor of a removed CPU */ -static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); - /* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended; @@ -941,7 +938,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) memcpy(&new_policy, policy, sizeof(*policy)); /* Update governor of new_policy to the governor used before hotplug */ - gov = find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu)); + gov = find_governor(policy->last_governor); if (gov) pr_debug("Restoring governor %s for cpu %d\n", policy->governor->name, policy->cpu); @@ -1366,8 +1363,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, return ret; } - strncpy(per_cpu(cpufreq_cpu_governor, cpu), - policy->governor->name, CPUFREQ_NAME_LEN); + if (cpus == 1) + strncpy(policy->last_governor, policy->governor->name, + CPUFREQ_NAME_LEN); } if (cpu != policy->cpu) { @@ -2096,7 +2094,8 @@ EXPORT_SYMBOL_GPL(cpufreq_register_governor); void cpufreq_unregister_governor(struct cpufreq_governor *governor) { - int cpu; + struct cpufreq_policy *policy; + unsigned long flags; if (!governor) return; @@ -2104,12 +2103,13 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor) if (cpufreq_disabled()) return; - for_each_present_cpu(cpu) { - if (cpu_online(cpu)) - continue; - if (!strcmp(per_cpu(cpufreq_cpu_governor, cpu), governor->name)) - strcpy(per_cpu(cpufreq_cpu_governor, cpu), "\0"); + /* clear last_governor for all fallback policies */ + read_lock_irqsave(&cpufreq_driver_lock, flags); + for_each_fallback_policy(policy) { + if (!strcmp(policy->last_governor, governor->name)) + strcpy(policy->last_governor, "\0"); } + read_unlock_irqrestore(&cpufreq_driver_lock, flags); mutex_lock(&cpufreq_governor_mutex); list_del(&governor->governor_list); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index df6af4cfa26a..e326cddef6db 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -80,6 +80,7 @@ struct cpufreq_policy { struct cpufreq_governor *governor; /* see below */ void *governor_data; bool governor_enabled; /* governor start/stop flag */ + char last_governor[CPUFREQ_NAME_LEN]; /* last governor used */ struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */
History of which governor was used last is common to all CPUs within a policy and maintaining it per-cpu isn't the best approach for sure. Apart from wasting memory, this also increases the complexity of managing this data structure as it has to be updated for all CPUs. To make that somewhat simpler, lets store this information in a new field 'last_governor' in struct cpufreq_policy and update it on removal of last cpu of a policy. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 24 ++++++++++++------------ include/linux/cpufreq.h | 1 + 2 files changed, 13 insertions(+), 12 deletions(-)