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 <[email protected]>
> ---
> 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
Sorry for being late buddy..
On 16 May 2013 11:44, Xiaoguang Chen <[email protected]> wrote:
> On 05/13/2013 06:47 PM, Xiaoguang Chen wrote:
>>
Why is the mail came this way.. You forwarded it?
>> 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 <[email protected]>
>> ---
>> 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);
We can't have locks are GOV_STOP earlier.. And now we can't have it
across *_EXIT.. Check latest code... As this gives some circular dependency
to locking and it fails.
On 05/22/2013 04:46 PM, Viresh Kumar wrote:
> Sorry for being late buddy..
>
> On 16 May 2013 11:44, Xiaoguang Chen <[email protected]> wrote:
>> On 05/13/2013 06:47 PM, Xiaoguang Chen wrote:
> Why is the mail came this way.. You forwarded it?
I didn't see your reponse, So I once replied this mail once.:)
>
>>> 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 <[email protected]>
>>> ---
>>> 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);
> We can't have locks are GOV_STOP earlier.. And now we can't have it
> across *_EXIT.. Check latest code... As this gives some circular dependency
> to locking and it fails.
Do you mean my patch will cause deadlock? I once tried to add another lock
to protect the GOV_STOP/START sequence instead of using the rwsem in
this patch.
But I saw deadlock indeed.
In cpufreq_add_policy_cpu, the lock has to be added before the rwsem
since GOV_STOP is
before lock_policy_rwsem_write, but in cpufreq_update_policy, it will
first get the rwsem, and then
call __cpufreq_set_policy which will contain GOV_STOP again, if we add
the new lock before this GOV_STOP,
then we may get deadlock in below sequence:
1) hotplug in one cpu by calling cpufreq_add_policy_cpu in which new
lock is locked first then rwsem is locked.
2) governor change in cpufreq_update_policy in which rwsem is locked
first then new lock is locked.
this is a deadlock issue if above two steps interleaves
--
Thanks
Xiaoguang
On 23 May 2013 08:14, Xiaoguang Chen <[email protected]> wrote:
> Do you mean my patch will cause deadlock? I once tried to add another lock
> to protect the GOV_STOP/START sequence instead of using the rwsem in this
> patch.
> But I saw deadlock indeed.
> In cpufreq_add_policy_cpu, the lock has to be added before the rwsem since
> GOV_STOP is
> before lock_policy_rwsem_write, but in cpufreq_update_policy, it will first
> get the rwsem, and then
> call __cpufreq_set_policy which will contain GOV_STOP again, if we add the
> new lock before this GOV_STOP,
> then we may get deadlock in below sequence:
> 1) hotplug in one cpu by calling cpufreq_add_policy_cpu in which new lock is
> locked first then rwsem is locked.
> 2) governor change in cpufreq_update_policy in which rwsem is locked first
> then new lock is locked.
> this is a deadlock issue if above two steps interleaves
Check this patch.
https://patchwork-mail.kernel.org/patch/2575231/