| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53C5C738.5040705@codeaurora.org>
Date: Tue, 15 Jul 2014 17:28:40 -0700
From: Saravana Kannan <skannan@...eaurora.org>
To: Saravana Kannan <skannan@...eaurora.org>
CC: "Rafael J . Wysocki" <rjw@...ysocki.net>,
Viresh Kumar <viresh.kumar@...aro.org>,
Todd Poynor <toddpoynor@...gle.com>,
"Srivatsa S . Bhat" <srivatsa@....edu>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Stephen Boyd <sboyd@...eaurora.org>
Subject: Re: [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@...eaurora.org>
> Signed-off-by: Saravana Kannan <skannan@...eaurora.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists