lkml.org 
[lkml]   [2014]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend
    One preemptive comment.

    On 07/15/2014 03:47 PM, Saravana Kannan wrote:
    > The CPUfreq core moves the cpufreq policy ownership between CPUs when CPUs
    > within a cluster (CPUs sharing same policy) go ONLINE/OFFLINE. When moving
    > policy ownership between CPUs, it also moves the cpufreq sysfs directory
    > between CPUs and also fixes up the symlinks of the other CPUs in the
    > cluster.
    >
    > Also, when all the CPUs in a cluster go OFFLINE, all the sysfs nodes and
    > directories are deleted, the kobject is released and the policy is freed.
    > And when the first CPU in a cluster comes up, the policy is reallocated and
    > initialized, kobject is acquired, the sysfs nodes are created or symlinked,
    > etc.
    >
    > All these steps end up creating unnecessarily complicated code and locking.
    > There's no real benefit to adding/removing/moving the sysfs nodes and the
    > policy between CPUs. Other per CPU sysfs directories like power and cpuidle
    > are left alone during hotplug. So there's some precedence to what this
    > patch is trying to do.
    >
    > This patch simplifies a lot of the code and locking by removing the
    > adding/removing/moving of policy/sysfs/kobj 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
    >
    > Tested-by: Stephen Boyd <sboyd@codeaurora.org>
    > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
    > ---
    > drivers/cpufreq/cpufreq.c | 388 +++++++++++++---------------------------------
    > 1 file changed, 107 insertions(+), 281 deletions(-)
    >
    > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
    > index 62259d2..a0a2ec2 100644
    > --- a/drivers/cpufreq/cpufreq.c
    > +++ b/drivers/cpufreq/cpufreq.c

    <SNIP>

    > @@ -961,60 +967,58 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
    > }
    >
    > #ifdef CONFIG_HOTPLUG_CPU
    > -static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
    > - unsigned int cpu, struct device *dev)
    > +static int cpufreq_change_policy_cpus(struct cpufreq_policy *policy,
    > + unsigned int cpu, bool add)
    > {
    > int ret = 0;
    > - unsigned long flags;
    > + unsigned int cpus, pcpu;
    >
    > - if (has_target()) {
    > + down_write(&policy->rwsem);
    > +
    > + cpus = !cpumask_empty(policy->cpus);
    > + if (has_target() && cpus) {
    > ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
    > if (ret) {
    > pr_err("%s: Failed to stop governor\n", __func__);
    > - return ret;
    > + goto unlock;
    > }
    > }
    >

    <SNIP>

    > + if (add)
    > + cpumask_set_cpu(cpu, policy->cpus);
    > + else
    > + cpumask_clear_cpu(cpu, policy->cpus);
    >
    > - up_write(&policy->rwsem);
    > + pcpu = cpumask_first(policy->cpus);
    > + if (pcpu < nr_cpu_ids && policy->cpu != pcpu) {
    > + policy->last_cpu = policy->cpu;
    > + policy->cpu = pcpu;
    > + blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
    > + CPUFREQ_UPDATE_POLICY_CPU, policy);
    > + }
    >
    > - if (has_target()) {
    > + cpus = !cpumask_empty(policy->cpus);
    > + if (has_target() && cpus) {
    > ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
    > if (!ret)
    > ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
    >
    > if (ret) {
    > pr_err("%s: Failed to start governor\n", __func__);
    > - return ret;
    > + goto unlock;
    > }
    > }
    >

    <SNIP>

    > + if (!cpus && cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
    > + cpufreq_driver->stop_cpu(policy);
    > + }
    >

    Viresh, I tried your suggestion (and my initial thought too) to combine
    this as an if/else with the previous if. But the indentation got nasty
    and made it hard to read. I'm sure the compiler will optimize it. So, I
    would prefer to leave it this way.


    > - policy->governor = NULL;
    > +unlock:
    > + up_write(&policy->rwsem);
    >
    > - return policy;
    > + return ret;
    > }
    > +#endif
    >

    -Saravana

    --
    The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
    hosted by The Linux Foundation


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