| 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: <51947935.50607@marvell.com>
Date: Thu, 16 May 2013 14:14:13 +0800
From: Xiaoguang Chen <chenxg@...vell.com>
To: Xiaoguang Chen <chenxg@...vell.com>
CC: "rjw@...k.pl" <rjw@...k.pl>,
"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ning Jiang <njiang1@...vell.com>, Yilu Mao <ylmao@...vell.com>,
Zhoujie Wu <zjwu@...vell.com>
Subject: Re: [PATCH] cpufreq: fix governor start/stop race condition
On 05/13/2013 06:47 PM, Xiaoguang Chen wrote:
> cpufreq governor stop and start should be kept in sequence.
> If not, there will be unexpected behavior, for example:
>
> we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0.
> the normal sequence is as below:
>
> 1) Current governor is userspace, one application tries to set
> governor to ondemand. it will call __cpufreq_set_policy in which it
> will stop userspace governor and then start ondemand governor.
>
> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
> call cpufreq_add_policy_cpu. on which it first stops userspace
> governor, and then starts userspace governor.
>
> Now if the sequence of above two cases interleaves, it becames
> below sequence:
>
> 1) application stops userspace governor
> 2) hotplug stops userspace governor
> 3) application starts ondemand governor
> 4) hotplug starts a governor
>
> in step 4, hotplug is supposed to start userspace governor, but now
> the governor has been changed by application to ondemand, so hotplug
> starts ondemand governor again !!!!
>
> The solution is as below:
> cpufreq policy has a rwsem to protect the read and write of policy.
> make the scope of the rwsem to contain cpufreq governor stop/start
> sequence, so that after the stop governor has started, other threads
> will not stop governor, they have to wait the current thread starts
> the governor and then do their job.
>
> Change-Id: I054bb52789fc8abdcf80bdcc1caebd429c182bb0
> Signed-off-by: Xiaoguang Chen <chenxg@...vell.com>
> ---
> drivers/cpufreq/cpufreq.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..935f750 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -811,14 +811,14 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
> int ret = 0, has_target = !!cpufreq_driver->target;
> unsigned long flags;
>
> + lock_policy_rwsem_write(sibling);
> +
> policy = cpufreq_cpu_get(sibling);
> WARN_ON(!policy);
>
> if (has_target)
> __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>
> - lock_policy_rwsem_write(sibling);
> -
> write_lock_irqsave(&cpufreq_driver_lock, flags);
>
> cpumask_set_cpu(cpu, policy->cpus);
> @@ -826,13 +826,13 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
> per_cpu(cpufreq_cpu_data, cpu) = policy;
> write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>
> - unlock_policy_rwsem_write(sibling);
> -
> if (has_target) {
> __cpufreq_governor(policy, CPUFREQ_GOV_START);
> __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> }
>
> + unlock_policy_rwsem_write(sibling);
> +
> ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> if (ret) {
> cpufreq_cpu_put(policy);
> @@ -1028,6 +1028,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> return -EINVAL;
> }
>
> + WARN_ON(lock_policy_rwsem_write(cpu));
> +
> if (cpufreq_driver->target)
> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
>
> @@ -1037,12 +1039,10 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> data->governor->name, CPUFREQ_NAME_LEN);
> #endif
>
> - WARN_ON(lock_policy_rwsem_write(cpu));
> cpus = cpumask_weight(data->cpus);
>
> if (cpus > 1)
> cpumask_clear_cpu(cpu, data->cpus);
> - unlock_policy_rwsem_write(cpu);
>
> if (cpu != data->cpu) {
> sysfs_remove_link(&dev->kobj, "cpufreq");
> @@ -1054,7 +1054,6 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> if (ret) {
> pr_err("%s: Failed to move kobj: %d", __func__, ret);
>
> - WARN_ON(lock_policy_rwsem_write(cpu));
> cpumask_set_cpu(cpu, data->cpus);
>
> write_lock_irqsave(&cpufreq_driver_lock, flags);
> @@ -1068,9 +1067,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> return -EINVAL;
> }
>
> - WARN_ON(lock_policy_rwsem_write(cpu));
> update_policy_cpu(data, cpu_dev->id);
> - unlock_policy_rwsem_write(cpu);
> pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
> __func__, cpu_dev->id, cpu);
> }
> @@ -1083,10 +1080,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> if (cpufreq_driver->target)
> __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
>
> - lock_policy_rwsem_read(cpu);
> kobj = &data->kobj;
> cmp = &data->kobj_unregister;
> - unlock_policy_rwsem_read(cpu);
> kobject_put(kobj);
>
> /* we need to make sure that the underlying kobj is actually
> @@ -1108,6 +1103,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
> __cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
> }
>
> + unlock_policy_rwsem_write(cpu);
> +
> per_cpu(cpufreq_policy_cpu, cpu) = -1;
> return 0;
> }
Hi, Guys
What's your opinion about this patch?
--
Thanks
Xiaoguang
--
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