lkml.org 
[lkml]   [2014]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v4 3/5] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend
    From
    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.

    > 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?

    > + 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..


    > @@ -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?

    > 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?

    > 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?

    > + 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?

    > /* 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?

    > 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?

    > 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


    \
     
     \ /
      Last update: 2014-08-07 13:01    [W:3.710 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site