[PATCH 1/2] cpufreq: don't allow governor limits to be changed when it is disabled

[PATCH 1/2] cpufreq: don't allow governor limits to be changed when it is disabled

[Viresh Kumar]

__cpufreq_governor() returns with -EBUSY when governor is already stopped and we
try to stop it again, but when it is stopped we must not allow calls to
CPUFREQ_GOV_LIMITS event as well.

This patch adds this check in __cpufreq_governor().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Rafael,

Its better if we can get these in for 3.11, otherwise we need to get them in the
stable tree..

Anyway, we will get these in 3.10 stable tree but that requires us to identify
few more patches that will go with these. I will do that later.

This must fix the issues reported by Stephen.

Tested on my thinkpad over your linux-next branch.

 drivers/cpufreq/cpufreq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..f320a20 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,8 +1692,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 
 	mutex_lock(&cpufreq_governor_lock);
-	if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
-	    (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
+	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
+		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
+					       (event == CPUFREQ_GOV_STOP)))) {
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy.

Lets introduce another variable which would guarantee serialization here.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++++-
 include/linux/cpufreq.h   | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 
 	mutex_lock(&cpufreq_governor_lock);
-	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
+	if (policy->governor_busy ||
+		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
 		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
 					       (event == CPUFREQ_GOV_STOP)))) {
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
 
+	policy->governor_busy = true;
 	if (event == CPUFREQ_GOV_STOP)
 		policy->governor_enabled = false;
 	else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
 		module_put(policy->governor->owner);
 
+	mutex_lock(&cpufreq_governor_lock);
+	policy->governor_busy = false;
+	mutex_unlock(&cpufreq_governor_lock);
 	return ret;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..cca885d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -76,6 +76,7 @@ struct cpufreq_policy {
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	bool			governor_enabled; /* governor start/stop flag */
+	bool			governor_busy;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 1/2] cpufreq: don't allow governor limits to be changed when it is disabled

[Viresh Kumar]

On 1 September 2013 10:56, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Its better if we can get these in for 3.11, otherwise we need to get them in the
> stable tree..

They can't get into 3.11 due to two reasons..
- I haven't tested them over 3.11 but linux-next/master.
- Few more dependency patches might also be required for 3.11..

So get them into 3.12-rc1 if they look okay to you :)

Re: [PATCH 1/2] cpufreq: don't allow governor limits to be changed when it is disabled

[Rafael J. Wysocki]

On Sunday, September 01, 2013 10:56:01 AM Viresh Kumar wrote:
> __cpufreq_governor() returns with -EBUSY when governor is already stopped and we
> try to stop it again, but when it is stopped we must not allow calls to
> CPUFREQ_GOV_LIMITS event as well.
> 
> This patch adds this check in __cpufreq_governor().
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Hi Rafael,
> 
> Its better if we can get these in for 3.11, otherwise we need to get them in the
> stable tree..

There's no way they could go into 3.11 or even 3.12 without speding time
in linux-next.  I'll queue them up for the second part of the 3.12 merge
window, unless there is 3.11-rc8 (which I doubt will happen).

> Anyway, we will get these in 3.10 stable tree but that requires us to identify
> few more patches that will go with these. I will do that later.
> 
> This must fix the issues reported by Stephen.
> 
> Tested on my thinkpad over your linux-next branch.
> 
>  drivers/cpufreq/cpufreq.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5c75e31..f320a20 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,8 +1692,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  						policy->cpu, event);
>  
>  	mutex_lock(&cpufreq_governor_lock);
> -	if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
> -	    (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
> +	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> +		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> +					       (event == CPUFREQ_GOV_STOP)))) {

Broken white space, but never mind.

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

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
> We can't take a big lock around __cpufreq_governor() as this causes recursive
> locking for some cases. But calls to this routine must be serialized for every
> policy.

Care to explain here why it must be serialized?

> Lets introduce another variable which would guarantee serialization here.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>  include/linux/cpufreq.h   | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f320a20..4d5723db 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  						policy->cpu, event);
>  
>  	mutex_lock(&cpufreq_governor_lock);
> -	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> +	if (policy->governor_busy ||
> +		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||

Again, broken white space, but I can fix it up.

>  		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
>  					       (event == CPUFREQ_GOV_STOP)))) {
>  		mutex_unlock(&cpufreq_governor_lock);
>  		return -EBUSY;
>  	}
>  
> +	policy->governor_busy = true;
>  	if (event == CPUFREQ_GOV_STOP)
>  		policy->governor_enabled = false;
>  	else if (event == CPUFREQ_GOV_START)
> @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
>  		module_put(policy->governor->owner);
>  
> +	mutex_lock(&cpufreq_governor_lock);
> +	policy->governor_busy = false;
> +	mutex_unlock(&cpufreq_governor_lock);
>  	return ret;
>  }
>  
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d568f39..cca885d 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -76,6 +76,7 @@ struct cpufreq_policy {
>  	struct cpufreq_governor	*governor; /* see below */
>  	void			*governor_data;
>  	bool			governor_enabled; /* governor start/stop flag */
> +	bool			governor_busy;
>  
>  	struct work_struct	update; /* if update_policy() needs to be
>  					 * called, but you're in IRQ context */
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] cpufreq: don't allow governor limits to be changed when it is disabled

[Rafael J. Wysocki]

