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

[Xiaoguang Chen]

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: do not allow stop governor multi-times
Governor stop should only do once, after it is stopped,
no other governor stop should be executed.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..c8d7cb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1562,6 +1562,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

pr_debug("__cpufreq_governor for CPU %u, event %u\n",
policy->cpu, event);
+
+ if ((!policy->governor->enabled && (event == CPUFREQ_GOV_STOP)) ||
+ (policy->governor->enabled && (event == CPUFREQ_GOV_START)))
+ return 0;
+
ret = policy->governor->governor(policy, event);

if (!ret) {
@@ -1569,6 +1574,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
policy->governor->initialized++;
else if (event == CPUFREQ_GOV_POLICY_EXIT)
policy->governor->initialized--;
+ else if (event == CPUFREQ_GOV_STOP)
+ policy->governor->enabled = 0;
+ else if (event == CPUFREQ_GOV_START)
+ policy->governor->enabled = 1;
}

/* we keep one module reference alive for
@@ -1581,7 +1590,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
return ret;
}

-
int cpufreq_register_governor(struct cpufreq_governor *governor)
{
int err;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..16c5b70 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -199,6 +199,7 @@ struct cpufreq_governor {
will fallback to performance governor */
struct list_head governor_list;
struct module *owner;
+ int enabled;
};

/*
--
1.8.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Viresh Kumar]

This isn't sufficient.

If there are two groups of clk-sharing-cpus, i.e. if we have multiple
policies and they are using same governor, then these functions
gets called twice for governor x. And you will return 0 for the second
policy.

[Xiaoguang Chen]

2013/6/10 Viresh Kumar <viresh...@linaro.org>:

So this enable flag should be per policy, right?
I'll modify this patch

Thanks
Xiaoguang

[Xiaoguang Chen]

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: do not allow stop one policy's governor multi-times
Governor stop should only do once for one policy, after it is stopped,

no other governor stop should be executed.

Signed-off-by: Xiaoguang Chen <che...@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 2d53f47..b4a2c94 100644

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1562,6 +1562,11 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

pr_debug("__cpufreq_governor for CPU %u, event %u\n",
policy->cpu, event);
+

+ if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
+ (policy->governor_enabled && (event == CPUFREQ_GOV_START)))

+ return 0;
+
ret = policy->governor->governor(policy, event);

if (!ret) {
@@ -1569,6 +1574,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
policy->governor->initialized++;
else if (event == CPUFREQ_GOV_POLICY_EXIT)
policy->governor->initialized--;
+ else if (event == CPUFREQ_GOV_STOP)

+ policy->governor_enabled = 0;

+ else if (event == CPUFREQ_GOV_START)

+ policy->governor_enabled = 1;

}

/* we keep one module reference alive for

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..c12db73 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -107,6 +107,7 @@ struct cpufreq_policy {
unsigned int policy; /* see above */
struct cpufreq_governor *governor; /* see below */
void *governor_data;
+ int governor_enabled; /* governor start/stop flag */

struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
1.8.0

[Viresh Kumar]

Few things:
- because __cpufreq_governor() isn't protected by locks, both calls for
stopping governor can reach to this point and race can still happen. So,
both may stop governor.
- Returning zero doesn't seems to be the right thing, as this may cause
STOP called by one user and START called by another. For example,
STOP happened due to governor change. STOP called for hotplug and
0 is returned. START called by hotplug as STOP was successful. :)

Maybe -EBUSY would make more sense here.

[Xiaoguang Chen]

2013/6/12 Viresh Kumar <viresh...@linaro.org>:

Ok, I'll think about how to protect this.

> - Returning zero doesn't seems to be the right thing, as this may cause
> STOP called by one user and START called by another. For example,
> STOP happened due to governor change. STOP called for hotplug and
> 0 is returned. START called by hotplug as STOP was successful. :)
>
> Maybe -EBUSY would make more sense here.

I thought about return one error code before. but I checked the code
and found that
most of the governor stop/start operations don't check the return
value. I can add error code
here, I think the checking of return value is also needed. May be
another patch can do this.

Thanks
Xiaoguang

[Viresh Kumar]

