Re: [PATCH] cpufreq: fix governor start/stop race condition

[Xiaoguang Chen <chenxg@marvell.com>]

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@marvell.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