On Sunday, September 01, 2013 11:56:09 AM Viresh Kumar wrote:
> On 1 September 2013 10:56, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Its better if we can get these in for 3.11, otherwise we need to get them in the
> > stable tree..
> 
> They can't get into 3.11 due to two reasons..
> - I haven't tested them over 3.11 but linux-next/master.
> - Few more dependency patches might also be required for 3.11..
> 
> So get them into 3.12-rc1 if they look okay to you :)

I'm going to do that, but please add explanation to the [2/2] changelog.

Thanks!

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

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On 1 September 2013 18:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
>> We can't take a big lock around __cpufreq_governor() as this causes recursive
>> locking for some cases. But calls to this routine must be serialized for every
>> policy.
>
> Care to explain here why it must be serialized?

Yes, added that to the attached patches (Added reported-by too):

commit dc51771506b113b998c49c3be2db0fb88bb92153
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Sat Aug 31 17:48:23 2013 +0530

    cpufreq: serialize calls to __cpufreq_governor()

    We can't take a big lock around __cpufreq_governor() as this
causes recursive
    locking for some cases. But calls to this routine must be
serialized for every
    policy. Otherwise we can see some unpredictable events.

    For example, consider following scenario:

    __cpufreq_remove_dev()
     __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
       policy->governor->governor(policy, CPUFREQ_GOV_STOP);
        cpufreq_governor_dbs()
         case CPUFREQ_GOV_STOP:
          mutex_destroy(&cpu_cdbs->timer_mutex)
          cpu_cdbs->cur_policy = NULL;
      <PREEMPT>
    store()
     __cpufreq_set_policy()
      __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
        policy->governor->governor(policy, CPUFREQ_GOV_LIMITS);
         case CPUFREQ_GOV_LIMITS:
          mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex)
           if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL

    And so store() will eventually result in a crash cur_policy is already NULL;

    Lets introduce another variable which would guarantee serialization here.

   Reported-by: Stephen Boyd <sboyd@codeaurora.org>
   Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

>> Lets introduce another variable which would guarantee serialization here.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>>  include/linux/cpufreq.h   | 1 +
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index f320a20..4d5723db 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>>                                               policy->cpu, event);
>>
>>       mutex_lock(&cpufreq_governor_lock);
>> -     if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>> +     if (policy->governor_busy ||
>> +             (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>
> Again, broken white space, but I can fix it up.

Sorry, what exactly.. Sorry couldn't understand it :(

[-- Attachment #2: 0001-cpufreq-don-t-allow-governor-limits-to-be-changed-wh.patch --]
[-- Type: application/octet-stream, Size: 1497 bytes --]

From 085013f4e584e3fef97187bcb349c3fa76942e19 Mon Sep 17 00:00:00 2001
Message-Id: <085013f4e584e3fef97187bcb349c3fa76942e19.1378051016.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Sat, 31 Aug 2013 17:53:40 +0530
Subject: [PATCH 1/2] cpufreq: don't allow governor limits to be changed when
 it is disabled

__cpufreq_governor() returns with -EBUSY when governor is already stopped and we
try to stop it again, but when it is stopped we must not allow calls to
CPUFREQ_GOV_LIMITS event as well.

This patch adds this check in __cpufreq_governor().

Reported-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..f320a20 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,8 +1692,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 
 	mutex_lock(&cpufreq_governor_lock);
-	if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
-	    (policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
+	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
+		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
+					       (event == CPUFREQ_GOV_STOP)))) {
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
-- 
1.7.12.rc2.18.g61b472e


[-- Attachment #3: 0002-cpufreq-serialize-calls-to-__cpufreq_governor.patch --]
[-- Type: application/octet-stream, Size: 3336 bytes --]

From dc51771506b113b998c49c3be2db0fb88bb92153 Mon Sep 17 00:00:00 2001
Message-Id: <dc51771506b113b998c49c3be2db0fb88bb92153.1378051016.git.viresh.kumar@linaro.org>
In-Reply-To: <085013f4e584e3fef97187bcb349c3fa76942e19.1378051016.git.viresh.kumar@linaro.org>
References: <085013f4e584e3fef97187bcb349c3fa76942e19.1378051016.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Sat, 31 Aug 2013 17:48:23 +0530
Subject: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy. Otherwise we can see some unpredictable events.

For example, consider following scenario:

__cpufreq_remove_dev()
 __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
   policy->governor->governor(policy, CPUFREQ_GOV_STOP);
    cpufreq_governor_dbs()
     case CPUFREQ_GOV_STOP:
      mutex_destroy(&cpu_cdbs->timer_mutex)
      cpu_cdbs->cur_policy = NULL;
  <PREEMPT>
store()
 __cpufreq_set_policy()
  __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
    policy->governor->governor(policy, CPUFREQ_GOV_LIMITS);
     case CPUFREQ_GOV_LIMITS:
      mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex)
       if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL

And so store() will eventually result in a crash cur_policy is already NULL;

Lets introduce another variable which would guarantee serialization here.

Reported-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++++-
 include/linux/cpufreq.h   | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 
 	mutex_lock(&cpufreq_governor_lock);
-	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
+	if (policy->governor_busy ||
+		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
 		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
 					       (event == CPUFREQ_GOV_STOP)))) {
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
 
+	policy->governor_busy = true;
 	if (event == CPUFREQ_GOV_STOP)
 		policy->governor_enabled = false;
 	else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
 		module_put(policy->governor->owner);
 
+	mutex_lock(&cpufreq_governor_lock);
+	policy->governor_busy = false;
+	mutex_unlock(&cpufreq_governor_lock);
 	return ret;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..cca885d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -76,6 +76,7 @@ struct cpufreq_policy {
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	bool			governor_enabled; /* governor start/stop flag */
+	bool			governor_busy;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote:
> On 1 September 2013 18:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
> >> We can't take a big lock around __cpufreq_governor() as this causes recursive
> >> locking for some cases. But calls to this routine must be serialized for every
> >> policy.
> >
> > Care to explain here why it must be serialized?
> 
> Yes, added that to the attached patches (Added reported-by too):
> 
> commit dc51771506b113b998c49c3be2db0fb88bb92153
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Sat Aug 31 17:48:23 2013 +0530
> 
>     cpufreq: serialize calls to __cpufreq_governor()
> 
>     We can't take a big lock around __cpufreq_governor() as this
> causes recursive
>     locking for some cases. But calls to this routine must be
> serialized for every
>     policy. Otherwise we can see some unpredictable events.
> 
>     For example, consider following scenario:
> 
>     __cpufreq_remove_dev()
>      __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>        policy->governor->governor(policy, CPUFREQ_GOV_STOP);
>         cpufreq_governor_dbs()
>          case CPUFREQ_GOV_STOP:
>           mutex_destroy(&cpu_cdbs->timer_mutex)
>           cpu_cdbs->cur_policy = NULL;
>       <PREEMPT>
>     store()
>      __cpufreq_set_policy()
>       __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
>         policy->governor->governor(policy, CPUFREQ_GOV_LIMITS);
>          case CPUFREQ_GOV_LIMITS:
>           mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex)
>            if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL
> 
>     And so store() will eventually result in a crash cur_policy is already NULL;
> 
>     Lets introduce another variable which would guarantee serialization here.
> 
>    Reported-by: Stephen Boyd <sboyd@codeaurora.org>
>    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> >> Lets introduce another variable which would guarantee serialization here.
> >>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> ---
> >>  drivers/cpufreq/cpufreq.c | 7 ++++++-
> >>  include/linux/cpufreq.h   | 1 +
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index f320a20..4d5723db 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> >>                                               policy->cpu, event);
> >>
> >>       mutex_lock(&cpufreq_governor_lock);
> >> -     if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> >> +     if (policy->governor_busy ||
> >> +             (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> >
> > Again, broken white space, but I can fix it up.
> 
> Sorry, what exactly.. Sorry couldn't understand it :(

The second tab is one too many, I usually write such things like this:

	if (policy->governor_busy
	    || (policy->governor_enabled && event == CPUFREQ_GOV_START)
	    || ...

Then it is much easier to distinguish the conditional code from the condition
itself.


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

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

On 2 September 2013 01:57, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> The second tab is one too many, I usually write such things like this:
>
>         if (policy->governor_busy
>             || (policy->governor_enabled && event == CPUFREQ_GOV_START)
>             || ...
>
> Then it is much easier to distinguish the conditional code from the condition
> itself.

I see... I tried to do it this way but got confused again :)
You fix it this time and I will understand it from that :)

--
viresh

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote:
> On 1 September 2013 18:58, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
> >> We can't take a big lock around __cpufreq_governor() as this causes recursive
> >> locking for some cases. But calls to this routine must be serialized for every
> >> policy.
> >
> > Care to explain here why it must be serialized?
> 
> Yes, added that to the attached patches (Added reported-by too):

OK, the attached patches queued up for 3.12.

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Srivatsa S. Bhat]

On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> We can't take a big lock around __cpufreq_governor() as this causes recursive
> locking for some cases. But calls to this routine must be serialized for every
> policy.
> 
> Lets introduce another variable which would guarantee serialization here.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>  include/linux/cpufreq.h   | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f320a20..4d5723db 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  						policy->cpu, event);
> 
>  	mutex_lock(&cpufreq_governor_lock);
> -	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> +	if (policy->governor_busy ||
> +		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>  		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
>  					       (event == CPUFREQ_GOV_STOP)))) {
>  		mutex_unlock(&cpufreq_governor_lock);
>  		return -EBUSY;
>  	}
> 
> +	policy->governor_busy = true;
>  	if (event == CPUFREQ_GOV_STOP)
>  		policy->governor_enabled = false;
>  	else if (event == CPUFREQ_GOV_START)
> @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>  			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
>  		module_put(policy->governor->owner);
> 
> +	mutex_lock(&cpufreq_governor_lock);
> +	policy->governor_busy = false;
> +	mutex_unlock(&cpufreq_governor_lock);
>  	return ret;
>  }
> 

This doesn't solve the problem completely: it prevents the store_*() task
from continuing *only* when it concurrently executes the __cpufreq_governor()
function along with the CPU offline task. But if the two calls don't overlap,
we will still have the possibility where the store_*() task tries to acquire
the timer mutex after the CPU offline task has just finished destroying it.

So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
store_*() thread will wait until the entire CPU offline operation is completed.
After that, if it continues, it will get -EBUSY, due to patch [1], since
policy->governor_enabled will be set to false.

[1]. https://patchwork.kernel.org/patch/2852463/

So here is the (completely untested) fix that I propose, as a replacement to
this patch [2/2]:


diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..71c4fb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -440,17 +440,24 @@ static ssize_t store_##file_name					\
 	unsigned int ret;						\
 	struct cpufreq_policy new_policy;				\
 									\
+	get_online_cpus();						\
 	ret = cpufreq_get_policy(&new_policy, policy->cpu);		\
-	if (ret)							\
-		return -EINVAL;						\
+	if (ret) {							\
+		ret = -EINVAL;						\
+		goto out;						\
+	}								\
 									\
 	ret = sscanf(buf, "%u", &new_policy.object);			\
-	if (ret != 1)							\
-		return -EINVAL;						\
+	if (ret != 1) {							\
+		ret = -EINVAL;						\
+		goto out;						\
+	}								\
 									\
 	ret = __cpufreq_set_policy(policy, &new_policy);		\
 	policy->user_policy.object = policy->object;			\
 									\
+out:									\
+	put_online_cpus();						\
 	return ret ? ret : count;					\
 }
 
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 	char	str_governor[16];
 	struct cpufreq_policy new_policy;
 
+	get_online_cpus();
 	ret = cpufreq_get_policy(&new_policy, policy->cpu);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = sscanf(buf, "%15s", str_governor);
-	if (ret != 1)
-		return -EINVAL;
+	if (ret != 1) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	if (cpufreq_parse_governor(str_governor, &new_policy.policy,
-						&new_policy.governor))
-		return -EINVAL;
+						&new_policy.governor)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/*
 	 * Do not use cpufreq_set_policy here or the user_policy.max
@@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 	policy->user_policy.policy = policy->policy;
 	policy->user_policy.governor = policy->governor;
 
+out:
+	put_online_cpus();
+
 	if (ret)
 		return ret;
 	else


Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> > We can't take a big lock around __cpufreq_governor() as this causes recursive
> > locking for some cases. But calls to this routine must be serialized for every
> > policy.
> > 
> > Lets introduce another variable which would guarantee serialization here.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 7 ++++++-
> >  include/linux/cpufreq.h   | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index f320a20..4d5723db 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> >  						policy->cpu, event);
> > 
> >  	mutex_lock(&cpufreq_governor_lock);
> > -	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > +	if (policy->governor_busy ||
> > +		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> >  		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> >  					       (event == CPUFREQ_GOV_STOP)))) {
> >  		mutex_unlock(&cpufreq_governor_lock);
> >  		return -EBUSY;
> >  	}
> > 
> > +	policy->governor_busy = true;
> >  	if (event == CPUFREQ_GOV_STOP)
> >  		policy->governor_enabled = false;
> >  	else if (event == CPUFREQ_GOV_START)
> > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> >  			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> >  		module_put(policy->governor->owner);
> > 
> > +	mutex_lock(&cpufreq_governor_lock);
> > +	policy->governor_busy = false;
> > +	mutex_unlock(&cpufreq_governor_lock);
> >  	return ret;
> >  }
> > 
> 
> This doesn't solve the problem completely: it prevents the store_*() task
> from continuing *only* when it concurrently executes the __cpufreq_governor()
> function along with the CPU offline task. But if the two calls don't overlap,
> we will still have the possibility where the store_*() task tries to acquire
> the timer mutex after the CPU offline task has just finished destroying it.

Yeah, I overlooked that.

> So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
> store_*() thread will wait until the entire CPU offline operation is completed.
> After that, if it continues, it will get -EBUSY, due to patch [1], since
> policy->governor_enabled will be set to false.
> 
> [1]. https://patchwork.kernel.org/patch/2852463/
> 
> So here is the (completely untested) fix that I propose, as a replacement to
> this patch [2/2]:

That looks reasonable to me.

Viresh?

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5c75e31..71c4fb2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -440,17 +440,24 @@ static ssize_t store_##file_name					\
>  	unsigned int ret;						\
>  	struct cpufreq_policy new_policy;				\
>  									\
> +	get_online_cpus();						\
>  	ret = cpufreq_get_policy(&new_policy, policy->cpu);		\
> -	if (ret)							\
> -		return -EINVAL;						\
> +	if (ret) {							\
> +		ret = -EINVAL;						\
> +		goto out;						\
> +	}								\
>  									\
>  	ret = sscanf(buf, "%u", &new_policy.object);			\
> -	if (ret != 1)							\
> -		return -EINVAL;						\
> +	if (ret != 1) {							\
> +		ret = -EINVAL;						\
> +		goto out;						\
> +	}								\
>  									\
>  	ret = __cpufreq_set_policy(policy, &new_policy);		\
>  	policy->user_policy.object = policy->object;			\
>  									\
> +out:									\
> +	put_online_cpus();						\
>  	return ret ? ret : count;					\
>  }
>  
> @@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
>  	char	str_governor[16];
>  	struct cpufreq_policy new_policy;
>  
> +	get_online_cpus();
>  	ret = cpufreq_get_policy(&new_policy, policy->cpu);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	ret = sscanf(buf, "%15s", str_governor);
> -	if (ret != 1)
> -		return -EINVAL;
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	if (cpufreq_parse_governor(str_governor, &new_policy.policy,
> -						&new_policy.governor))
> -		return -EINVAL;
> +						&new_policy.governor)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/*
>  	 * Do not use cpufreq_set_policy here or the user_policy.max
> @@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
>  	policy->user_policy.policy = policy->policy;
>  	policy->user_policy.governor = policy->governor;
>  
> +out:
> +	put_online_cpus();
> +
>  	if (ret)
>  		return ret;
>  	else
> 
> 
> Regards,
> Srivatsa S. Bhat
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Stephen Boyd]

On 09/04/13 16:55, Rafael J. Wysocki wrote:
>
> Well, I'm not sure when Viresh is going to be back.
>
> Srivatsa, can you please resend this patch with a proper changelog?
>

I haven't had a chance to try this out yet, but I was just thinking
about this patch. How is it going to work? If one task opens the file
and another task is taking down the CPU wouldn't we deadlock in the
CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
grab the kobject reference and sleep on the hotplug mutex and task 2
will put the kobject and wait for the completion, but it won't happen.
At least I think that's what would happen.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> > We can't take a big lock around __cpufreq_governor() as this causes recursive
> > locking for some cases. But calls to this routine must be serialized for every
> > policy.
> > 
> > Lets introduce another variable which would guarantee serialization here.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 7 ++++++-
> >  include/linux/cpufreq.h   | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index f320a20..4d5723db 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> >  						policy->cpu, event);
> > 
> >  	mutex_lock(&cpufreq_governor_lock);
> > -	if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > +	if (policy->governor_busy ||
> > +		(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> >  		(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> >  					       (event == CPUFREQ_GOV_STOP)))) {
> >  		mutex_unlock(&cpufreq_governor_lock);
> >  		return -EBUSY;
> >  	}
> > 
> > +	policy->governor_busy = true;
> >  	if (event == CPUFREQ_GOV_STOP)
> >  		policy->governor_enabled = false;
> >  	else if (event == CPUFREQ_GOV_START)
> > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> >  			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> >  		module_put(policy->governor->owner);
> > 
> > +	mutex_lock(&cpufreq_governor_lock);
> > +	policy->governor_busy = false;
> > +	mutex_unlock(&cpufreq_governor_lock);
> >  	return ret;
> >  }
> > 
> 
> This doesn't solve the problem completely: it prevents the store_*() task
> from continuing *only* when it concurrently executes the __cpufreq_governor()
> function along with the CPU offline task. But if the two calls don't overlap,
> we will still have the possibility where the store_*() task tries to acquire
> the timer mutex after the CPU offline task has just finished destroying it.
> 
> So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
> store_*() thread will wait until the entire CPU offline operation is completed.
> After that, if it continues, it will get -EBUSY, due to patch [1], since
> policy->governor_enabled will be set to false.
> 
> [1]. https://patchwork.kernel.org/patch/2852463/
> 
> So here is the (completely untested) fix that I propose, as a replacement to
> this patch [2/2]:
> 
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5c75e31..71c4fb2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -440,17 +440,24 @@ static ssize_t store_##file_name					\
>  	unsigned int ret;						\
>  	struct cpufreq_policy new_policy;				\
>  									\
> +	get_online_cpus();						\
>  	ret = cpufreq_get_policy(&new_policy, policy->cpu);		\
> -	if (ret)							\
> -		return -EINVAL;						\
> +	if (ret) {							\
> +		ret = -EINVAL;						\
> +		goto out;						\
> +	}								\
>  									\
>  	ret = sscanf(buf, "%u", &new_policy.object);			\
> -	if (ret != 1)							\
> -		return -EINVAL;						\
> +	if (ret != 1) {							\
> +		ret = -EINVAL;						\
> +		goto out;						\
> +	}								\
>  									\
>  	ret = __cpufreq_set_policy(policy, &new_policy);		\
>  	policy->user_policy.object = policy->object;			\
>  									\
> +out:									\
> +	put_online_cpus();						\
>  	return ret ? ret : count;					\
>  }
>  
> @@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
>  	char	str_governor[16];
>  	struct cpufreq_policy new_policy;
>  
> +	get_online_cpus();
>  	ret = cpufreq_get_policy(&new_policy, policy->cpu);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	ret = sscanf(buf, "%15s", str_governor);
> -	if (ret != 1)
> -		return -EINVAL;
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	if (cpufreq_parse_governor(str_governor, &new_policy.policy,
> -						&new_policy.governor))
> -		return -EINVAL;
> +						&new_policy.governor)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/*
>  	 * Do not use cpufreq_set_policy here or the user_policy.max
> @@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
>  	policy->user_policy.policy = policy->policy;
>  	policy->user_policy.governor = policy->governor;
>  
> +out:
> +	put_online_cpus();
> +
>  	if (ret)
>  		return ret;
>  	else

Well, I'm not sure when Viresh is going to be back.

Srivatsa, can you please resend this patch with a proper changelog?

Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
> On 09/04/13 16:55, Rafael J. Wysocki wrote:
> >
> > Well, I'm not sure when Viresh is going to be back.
> >
> > Srivatsa, can you please resend this patch with a proper changelog?
> >
> 
> I haven't had a chance to try this out yet, but I was just thinking
> about this patch. How is it going to work? If one task opens the file
> and another task is taking down the CPU wouldn't we deadlock in the
> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
> grab the kobject reference and sleep on the hotplug mutex and task 2
> will put the kobject and wait for the completion, but it won't happen.
> At least I think that's what would happen.

Do you mean the completion in sysfs_deactivate()?  Yes, we can deadlock
there.

Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock"
version of get_online_cpus(), but then I wonder if there's no better way.

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Stephen Boyd]

On 09/04/13 17:26, Rafael J. Wysocki wrote:
> On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
>> On 09/04/13 16:55, Rafael J. Wysocki wrote:
>>> Well, I'm not sure when Viresh is going to be back.
>>>
>>> Srivatsa, can you please resend this patch with a proper changelog?
>>>
>> I haven't had a chance to try this out yet, but I was just thinking
>> about this patch. How is it going to work? If one task opens the file
>> and another task is taking down the CPU wouldn't we deadlock in the
>> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
>> grab the kobject reference and sleep on the hotplug mutex and task 2
>> will put the kobject and wait for the completion, but it won't happen.
>> At least I think that's what would happen.
> Do you mean the completion in sysfs_deactivate()?  Yes, we can deadlock
> there.

I mean the complete in cpufreq_sysfs_release(). I don't think that will
ever be called because the kobject is held by the task calling store()
which is waiting on the hotplug lock to be released.

>
> Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock"
> version of get_online_cpus(), but then I wonder if there's no better way.

I think the real solution is to remove the kobject first if the CPU
going down is the last user of that policy. Once the completion is done
we can stop the governor and clean up state. For the case where there
are still CPUs using the policy why can't we cancel that CPU's work and
do nothing else? Even in the case where we have to move the cpufreq
directory do we need to do a STOP/START/LIMITS sequence? I hope we can
get away with just moving the directory and canceling that CPUs work then.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Srivatsa S. Bhat]

On 09/05/2013 06:24 AM, Stephen Boyd wrote:
> On 09/04/13 17:26, Rafael J. Wysocki wrote:
>> On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
>>> On 09/04/13 16:55, Rafael J. Wysocki wrote:
>>>> Well, I'm not sure when Viresh is going to be back.
>>>>
>>>> Srivatsa, can you please resend this patch with a proper changelog?
>>>>
>>> I haven't had a chance to try this out yet, but I was just thinking
>>> about this patch. How is it going to work? If one task opens the file
>>> and another task is taking down the CPU wouldn't we deadlock in the
>>> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
>>> grab the kobject reference and sleep on the hotplug mutex and task 2
>>> will put the kobject and wait for the completion, but it won't happen.
>>> At least I think that's what would happen.
>> Do you mean the completion in sysfs_deactivate()?  Yes, we can deadlock
>> there.
> 
> I mean the complete in cpufreq_sysfs_release(). I don't think that will
> ever be called because the kobject is held by the task calling store()
> which is waiting on the hotplug lock to be released.
> 
>>
>> Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock"
>> version of get_online_cpus(), but then I wonder if there's no better way.
> 
> I think the real solution is to remove the kobject first if the CPU
> going down is the last user of that policy. Once the completion is done
> we can stop the governor and clean up state. For the case where there
> are still CPUs using the policy why can't we cancel that CPU's work and
> do nothing else? Even in the case where we have to move the cpufreq
> directory do we need to do a STOP/START/LIMITS sequence? I hope we can
> get away with just moving the directory and canceling that CPUs work then.
> 

Conceptually, I agree that your idea of not allowing any process to take
a new reference to the kobject while we are taking the CPU offline, is a
sound solution.

However, I am reluctant to go down that path because, handling the CPU hotplug
sequence in the suspend/resume path might get even more tricky, if we want
to implement the changes that you propose. Just recently we managed to
rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
around suspend/resume, and doing that was not at all simple. Adding more
quirks and complexity to the kobject handling in that path will only make
things even more challenging, IMHO. That's the reason I'm trying to think
of ways to avoid touching that fragile code, and instead solve this problem
in some other way, without compromising on the robustness of the solution.

So here is my new proposal, as a replacement to this patch[2/2]:

We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
cpu_online_mask, and also the cpu_hotplug lock is dropped.

So, let us split up __cpu_remove_dev() into 2 parts, say:
__cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
__cpu_remove_finish()  - invoked during CPU_POST_DEAD


In the former function, we stop the governors, so that policy->governor_enabled
gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
->store() requests. Also, we do everything except the kobject cleanup.

In the latter function, we do the remaining work, particularly the part
where we wait for the kobject refcount to drop to zero and the subsequent
cleanup.


And the ->store() functions will be modified to look like this:

store()
{
	get_online_cpus();

	if (!cpu_online(cpu))
		goto out;

	/* Body of the function*/