On 12 June 2013 14:13, Xiaoguang Chen <chenxg....@gmail.com> wrote:
> I thought about return one error code before. but I checked the code
> and found that
> most of the governor stop/start operations don't check the return
> value. I can add error code
> here, I think the checking of return value is also needed. May be
> another patch can do this.

Yeah.

[Xiaoguang Chen]

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: do not allow stop one policy's governor multi-times
Governor stop should only do once for one policy, after it is stopped,

no other governor stop should be executed. also add one mutext to
protect __cpufreq_governor so governor operation can be kept in sequence.

Signed-off-by: Xiaoguang Chen <che...@marvell.com>
---

drivers/cpufreq/cpufreq.c | 28 +++++++++++++++++++++++-----
include/linux/cpufreq.h | 1 +
2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..6c10cf0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
#endif
static DEFINE_RWLOCK(cpufreq_driver_lock);
+static DEFINE_MUTEX(cpufreq_governor_lock);

/*
* cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure
@@ -896,6 +897,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
goto module_out;
}

+
+
policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
if (!policy)
goto nomem_out;
@@ -1541,13 +1544,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
#else
struct cpufreq_governor *gov = NULL;
#endif
-
+ mutex_lock(&cpufreq_governor_lock);
if (policy->governor->max_transition_latency &&
policy->cpuinfo.transition_latency >
policy->governor->max_transition_latency) {
- if (!gov)
+ if (!gov) {
+ mutex_unlock(&cpufreq_governor_lock);
return -EINVAL;
- else {
+ } else {
printk(KERN_WARNING "%s governor failed, too long"
" transition latency of HW, fallback"
" to %s governor\n",
@@ -1557,11 +1561,19 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
}
}

- if (!try_module_get(policy->governor->owner))
+ if (!try_module_get(policy->governor->owner)) {
+ mutex_unlock(&cpufreq_governor_lock);
return -EINVAL;
-
+ }

pr_debug("__cpufreq_governor for CPU %u, event %u\n",
policy->cpu, event);
+
+ if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
+ (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {

+ mutex_unlock(&cpufreq_governor_lock);
+ return -EBUSY;
+ }

+
ret = policy->governor->governor(policy, event);

if (!ret) {

@@ -1569,6 +1581,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

policy->governor->initialized++;
else if (event == CPUFREQ_GOV_POLICY_EXIT)
policy->governor->initialized--;
+ else if (event == CPUFREQ_GOV_STOP)
+ policy->governor_enabled = 0;
+ else if (event == CPUFREQ_GOV_START)
+ policy->governor_enabled = 1;
}

/* we keep one module reference alive for

@@ -1578,6 +1594,8 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
if ((event == CPUFREQ_GOV_STOP) && !ret)
module_put(policy->governor->owner);

+ mutex_unlock(&cpufreq_governor_lock);
+
return ret;

}

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..c12db73 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -107,6 +107,7 @@ struct cpufreq_policy {
unsigned int policy; /* see above */
struct cpufreq_governor *governor; /* see below */
void *governor_data;
+ int governor_enabled; /* governor start/stop flag */

struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
1.8.0

[Viresh Kumar]

On 12 June 2013 14:39, Xiaoguang Chen <che...@marvell.com> wrote:

> ret = policy->governor->governor(policy, event);

We again reached to the same problem. We shouldn't call
this between taking locks, otherwise recursive locks problems
would be seen again.

[Xiaoguang Chen]

2013/6/12 Viresh Kumar <viresh...@linaro.org>:

> On 12 June 2013 14:39, Xiaoguang Chen <che...@marvell.com> wrote:
>
>> ret = policy->governor->governor(policy, event);
>
> We again reached to the same problem. We shouldn't call
> this between taking locks, otherwise recursive locks problems
> would be seen again.

But this is not the same lock as the deadlock case, it is a new lock,
and only used in this function. no other functions use this lock.
I don't know how can we get dead lock again?

Thanks
Xiaoguang

[Xiaoguang Chen]

2013/6/13 Viresh Kumar <viresh...@linaro.org>:

> On 13 June 2013 11:10, Xiaoguang Chen <chenxg....@gmail.com> wrote:
>> 2013/6/12 Viresh Kumar <viresh...@linaro.org>:
>>> On 12 June 2013 14:39, Xiaoguang Chen <che...@marvell.com> wrote:
>>>
>>>> ret = policy->governor->governor(policy, event);
>>>
>>> We again reached to the same problem. We shouldn't call
>>> this between taking locks, otherwise recursive locks problems
>>> would be seen again.
>>
>> But this is not the same lock as the deadlock case, it is a new lock,
>> and only used in this function. no other functions use this lock.
>> I don't know how can we get dead lock again?
>

> I believe I have seen the recursive lock issue with different locks but
> I am not sure. Anyway, I believe the implementation can be simpler than
> that.
>
> Check below patch (attached too):
>
> ------------x------------------x----------------
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2d53f47..80b9798 100644

> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *,
> cpufreq_cpu_data);
> static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
> #endif
> static DEFINE_RWLOCK(cpufreq_driver_lock);
> +static DEFINE_MUTEX(cpufreq_governor_lock);
>
> /*
> * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure

> @@ -1541,7 +1542,6 @@ static int __cpufreq_governor(struct

> cpufreq_policy *policy,
> #else
> struct cpufreq_governor *gov = NULL;
> #endif
> -

> if (policy->governor->max_transition_latency &&
> policy->cpuinfo.transition_latency >
> policy->governor->max_transition_latency) {

> @@ -1562,6 +1562,21 @@ static int __cpufreq_governor(struct
> cpufreq_policy *policy,
>

> pr_debug("__cpufreq_governor for CPU %u, event %u\n",
> policy->cpu, event);
> +

> + mutex_lock(&cpufreq_governor_lock);

> + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
> + (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
> + mutex_unlock(&cpufreq_governor_lock);
> + return -EBUSY;
> + }
> +

> + if (event == CPUFREQ_GOV_STOP)

> + policy->governor_enabled = 0;
> + else if (event == CPUFREQ_GOV_START)
> + policy->governor_enabled = 1;

> +
> + mutex_unlock(&cpufreq_governor_lock);
> +

> ret = policy->governor->governor(policy, event);
>

> if (!ret) {
> @@ -1569,6 +1584,14 @@ static int __cpufreq_governor(struct

> cpufreq_policy *policy,
> policy->governor->initialized++;
> else if (event == CPUFREQ_GOV_POLICY_EXIT)
> policy->governor->initialized--;

> + } else {
> + /* Restore original values */
> + mutex_lock(&cpufreq_governor_lock);
> + if (event == CPUFREQ_GOV_STOP)
> + policy->governor_enabled = 1;

> + else if (event == CPUFREQ_GOV_START)

> + policy->governor_enabled = 0;
> + mutex_unlock(&cpufreq_governor_lock);

> }
>
> /* we keep one module reference alive for

> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 037d36a..c12db73 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -107,6 +107,7 @@ struct cpufreq_policy {
> unsigned int policy; /* see above */
> struct cpufreq_governor *governor; /* see below */
> void *governor_data;
> + int governor_enabled; /* governor start/stop flag */
>
> struct work_struct update; /* if update_policy() needs to be
> * called, but you're in IRQ context */

Thanks
So you add the return value checking, I was about to do it in another patch :)
this patch is simpler than my previous patch, it is ok for me.
Do I need to submit it again or it can be merged?

[Viresh Kumar]

On 13 June 2013 12:49, Xiaoguang Chen <chenxg....@gmail.com> wrote:

> So you add the return value checking, I was about to do it in another patch :)

What? I couldn't related that statement to my code.

> this patch is simpler than my previous patch, it is ok for me.
> Do I need to submit it again or it can be merged?

Yeah.. Send this patch as another version so that Rafael can easily pick it
up.

[Xiaoguang Chen]

drivers/cpufreq/cpufreq.c | 24 ++++++++++++++++++++++++
include/linux/cpufreq.h | 1 +
2 files changed, 25 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2d53f47..b51473e 100644

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
#endif
static DEFINE_RWLOCK(cpufreq_driver_lock);
+static DEFINE_MUTEX(cpufreq_governor_lock);

/*
* cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure

@@ -1562,6 +1563,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

pr_debug("__cpufreq_governor for CPU %u, event %u\n",
policy->cpu, event);
+
+ mutex_lock(&cpufreq_governor_lock);
+ if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
+ (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
+ mutex_unlock(&cpufreq_governor_lock);
+ return -EBUSY;
+ }
+
+ if (event == CPUFREQ_GOV_STOP)
+ policy->governor_enabled = 0;
+ else if (event == CPUFREQ_GOV_START)
+ policy->governor_enabled = 1;
+
+ mutex_unlock(&cpufreq_governor_lock);
+
ret = policy->governor->governor(policy, event);

if (!ret) {

@@ -1569,6 +1585,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,

policy->governor->initialized++;
else if (event == CPUFREQ_GOV_POLICY_EXIT)
policy->governor->initialized--;
+ } else {
+ /* Restore original values */
+ mutex_lock(&cpufreq_governor_lock);
+ if (event == CPUFREQ_GOV_STOP)
+ policy->governor_enabled = 1;
+ else if (event == CPUFREQ_GOV_START)
+ policy->governor_enabled = 0;
+ mutex_unlock(&cpufreq_governor_lock);
}

/* we keep one module reference alive for
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 037d36a..c12db73 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -107,6 +107,7 @@ struct cpufreq_policy {
unsigned int policy; /* see above */
struct cpufreq_governor *governor; /* see below */
void *governor_data;
+ int governor_enabled; /* governor start/stop flag */

struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */

--
1.8.0

[Viresh Kumar]

I forgot to tell you earlier but please update changelog everytime you
send a new version.

Acked-by: Viresh Kumar <viresh...@linaro.org>

[Xiaoguang Chen]

2013/6/13 Viresh Kumar <viresh...@linaro.org>:

Thanks
I'll pay attention next time :)

Xiaoguang

[Rafael J. Wysocki]

On Thursday, June 13, 2013 05:01:58 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.

Please spell cpus as "CPUs". And please start sequences from capitals.

[Yes, it *really* is a problem.]

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

Do I think correctly that this is for all CPUs?

> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will

Can you please tell me what the above is supposed to mean? Is it supposed to
mean "the online of cpu3 is being run on cpu0" or something different? If
something different, then what?

> 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

The problem is already here, right? The governor shouldn't be stopped twice?

> 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: do not allow stop one policy's governor multi-times
> Governor stop should only do once for one policy, after it is stopped,
> no other governor stop should be executed. also add one mutext to
> protect __cpufreq_governor so governor operation can be kept in sequence.

One more request. ->

-> Please use bool here and true/false instead of 1/0 above.

>
> struct work_struct update; /* if update_policy() needs to be
> * called, but you're in IRQ context */

Thanks,
Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Xiaoguang Chen]

2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:

> On Thursday, June 13, 2013 05:01:58 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.
>
> Please spell cpus as "CPUs". And please start sequences from capitals.

Ok, thanks for the remind

>
> [Yes, it *really* is a problem.]
>
>> 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.
>
> Do I think correctly that this is for all CPUs?

From current code design, it is for all CPUs.

>
>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
>
> Can you please tell me what the above is supposed to mean? Is it supposed to
> mean "the online of cpu3 is being run on cpu0" or something different? If
> something different, then what?
>
>> 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
>
> The problem is already here, right? The governor shouldn't be stopped twice?

Yes, we should make sure governor is started before it is stopped.

Ok, I'll change it to bool.

[Viresh Kumar]

On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:
> 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:
>> On Thursday, June 13, 2013 05:01:58 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.
>>
>> Please spell cpus as "CPUs". And please start sequences from capitals.
>
> Ok, thanks for the remind
>
>>
>> [Yes, it *really* is a problem.]

Just wanted to know the reasoning behind it so that I can remind
others about it and then argue :)

>>> 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.
>>
>> Do I think correctly that this is for all CPUs?
>
> From current code design, it is for all CPUs.

Why? This can be for a single cpu (which would eventually force all
others CPUs sharing policy with it).

[Viresh Kumar]

On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:

> 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:

>>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
>>
>> Can you please tell me what the above is supposed to mean? Is it supposed to
>> mean "the online of cpu3 is being run on cpu0" or something different? If
>> something different, then what?

Please read all the questions carefully. You missed this one.

Actually you should write: Current governor is userspace, now cpu0 hot-unplugs
cpu3, it will **

[Viresh Kumar]

On 19 June 2013 08:43, Viresh Kumar <viresh...@linaro.org> wrote:
> On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:
>> 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:
>
>>>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
>>>
>>> Can you please tell me what the above is supposed to mean? Is it supposed to
>>> mean "the online of cpu3 is being run on cpu0" or something different? If
>>> something different, then what?
>
> Please read all the questions carefully. You missed this one.
>
> Actually you should write: Current governor is userspace, now cpu0 hot-unplugs
> cpu3, it will **

Ahh I am mistaken, you are actually bringing cpu3 back to the system. Then you
must have mentioned earlier that cpu3 wasn't online.

[Xiaoguang Chen]

2013/6/19 Viresh Kumar <viresh...@linaro.org>:

> On 19 June 2013 08:43, Viresh Kumar <viresh...@linaro.org> wrote:
>> On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:
>>> 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:
>>
>>>>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
>>>>
>>>> Can you please tell me what the above is supposed to mean? Is it supposed to
>>>> mean "the online of cpu3 is being run on cpu0" or something different? If
>>>> something different, then what?

Sorry I missed this question, Let me explain it in detail
Suppose we are in such condtition, current cpufreq goveror is
userspace governor. cpu3 is offline.
and two things happen as above two cases, first thing is application
tries to change current governor to ondemand governor,
second thing is cpu0 tries to make cpu3 online which is off line
before. both of these two cases will try to stop current governor and
start a governor. if above two things interleave, unexpected behavior
will happen.

[Rafael J. Wysocki]

On Wednesday, June 19, 2013 08:30:11 AM Viresh Kumar wrote:
> On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:
> > 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:
> >> On Thursday, June 13, 2013 05:01:58 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.
> >>
> >> Please spell cpus as "CPUs". And please start sequences from capitals.
> >
> > Ok, thanks for the remind
> >
> >>
> >> [Yes, it *really* is a problem.]
>
> Just wanted to know the reasoning behind it so that I can remind
> others about it and then argue :)

Well, sentences that don't start from upper-case letters don't look like
separate sentences and when I see "cpus" written like this I tend to read
it as "cups" with a typo.

Generally speaking, don't make changelogs harder to read than they have to be.

They usually are hard enough to read anyway.

Thanks,
Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Rafael J. Wysocki]

On Wednesday, June 19, 2013 11:26:13 AM Xiaoguang Chen wrote:
> 2013/6/19 Viresh Kumar <viresh...@linaro.org>:
> > On 19 June 2013 08:43, Viresh Kumar <viresh...@linaro.org> wrote:
> >> On 19 June 2013 06:50, Xiaoguang Chen <chenxg....@gmail.com> wrote:
> >>> 2013/6/19 Rafael J. Wysocki <r...@sisk.pl>:
> >>
> >>>>> 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will
> >>>>
> >>>> Can you please tell me what the above is supposed to mean? Is it supposed to
> >>>> mean "the online of cpu3 is being run on cpu0" or something different? If
> >>>> something different, then what?
>
> Sorry I missed this question, Let me explain it in detail
> Suppose we are in such condtition, current cpufreq goveror is
> userspace governor. cpu3 is offline.
> and two things happen as above two cases, first thing is application
> tries to change current governor to ondemand governor,
> second thing is cpu0 tries to make cpu3 online which is off line
> before. both of these two cases will try to stop current governor and
> start a governor. if above two things interleave, unexpected behavior
> will happen.

Now it's clear, thanks for the explanation. [Well, I'll still need to fix up
the changelog. Sigh.]

Thanks,
Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Viresh Kumar]

On 20 June 2013 04:19, Rafael J. Wysocki <r...@sisk.pl> wrote:

> Well, sentences that don't start from upper-case letters don't look like
> separate sentences

s/sequences/sentences for your initial mail.

I misunderstood it as, you want to name cpu{0|1|2} as CPU{1|2|3}..
Obviously sentences must start with capitals.

> and when I see "cpus" written like this I tend to read
> it as "cups" with a typo.

Hmm..