[PATCH 1/2] cpufreq: Add governor operation ongoing flag

[PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Xiaoguang Chen]

__cpufreq_governor operation needs to be executed one by one.
If one operation is ongoing, the other operation can't be executed.
If the order is not guaranteed, there may be unexpected behavior.

 For example, governor is in enable state, and one process
tries to stop the goveror, but it is scheduled out before policy->
governor->governor() is executed, but the governor enable flag is
set to false already. Then one other process tries to start governor,
It finds enable flag is false, and it can process down to do governor
start operation, So the governor is started twice.

Add the governor_ops_ongoing flag to check whether there is governor
operation ongoing, if so, return -EAGAIN.

Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
---
 drivers/cpufreq/cpufreq.c | 9 +++++++++
 include/linux/cpufreq.h   | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f0a5e2b..dfe0b86 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1652,6 +1652,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
+	if (policy->governor_ops_ongoing) {
+		mutex_unlock(&cpufreq_governor_lock);
+		return -EAGAIN;
+	}
+	policy->governor_ops_ongoing = true;
 
 	if (event == CPUFREQ_GOV_STOP)
 		policy->governor_enabled = false;
@@ -1684,6 +1689,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 	if ((event == CPUFREQ_GOV_STOP) && !ret)
 		module_put(policy->governor->owner);
 