out:
	put_online_cpus();
}

That way, if a task tries to write to a cpufreq file during CPU offline,
it will get blocked on get_online_cpus(), and will continue after
CPU_DEAD (since we release the lock here). Then it will notice that the cpu
is offline, and hence will return silently, thus dropping the kobject refcnt.
So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
the kobject, it won't encounter any problems.

Any thoughts on this approach? I'll try to code it up and post the patch
later today.

Thank you!

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Friday, September 06, 2013 11:33:55 AM Srivatsa S. Bhat wrote:
> On 09/05/2013 06:24 AM, Stephen Boyd wrote:
> > On 09/04/13 17:26, Rafael J. Wysocki wrote:
> >> On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
> >>> On 09/04/13 16:55, Rafael J. Wysocki wrote:
> >>>> Well, I'm not sure when Viresh is going to be back.
> >>>>
> >>>> Srivatsa, can you please resend this patch with a proper changelog?
> >>>>
> >>> I haven't had a chance to try this out yet, but I was just thinking
> >>> about this patch. How is it going to work? If one task opens the file
> >>> and another task is taking down the CPU wouldn't we deadlock in the
> >>> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
> >>> grab the kobject reference and sleep on the hotplug mutex and task 2
> >>> will put the kobject and wait for the completion, but it won't happen.
> >>> At least I think that's what would happen.
> >> Do you mean the completion in sysfs_deactivate()?  Yes, we can deadlock
> >> there.
> > 
> > I mean the complete in cpufreq_sysfs_release(). I don't think that will
> > ever be called because the kobject is held by the task calling store()
> > which is waiting on the hotplug lock to be released.
> > 
> >>
> >> Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock"
> >> version of get_online_cpus(), but then I wonder if there's no better way.
> > 
> > I think the real solution is to remove the kobject first if the CPU
> > going down is the last user of that policy. Once the completion is done
> > we can stop the governor and clean up state. For the case where there
> > are still CPUs using the policy why can't we cancel that CPU's work and
> > do nothing else? Even in the case where we have to move the cpufreq
> > directory do we need to do a STOP/START/LIMITS sequence? I hope we can
> > get away with just moving the directory and canceling that CPUs work then.
> > 
> 
> Conceptually, I agree that your idea of not allowing any process to take
> a new reference to the kobject while we are taking the CPU offline, is a
> sound solution.
> 
> However, I am reluctant to go down that path because, handling the CPU hotplug
> sequence in the suspend/resume path might get even more tricky, if we want
> to implement the changes that you propose. Just recently we managed to
> rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
> around suspend/resume, and doing that was not at all simple. Adding more
> quirks and complexity to the kobject handling in that path will only make
> things even more challenging, IMHO. That's the reason I'm trying to think
> of ways to avoid touching that fragile code, and instead solve this problem
> in some other way, without compromising on the robustness of the solution.
> 
> So here is my new proposal, as a replacement to this patch[2/2]:

Actually, I've already applied patch [2/2], because while it doesn't solve the
whole problem, it narrows it down somewhat.  So, please work on top of that
patch going forward.

> We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
> whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
> cpu_online_mask, and also the cpu_hotplug lock is dropped.
> 
> So, let us split up __cpu_remove_dev() into 2 parts, say:
> __cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
> __cpu_remove_finish()  - invoked during CPU_POST_DEAD
> 
> 
> In the former function, we stop the governors, so that policy->governor_enabled
> gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
> ->store() requests. Also, we do everything except the kobject cleanup.
> 
> In the latter function, we do the remaining work, particularly the part
> where we wait for the kobject refcount to drop to zero and the subsequent
> cleanup.
> 
> 
> And the ->store() functions will be modified to look like this:
> 
> store()
> {
> 	get_online_cpus();
> 
> 	if (!cpu_online(cpu))
> 		goto out;
> 
> 	/* Body of the function*/
> out:
> 	put_online_cpus();
> }
> 
> That way, if a task tries to write to a cpufreq file during CPU offline,
> it will get blocked on get_online_cpus(), and will continue after
> CPU_DEAD (since we release the lock here). Then it will notice that the cpu
> is offline, and hence will return silently, thus dropping the kobject refcnt.
> So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
> the kobject, it won't encounter any problems.
> 
> Any thoughts on this approach? I'll try to code it up and post the patch
> later today.

