From: skannan@codeaurora.org (Saravana Kannan)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 3/5] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend
Date: Mon, 11 Aug 2014 15:13:05 -0700 [thread overview]
Message-ID: <53E93FF1.1010809@codeaurora.org> (raw)
In-Reply-To: <CAKohpom-+At=YjEkZH-mn8OP1ATDBzuLw7mTPaUnG+1qvJ8FTA@mail.gmail.com>
On 08/07/2014 03:48 AM, Viresh Kumar wrote:
> On 25 July 2014 06:37, Saravana Kannan <skannan@codeaurora.org> wrote:
>> This patch simplifies a lot of the hotplug/suspend code by not
>> adding/removing/moving the policy/sysfs/kobj during hotplug and just leaves
>> the cpufreq directory and policy in place irrespective of whether the CPUs
>> are ONLINE/OFFLINE.
>>
>> Leaving the policy, sysfs and kobject in place also brings these additional
>> benefits:
>> * Faster suspend/resume
>> * Faster hotplug
>> * Sysfs file permissions maintained across hotplug
>> * Policy settings and governor tunables maintained across hotplug
>> * Cpufreq stats would be maintained across hotplug for all CPUs and can be
>> queried even after CPU goes OFFLINE
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>> drivers/cpufreq/cpufreq.c | 83 ++++++++++++++++-------------------------------
>> 1 file changed, 28 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index af4f291..d9fc6e5 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -865,7 +865,7 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>> unsigned int j;
>> int ret = 0;
>>
>> - for_each_cpu(j, policy->cpus) {
>> + for_each_cpu(j, policy->related_cpus) {
>> struct device *cpu_dev;
>>
>> if (j == policy->kobj_cpu)
>> @@ -968,7 +968,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>> int ret = 0;
>> unsigned long flags;
>>
>> - if (has_target()) {
>> + if (cpumask_weight(policy->cpus) && has_target()) {
>
> Probably cpumask_empty() would be more readable here.
Agreed.
>
>> ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>> if (ret) {
>> pr_err("%s: Failed to stop governor\n", __func__);
>> @@ -997,7 +997,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>> }
>> }
>>
>> - return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
>> + return 0;
>> }
>> #endif
>>
>> @@ -1100,9 +1100,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> struct cpufreq_policy *policy;
>> unsigned long flags;
>> bool recover_policy = cpufreq_suspended;
>> -#ifdef CONFIG_HOTPLUG_CPU
>> - struct cpufreq_policy *tpolicy;
>> -#endif
>>
>> if (cpu_is_offline(cpu))
>> return 0;
>> @@ -1113,28 +1110,22 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> /* check whether a different CPU already registered this
>> * CPU because it is in the same boat. */
>> policy = cpufreq_cpu_get(cpu);
>> - if (unlikely(policy)) {
>> + if (policy) {
>> + if (!cpumask_test_cpu(cpu, policy->cpus))
>> + ret = cpufreq_add_policy_cpu(policy, cpu, dev);
>> + else
>> + ret = 0;
>> cpufreq_cpu_put(policy);
>> - return 0;
>> + return ret;
>> }
>> #endif
>>
>> if (!down_read_trylock(&cpufreq_rwsem))
>> return 0;
>>
>> -#ifdef CONFIG_HOTPLUG_CPU
>> - /* Check if this cpu was hot-unplugged earlier and has siblings */
>> - read_lock_irqsave(&cpufreq_driver_lock, flags);
>> - list_for_each_entry(tpolicy, &cpufreq_policy_list, policy_list) {
>> - if (cpumask_test_cpu(cpu, tpolicy->related_cpus)) {
>> - read_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> - ret = cpufreq_add_policy_cpu(tpolicy, cpu, dev);
>> - up_read(&cpufreq_rwsem);
>> - return ret;
>> - }
>> - }
>> - read_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> -#endif
>> + /* If we get this far, this is the first time we are adding the
>> + * policy */
>
> I think I have already asked you to use proper comment style?
I did. Then I think I noticed some of the existing comments did keep the
/* in its own line even for multiline comments. So, I got confused. Will
fix.
>
>> + recover_policy = false;
>
> For this patch, probably it will work fine but I hope you will get rid of
> this variable completely in next patches..
>
Yup. In 5/5
>
>> @@ -1340,21 +1331,15 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>> struct subsys_interface *sif)
>> {
>> unsigned int cpu = dev->id, cpus;
>> - int new_cpu, ret;
>> + int new_cpu, ret = 0;
>
> Why?
Apparently for no good reason :) Probably some stale change when I was
splitting up the patches. I'll double check and remove this.
>> unsigned long flags;
>> struct cpufreq_policy *policy;
>>
>> pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
>>
>> - write_lock_irqsave(&cpufreq_driver_lock, flags);
>> -
>> + read_lock_irqsave(&cpufreq_driver_lock, flags);
>> policy = per_cpu(cpufreq_cpu_data, cpu);
>> -
>> - /* Save the policy somewhere when doing a light-weight tear-down */
>> - if (cpufreq_suspended)
>> - per_cpu(cpufreq_cpu_data_fallback, cpu) = policy;
>> -
>> - write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> + read_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>
>> if (!policy) {
>> pr_debug("%s: No cpu_data found\n", __func__);
>> @@ -1369,24 +1354,15 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>> }
>> }
>>
>> - if (!cpufreq_driver->setpolicy)
>> - strncpy(per_cpu(cpufreq_cpu_governor, cpu),
>> - policy->governor->name, CPUFREQ_NAME_LEN);
>> -
>
> Why? Probably I did mention this earlier as well?
This code is saving the governor name here to restore it when the policy
is created again after suspend/resume or hotplug of all CPUs. Since we
no longer throw away the policy struct, there's no point in doing this.
I should remove this per cpu variable though. Will do it in v5.
>
>> down_read(&policy->rwsem);
>> cpus = cpumask_weight(policy->cpus);
>> up_read(&policy->rwsem);
>>
>> - if (cpu != policy->cpu) {
>> - sysfs_remove_link(&dev->kobj, "cpufreq");
>> - } else if (cpus > 1) {
>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu);
>> - if (new_cpu >= 0) {
>> - update_policy_cpu(policy, new_cpu);
>> -
>> - if (!cpufreq_suspended)
>> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
>> - __func__, new_cpu, cpu);
>> + if (cpus > 1) {
>> + if (cpu == policy->cpu) {
>> + new_cpu = cpumask_any_but(policy->cpus, cpu);
>> + if (new_cpu >= 0)
>
> Can this ever be false?
If this is the last CPU going down. This part of the code didn't really
change. I just moved the cpumask_any_but() from nominate policy to here
since I'm not longer moving the kobj around.
>
>> + update_policy_cpu(policy, new_cpu);
>> }
>> } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
>> cpufreq_driver->stop_cpu(policy);
>> @@ -1431,6 +1407,9 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>> cpus = cpumask_weight(policy->cpus);
>> up_read(&policy->rwsem);
>>
>> + if (cpu != policy->kobj_cpu)
>> + sysfs_remove_link(&dev->kobj, "cpufreq");
>> +
>
> Why?
For the physical hot-remove case or when the cpufreq driver is unregistered.
>
>> /* If cpu is last user of policy, free policy */
>> if (cpus == 0) {
>> if (has_target()) {
>> @@ -1475,12 +1454,10 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>> static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>> {
>> unsigned int cpu = dev->id;
>> - int ret;
>> -
>> - if (cpu_is_offline(cpu))
>> - return 0;
>> + int ret = 0;
>>
>> - ret = __cpufreq_remove_dev_prepare(dev, sif);
>> + if (cpu_online(cpu))
>> + ret = __cpufreq_remove_dev_prepare(dev, sif);
>
> Why do you need a change here?
Since we no longer do remove_dev_finish during hotplug, we can't just
short circuit the entire function. We have to finish the remove when the
CPU is hot-removed or when the cpufreq driver is unregistered.
>
>> if (!ret)
>> ret = __cpufreq_remove_dev_finish(dev, sif);
>> @@ -2307,10 +2284,6 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,
>> __cpufreq_remove_dev_prepare(dev, NULL);
>> break;
>>
>> - case CPU_POST_DEAD:
>> - __cpufreq_remove_dev_finish(dev, NULL);
>> - break;
>> -
>
> Sure? Who will call dev_finish() now?
At this point, all remove_dev_finish() does is remove the sysfs links
and destroy the policy. So, it never needs to be called for hotplug.
Only during physical hot-remove or during cpufreq driver unregister.
>
>> case CPU_DOWN_FAILED:
>> __cpufreq_add_dev(dev, NULL);
>> break;
>> --
>> 1.8.2.1
>>
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
next prev parent reply other threads:[~2014-08-11 22:13 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-10 2:37 [PATCH] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan
2014-07-11 4:18 ` [PATCH v2] " Saravana Kannan
2014-07-11 6:19 ` Viresh Kumar
2014-07-11 9:59 ` skannan at codeaurora.org
2014-07-11 10:07 ` skannan at codeaurora.org
2014-07-11 10:52 ` Viresh Kumar
2014-07-12 2:44 ` Saravana Kannan
2014-07-14 6:09 ` Viresh Kumar
2014-07-14 19:08 ` Saravana Kannan
2014-07-15 4:35 ` Viresh Kumar
2014-07-15 5:36 ` Saravana Kannan
2014-07-15 5:52 ` Viresh Kumar
2014-07-15 6:58 ` Srivatsa S. Bhat
2014-07-15 17:35 ` skannan at codeaurora.org
2014-07-16 7:44 ` Srivatsa S. Bhat
2014-07-16 5:44 ` Viresh Kumar
2014-07-16 7:49 ` Srivatsa S. Bhat
2014-07-12 3:06 ` Saravana Kannan
2014-07-14 6:13 ` Viresh Kumar
2014-07-14 19:10 ` Saravana Kannan
2014-07-11 7:43 ` Srivatsa S. Bhat
2014-07-11 10:02 ` skannan at codeaurora.org
2014-07-15 22:47 ` [PATCH v3 0/2] Simplify hotplug/suspend handling Saravana Kannan
2014-07-15 22:47 ` [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan
2014-07-16 0:28 ` Saravana Kannan
2014-07-16 8:30 ` Viresh Kumar
2014-07-16 19:19 ` Saravana Kannan
2014-07-16 8:24 ` Viresh Kumar
2014-07-16 11:16 ` Srivatsa S. Bhat
2014-07-16 13:13 ` Viresh Kumar
2014-07-16 18:04 ` Srivatsa S. Bhat
2014-07-16 19:56 ` Saravana Kannan
2014-07-17 5:51 ` Viresh Kumar
2014-07-16 19:56 ` Saravana Kannan
2014-07-17 5:35 ` Viresh Kumar
2014-07-18 3:25 ` Saravana Kannan
2014-07-18 4:19 ` Viresh Kumar
2014-07-16 20:25 ` Saravana Kannan
2014-07-16 21:45 ` Saravana Kannan
2014-07-17 6:24 ` Viresh Kumar
2014-07-16 14:29 ` Dirk Brandewie
2014-07-16 15:28 ` Viresh Kumar
2014-07-16 19:42 ` Saravana Kannan
2014-07-15 22:47 ` [PATCH v3 2/2] cpufreq: Simplify and fix mutual exclusion with hotplug Saravana Kannan
2014-07-16 8:48 ` Viresh Kumar
2014-07-16 19:34 ` Saravana Kannan
2014-07-25 1:07 ` [PATCH v4 0/5] Simplify hotplug/suspend handling Saravana Kannan
2014-07-25 1:07 ` [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor Saravana Kannan
2014-07-31 20:47 ` Saravana Kannan
2014-07-25 1:07 ` [PATCH v4 2/5] cpufreq: Keep track of which CPU owns the kobj/sysfs nodes separately Saravana Kannan
2014-08-07 9:02 ` Viresh Kumar
2014-07-25 1:07 ` [PATCH v4 3/5] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan
2014-07-31 21:56 ` Rafael J. Wysocki
2014-07-31 22:15 ` Saravana Kannan
2014-07-31 23:48 ` Saravana Kannan
2014-08-07 10:51 ` Viresh Kumar
2014-08-12 9:17 ` Viresh Kumar
2014-08-07 10:48 ` Viresh Kumar
2014-08-11 22:13 ` Saravana Kannan [this message]
2014-08-12 8:51 ` Viresh Kumar
2014-07-25 1:07 ` [PATCH v4 4/5] cpufreq: Properly handle physical CPU hot-add/hot-remove Saravana Kannan
2014-08-07 11:02 ` Viresh Kumar
2014-08-11 22:15 ` Saravana Kannan
2014-07-25 1:07 ` [PATCH v4 5/5] cpufreq: Delete dead code related to policy save/restore Saravana Kannan
2014-08-07 11:06 ` Viresh Kumar
2014-07-29 5:52 ` [PATCH v4 0/5] Simplify hotplug/suspend handling skannan at codeaurora.org
2014-07-30 0:29 ` Rafael J. Wysocki
2014-07-31 20:25 ` Saravana Kannan
2014-08-07 6:04 ` skannan at codeaurora.org
2014-10-16 8:53 ` Viresh Kumar
2014-10-23 21:41 ` Saravana Kannan
2014-07-16 22:02 ` [PATCH] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Rafael J. Wysocki
2014-07-16 22:35 ` Saravana Kannan
2014-07-24 3:02 ` Saravana Kannan
2014-07-24 5:04 ` Viresh Kumar
2014-07-24 9:12 ` skannan at codeaurora.org
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53E93FF1.1010809@codeaurora.org \
--to=skannan@codeaurora.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).