+	mutex_lock(&cpufreq_governor_lock);
+	policy->governor_ops_ongoing = false;
+	mutex_unlock(&cpufreq_governor_lock);
+
 	return ret;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 90d5a15..8d3eac2 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -111,6 +111,7 @@ struct cpufreq_policy {
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	bool			governor_enabled; /* governor start/stop flag */
+	bool			governor_ops_ongoing;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
-- 
1.8.0

[PATCH 2/2] cpufreq: Only do governor start after successful stop

[Xiaoguang Chen]

cpufreq_add_policy_cpu, __cpufreq_remove_dev and __cpufreq_set_policy
have operations for governor stop and start.
Only do the start operation when the previous stop operation succeeds.

Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
---
 drivers/cpufreq/cpufreq.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index dfe0b86..2e0b17f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,7 +916,7 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	WARN_ON(!policy);
 
 	if (has_target)
-		__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 
 	lock_policy_rwsem_write(sibling);
 
@@ -930,7 +930,8 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	unlock_policy_rwsem_write(sibling);
 
 	if (has_target) {
-		__cpufreq_governor(policy, CPUFREQ_GOV_START);
+		if (!ret)
+			__cpufreq_governor(policy, CPUFREQ_GOV_START);
 		__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 	}
 
@@ -1109,7 +1110,7 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 static int __cpufreq_remove_dev(struct device *dev,
 		struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id, ret, cpus;
+	unsigned int cpu = dev->id, ret, opsret = 0, cpus;
 	unsigned long flags;
 	struct cpufreq_policy *data;
 	struct kobject *kobj;
@@ -1131,7 +1132,7 @@ static int __cpufreq_remove_dev(struct device *dev,
 	}
 
 	if (cpufreq_driver->target)
-		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+		opsret = __cpufreq_governor(data, CPUFREQ_GOV_STOP);
 
 #ifdef CONFIG_HOTPLUG_CPU
 	if (!cpufreq_driver->setpolicy)
@@ -1206,7 +1207,8 @@ static int __cpufreq_remove_dev(struct device *dev,
 		pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
 		cpufreq_cpu_put(data);
 		if (cpufreq_driver->target) {
-			__cpufreq_governor(data, CPUFREQ_GOV_START);
+			if (!opsret)
+				__cpufreq_governor(data, CPUFREQ_GOV_START);
 			__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
 		}
 	}
@@ -1784,7 +1786,7 @@ EXPORT_SYMBOL(cpufreq_get_policy);
 static int __cpufreq_set_policy(struct cpufreq_policy *data,
 				struct cpufreq_policy *policy)
 {
-	int ret = 0, failed = 1;
+	int ret = 0, failed = 1, opsret = 0;
 
 	pr_debug("setting new policy for CPU %u: %u - %u kHz\n", policy->cpu,
 		policy->min, policy->max);
@@ -1841,7 +1843,8 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 
 			/* end old governor */
 			if (data->governor) {
-				__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+				opsret = __cpufreq_governor(data,
+							    CPUFREQ_GOV_STOP);
 				unlock_policy_rwsem_write(policy->cpu);
 				__cpufreq_governor(data,
 						CPUFREQ_GOV_POLICY_EXIT);
@@ -1851,7 +1854,8 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 			/* start new governor */
 			data->governor = policy->governor;
 			if (!__cpufreq_governor(data, CPUFREQ_GOV_POLICY_INIT)) {
-				if (!__cpufreq_governor(data, CPUFREQ_GOV_START)) {
+				if (!opsret && !__cpufreq_governor(
+					data, CPUFREQ_GOV_START)) {
 					failed = 0;
 				} else {
 					unlock_policy_rwsem_write(policy->cpu);
@@ -1869,8 +1873,9 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 					data->governor = old_gov;
 					__cpufreq_governor(data,
 							CPUFREQ_GOV_POLICY_INIT);
-					__cpufreq_governor(data,
-							   CPUFREQ_GOV_START);
+					if (!opsret)
+						__cpufreq_governor(data,
+							CPUFREQ_GOV_START);
 				}
 				ret = -EINVAL;
 				goto error_out;
-- 
1.8.0

Re: [PATCH 2/2] cpufreq: Only do governor start after successful stop

[Viresh Kumar]

On 13 August 2013 12:39, Xiaoguang Chen <chenxg@marvell.com> wrote:
> cpufreq_add_policy_cpu, __cpufreq_remove_dev and __cpufreq_set_policy
> have operations for governor stop and start.
> Only do the start operation when the previous stop operation succeeds.
>
> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
> ---
>  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

I hope you have seen this patch, which is already in Rafael's tree?

https://lkml.org/lkml/2013/8/6/357

Re: [PATCH 2/2] cpufreq: Only do governor start after successful stop

[Xiaoguang Chen]

2013/8/14 Viresh Kumar <viresh.kumar@linaro.org>:
> On 13 August 2013 12:39, Xiaoguang Chen <chenxg@marvell.com> wrote:
>> cpufreq_add_policy_cpu, __cpufreq_remove_dev and __cpufreq_set_policy
>> have operations for governor stop and start.
>> Only do the start operation when the previous stop operation succeeds.
>>
>> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> I hope you have seen this patch, which is already in Rafael's tree?
>
> https://lkml.org/lkml/2013/8/6/357

ohh, That is exactly what my patch tries to do. Sorry I just checked
mainline patch. I missed this one. Thank you very much!

Thanks
Xiaoguang

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Viresh Kumar]

On 13 August 2013 12:39, Xiaoguang Chen <chenxg@marvell.com> wrote:
> __cpufreq_governor operation needs to be executed one by one.
> If one operation is ongoing, the other operation can't be executed.
> If the order is not guaranteed, there may be unexpected behavior.

What order??

>  For example, governor is in enable state, and one process
> tries to stop the goveror, but it is scheduled out before policy->
> governor->governor() is executed, but the governor enable flag is
> set to false already. Then one other process tries to start governor,
> It finds enable flag is false, and it can process down to do governor
> start operation, So the governor is started twice.

That's not possible. A process will not and should not call START
before calling STOP. And so the order of calling these routines must
be forced.

Hence, we may not need your patch.

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Xiaoguang Chen]

2013/8/14 Viresh Kumar <viresh.kumar@linaro.org>:
> On 13 August 2013 12:39, Xiaoguang Chen <chenxg@marvell.com> wrote:
>> __cpufreq_governor operation needs to be executed one by one.
>> If one operation is ongoing, the other operation can't be executed.
>> If the order is not guaranteed, there may be unexpected behavior.
>
> What order??
I mean one stop operation is ongoing, one other process tries to call
a start operation.

>
>>  For example, governor is in enable state, and one process
>> tries to stop the goveror, but it is scheduled out before policy->
>> governor->governor() is executed, but the governor enable flag is
>> set to false already. Then one other process tries to start governor,
>> It finds enable flag is false, and it can process down to do governor
>> start operation, So the governor is started twice.
>
> That's not possible. A process will not and should not call START
> before calling STOP. And so the order of calling these routines must
> be forced.
>
> Hence, we may not need your patch.

Please see below code in __cpufreq_governor function

mutex_lock(&cpufreq_governor_lock);
if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
                            //////////// <1> Here one process A tries
to stop governor, it finds governor is enabled, so it will pass down.
   (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
                          /////////////<3> Process B tries to start
governor, it finds enable flag is false, so it can also pass down.
mutex_unlock(&cpufreq_governor_lock);
return -EBUSY;
}

if (event == CPUFREQ_GOV_STOP)
policy->governor_enabled = false;
                                                  //////////// < 2>
Here process A set flag to false and then process A is scheduled out
for some reasons(like interrupt or time slice end)
else if (event == CPUFREQ_GOV_START)
policy->governor_enabled = true;

mutex_unlock(&cpufreq_governor_lock);

ret = policy->governor->governor(policy, event);
                                           ///////////////<4>  Process
B executes the governor start operation, process A is not scheduled
back yet. as policy->governor->governor is not protected by


                the cpufreq_governor_lock, So this sequence can happen
really.









Thanks
Xiaoguang

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Viresh Kumar]

I am still not sure if I got what you are trying to say, sorry :(

On 14 August 2013 13:06, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
> Please see below code in __cpufreq_governor function
>
> mutex_lock(&cpufreq_governor_lock);
> if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
>                             //////////// <1> Here one process A tries
> to stop governor, it finds governor is enabled, so it will pass down.
>    (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
>                           /////////////<3> Process B tries to start
> governor, it finds enable flag is false, so it can also pass down.

Processes aren't allowed to call START/STOP in random manner. They
must do a STOP first, if it succeeds do a START.

So lets see it this way:

Process A                            Process B
STOP
                                            STOP
                                            START (If STOP passed)
START (If STOP passed)

So, Process B tries to STOP/START after governor is Stopped by A.
Now call to STOP for process B will fail as we are already stopped..

Can you explain with this example the problem you are trying to solve?

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Xiaoguang Chen]

2013/8/14 Viresh Kumar <viresh.kumar@linaro.org>:
> I am still not sure if I got what you are trying to say, sorry :(
>
> On 14 August 2013 13:06, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
>> Please see below code in __cpufreq_governor function
>>
>> mutex_lock(&cpufreq_governor_lock);
>> if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
>>                             //////////// <1> Here one process A tries
>> to stop governor, it finds governor is enabled, so it will pass down.
>>    (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
>>                           /////////////<3> Process B tries to start
>> governor, it finds enable flag is false, so it can also pass down.
>
> Processes aren't allowed to call START/STOP in random manner. They
> must do a STOP first, if it succeeds do a START.
>
> So lets see it this way:
>
> Process A                            Process B
> STOP
>                                             STOP
>                                             START (If STOP passed)
> START (If STOP passed)
>
> So, Process B tries to STOP/START after governor is Stopped by A.
> Now call to STOP for process B will fail as we are already stopped..
>
> Can you explain with this example the problem you are trying to solve?

Yes, "START (If STOP passed)", this is important,  we don't have this
patch on our code base, So even Process B's STOP failed(as governor
enable flag is set to false by process A already ), it can still do
START operation, So my problem occurs.
My problem is that I see ondemand governor is started twice during
frequent governor switch and cpu hotplug stress test.

The After seeing your patch for the return value checking, I think my
problem should not occur.
This issue really botherred me for a long time. :(


Thanks
Xiaoguang

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Viresh Kumar]

On 14 August 2013 13:49, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
> Yes, "START (If STOP passed)", this is important,  we don't have this
> patch on our code base, So even Process B's STOP failed(as governor
> enable flag is set to false by process A already ), it can still do
> START operation, So my problem occurs.
> My problem is that I see ondemand governor is started twice during
> frequent governor switch and cpu hotplug stress test.
>
> The After seeing your patch for the return value checking, I think my
> problem should not occur.
> This issue really botherred me for a long time. :(

Exactly, the problem was users of this API were abusing it as they didn't
cared for its return value. I hope that is fixed widely now and in case
some places are still left, they should be fixed as well.

--
viresh

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Xiaoguang Chen]

2013/8/14 Viresh Kumar <viresh.kumar@linaro.org>:
> On 14 August 2013 13:49, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
>> Yes, "START (If STOP passed)", this is important,  we don't have this
>> patch on our code base, So even Process B's STOP failed(as governor
>> enable flag is set to false by process A already ), it can still do
>> START operation, So my problem occurs.
>> My problem is that I see ondemand governor is started twice during
>> frequent governor switch and cpu hotplug stress test.
>>
>> The After seeing your patch for the return value checking, I think my
>> problem should not occur.
>> This issue really botherred me for a long time. :(
>
> Exactly, the problem was users of this API were abusing it as they didn't
> cared for its return value. I hope that is fixed widely now and in case
> some places are still left, they should be fixed as well.
>
> --
> viresh


Hi, Viresh
After checking your patch, I find that __cpufreq_set_policy function
doesn't check STOP and EXIT 's return value
is it on purpose? if not, I can provide a patch to add it.

Thanks
Xiaoguang

Re: [PATCH 1/2] cpufreq: Add governor operation ongoing flag

[Viresh Kumar]

On 14 August 2013 14:20, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote:
> Hi, Viresh
> After checking your patch, I find that __cpufreq_set_policy function
> doesn't check STOP and EXIT 's return value
> is it on purpose? if not, I can provide a patch to add it.

I thought we probably can't break on these calls here, while we are trying to
stop existing governor. And so didn't check return values..

If you face some issues with them, you are free to add checks.