It sounds good to me (modulo the remark above).

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

Back finally and I see lots of mails over cpufreq stuff.. :)

On 3 September 2013 18:50, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> This doesn't solve the problem completely: it prevents the store_*() task
> from continuing *only* when it concurrently executes the __cpufreq_governor()
> function along with the CPU offline task. But if the two calls don't overlap,
> we will still have the possibility where the store_*() task tries to acquire
> the timer mutex after the CPU offline task has just finished destroying it.

How exactly? My brain is still on vacations :)

Anyway, this was one of the problem that I tried to solve with my patch.
But there can be other race conditions where things can go wrong and so
that patch may still be useful.

Call to __cpufreq_governor() must be serialized I believe.

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Srivatsa S. Bhat]

On 09/10/2013 12:22 PM, Viresh Kumar wrote:
> Back finally and I see lots of mails over cpufreq stuff.. :)
> 
> On 3 September 2013 18:50, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> This doesn't solve the problem completely: it prevents the store_*() task
>> from continuing *only* when it concurrently executes the __cpufreq_governor()
>> function along with the CPU offline task. But if the two calls don't overlap,
>> we will still have the possibility where the store_*() task tries to acquire
>> the timer mutex after the CPU offline task has just finished destroying it.
> 
> How exactly? My brain is still on vacations :)
>

The race window is not limited to __cpufreq_governor() alone. It just starts there,
but doesn't end there; it extends beyond that point. That's the main problem. And
that's why serializing __cpufreq_governor() won't completely solve the issue.

CPU_DOWN:

__cpufreq_governor(STOP)  ----+
  ...                         |
  ...                         | RACE
  ...                         | WINDOW
  ...                         |
sysfs teardown            ----+
 
In the STOP call, we destroy the timer mutex and set cur_policy = NULL. So the
incorrect access to timer mutex can occur from that point until we finally unlink
or teardown the sysfs files and prevent userspace from writing to those files.
Thus, this window involves stuff other than the call to __cpufreq_governor() as
well. So serializing __cpufreq_governor() alone is not enough.

> Anyway, this was one of the problem that I tried to solve with my patch.
> But there can be other race conditions where things can go wrong and so
> that patch may still be useful.
>

I agree, but see below (its the "may be useful" part that I'm not convinced
about).
 
> Call to __cpufreq_governor() must be serialized I believe.
> 

Sure, its a good idea to serialize __cpufreq_governor(). However, we need
specific justification for that. Just a hunch that something will go wrong is
not enough, IMHO. We have to *prove* that something is indeed wrong, and only
then add appropriate synchronization to fix it.

Besides, even the commit message appeared a bit odd. It mentions something about
problems with recursive calls. What cases are those? When do we end up recursively
calling __cpufreq_governor()?  And hence, why can't we use plain locks and must
instead use yet another quick-and-dirty variable to protect that function?
[Using variables like that is bad from a lockdep perspective as well (apart from
other things) : lockdep can't catch any resulting locking issues.]

Other related questions that are worth answering would be: why should we add the
synchronization *inside* __cpufreq_governor()? Why not add it at its call-sites?

IOW, I'm all in for fixing problems, just that I'd be comfortable with the
changes only when we completely understand what problem we are fixing and why
that patch is the right fix for that problem.

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

On 4 September 2013 01:10, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:

>> This doesn't solve the problem completely: it prevents the store_*() task
>> from continuing *only* when it concurrently executes the __cpufreq_governor()
>> function along with the CPU offline task. But if the two calls don't overlap,
>> we will still have the possibility where the store_*() task tries to acquire
>> the timer mutex after the CPU offline task has just finished destroying it.
>
> Yeah, I overlooked that.

As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked
this unread as there were other important topics to close)..

And he had some other code in mind and these synchronization problems
aren't there with my patch at all (as per him too)..

Rafael, probably both me and Srivatsa are missing something that you
understood, can you please share what problem you see here with my patch?

And yes, even with Srivatsa's patchset I found a problem:
Two threads, one changing governor from ondemand->conservative and other
one changing min/max freq..

First one will try to STOP governor and other one will try to change
limits of gov.
Suppose 2nd one gets to ->governor() and after this first one stops
the governor.
Now the first one tries to access lock and crashes..

On IRC, Srivatsa agreed about the problem..

So, probably the first thing to do is to get this patch back, i.e.
revert of Srivatsa's
patch:

commit 56d07db274b7b15ca38b60ea4a762d40de093000
Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Date:   Sat Sep 7 01:23:55 2013 +0530

    cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes


Srivatsa also asked if we can get a big lock around call to ->governor()
which would create recursive locks on a call to EXIT event (as we have seen it
earlier..)..

But he believes he can pull it off and will try this later.. So, for
now we can revert
the above patch :)

--
viresh

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Rafael J. Wysocki]

On Friday, September 13, 2013 05:14:26 PM Viresh Kumar wrote:
> On 4 September 2013 01:10, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> 
> >> This doesn't solve the problem completely: it prevents the store_*() task
> >> from continuing *only* when it concurrently executes the __cpufreq_governor()
> >> function along with the CPU offline task. But if the two calls don't overlap,
> >> we will still have the possibility where the store_*() task tries to acquire
> >> the timer mutex after the CPU offline task has just finished destroying it.
> >
> > Yeah, I overlooked that.
> 
> As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked
> this unread as there were other important topics to close)..
> 
> And he had some other code in mind and these synchronization problems
> aren't there with my patch at all (as per him too)..
> 
> Rafael, probably both me and Srivatsa are missing something that you
> understood, can you please share what problem you see here with my patch?
> 
> And yes, even with Srivatsa's patchset I found a problem:
> Two threads, one changing governor from ondemand->conservative and other
> one changing min/max freq..
> 
> First one will try to STOP governor and other one will try to change
> limits of gov.
> Suppose 2nd one gets to ->governor() and after this first one stops
> the governor.
> Now the first one tries to access lock and crashes..
> 
> On IRC, Srivatsa agreed about the problem..
> 
> So, probably the first thing to do is to get this patch back, i.e.
> revert of Srivatsa's
> patch:
> 
> commit 56d07db274b7b15ca38b60ea4a762d40de093000
> Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Date:   Sat Sep 7 01:23:55 2013 +0530
> 
>     cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes
> 
> 
> Srivatsa also asked if we can get a big lock around call to ->governor()
> which would create recursive locks on a call to EXIT event (as we have seen it
> earlier..)..
> 
> But he believes he can pull it off and will try this later.. So, for
> now we can revert
> the above patch :)

OK, I'll think about that, but not today and probably not within the next
few days, because I'm heading to New Orleans in several hours and really
have other stuff to take care of now.  Sorry about that.

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Viresh Kumar]

On 14 September 2013 05:18, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> OK, I'll think about that, but not today and probably not within the next
> few days, because I'm heading to New Orleans in several hours and really
> have other stuff to take care of now.  Sorry about that.

No issues..

Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

[Srivatsa S. Bhat]

On 09/13/2013 05:14 PM, Viresh Kumar wrote:
> On 4 September 2013 01:10, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> 
>>> This doesn't solve the problem completely: it prevents the store_*() task
>>> from continuing *only* when it concurrently executes the __cpufreq_governor()
>>> function along with the CPU offline task. But if the two calls don't overlap,
>>> we will still have the possibility where the store_*() task tries to acquire
>>> the timer mutex after the CPU offline task has just finished destroying it.
>>
>> Yeah, I overlooked that.
> 
> As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked
> this unread as there were other important topics to close)..
> 
> And he had some other code in mind and these synchronization problems
> aren't there with my patch at all (as per him too)..
> 
> Rafael, probably both me and Srivatsa are missing something that you
> understood, can you please share what problem you see here with my patch?
> 
> And yes, even with Srivatsa's patchset I found a problem:
> Two threads, one changing governor from ondemand->conservative and other
> one changing min/max freq..
> 
> First one will try to STOP governor and other one will try to change
> limits of gov.
> Suppose 2nd one gets to ->governor() and after this first one stops
> the governor.
> Now the first one tries to access lock and crashes..
> 
> On IRC, Srivatsa agreed about the problem..
> 

Actually, we had another round of discussions and decided that there is no
problem here. The reason is, the top-level store() routine takes the rwsem of
that policy for write; so only one store() can go at a time. So the race between
changing the governor and changing the min/max freq won't happen because only
one of them can execute at a given time. In fact, by extension, *any* two store()s
in cpufreq shouldn't race with one another.

So the conclusion is that the mainline code is fine in this aspect. Nothing
needs to be reverted.

That said, there are still certain low-priority/less-visible synchronization
issues to be fixed in cpufreq, and we'll take them up later, once the dust is
settled.

Regards,
Srivatsa S. Bhat