Re: [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

Re: [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

[Viresh Kumar]

Sorry for the really long delay this time around. I am used to replying within a
day normally, and this time it just took so much time.

For next time please rebase on latest updates in pm/linux-next as there are
few updates there.

On 25 July 2014 06:37, Saravana Kannan <skannan@codeaurora.org> wrote:
> There's no need to wait for the CPU going down to fully go offline to
> restart the governor. We can stop the governor, change policy->cpus and
> immediately restart the governor. This should reduce the time without any
> CPUfreq monitoring and also help future patches with simplifying the code.

I agree with the idea here, though the $subject can be improved a bit
here..

> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 62259d2..ee0eb7b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1390,6 +1390,21 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>                 cpufreq_driver->stop_cpu(policy);
>         }
>
> +       down_write(&policy->rwsem);
> +       cpumask_clear_cpu(cpu, policy->cpus);
> +       up_write(&policy->rwsem);

There is a down_read() present early in this routine and we better update this
at that place only.

> +       if (cpus > 1 && has_target()) {

We already have a if (cpus > 1) block, move this there.

> +               ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
> +               if (!ret)
> +                       ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> +
> +               if (ret) {
> +                       pr_err("%s: Failed to start governor\n", __func__);
> +                       return ret;
> +               }
> +       }
> +
>         return 0;
>  }
>
> @@ -1410,15 +1425,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>                 return -EINVAL;
>         }
>
> -       down_write(&policy->rwsem);
> +       down_read(&policy->rwsem);
>         cpus = cpumask_weight(policy->cpus);
> -
> -       if (cpus > 1)
> -               cpumask_clear_cpu(cpu, policy->cpus);
> -       up_write(&policy->rwsem);
> +       up_read(&policy->rwsem);
>
>         /* If cpu is last user of policy, free policy */
> -       if (cpus == 1) {
> +       if (cpus == 0) {
>                 if (has_target()) {
>                         ret = __cpufreq_governor(policy,
>                                         CPUFREQ_GOV_POLICY_EXIT);
> @@ -1447,15 +1459,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>
>                 if (!cpufreq_suspended)
>                         cpufreq_policy_free(policy);
> -       } else if (has_target()) {
> -               ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
> -               if (!ret)
> -                       ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> -
> -               if (ret) {
> -                       pr_err("%s: Failed to start governor\n", __func__);
> -                       return ret;
> -               }
>         }

Also, you must mention in the log about an important change you are making.
Don't know if there are any side effects...

You are emptying policy->cpus on removal of last CPU of a policy, which wasn't
the case earlier.

Re: [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

[Saravana Kannan]

On 08/07/2014 01:54 AM, Viresh Kumar wrote:
> Sorry for the really long delay this time around. I am used to replying within a
> day normally, and this time it just took so much time.
>
> For next time please rebase on latest updates in pm/linux-next as there are
> few updates there.

Will do.

>
> On 25 July 2014 06:37, Saravana Kannan <skannan@codeaurora.org> wrote:
>> There's no need to wait for the CPU going down to fully go offline to
>> restart the governor. We can stop the governor, change policy->cpus and
>> immediately restart the governor. This should reduce the time without any
>> CPUfreq monitoring and also help future patches with simplifying the code.
>
> I agree with the idea here, though the $subject can be improved a bit
> here..

Suggestions welcome. I think the current one explains the main point of 
this change.

>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>   drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++---------------
>>   1 file changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 62259d2..ee0eb7b 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1390,6 +1390,21 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>>                  cpufreq_driver->stop_cpu(policy);
>>          }
>>
>> +       down_write(&policy->rwsem);
>> +       cpumask_clear_cpu(cpu, policy->cpus);
>> +       up_write(&policy->rwsem);
>
> There is a down_read() present early in this routine and we better update this
> at that place only.

I would rather not. My v1 patch series was super refactored to allow a 
lot of reuse, etc. But you guys complained about the diffs being 
confusing (which was a valid point).

Also, if we are talking about refactoring this, there's room for much 
better refactor at the end of the series. I will add a patch to the 
series to do the refactoring.

>
>> +       if (cpus > 1 && has_target()) {
>
> We already have a if (cpus > 1) block, move this there.

That only runs if cpu != policy->cpu. This needs to run irrespective of 
that.

>
>> +               ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
>> +               if (!ret)
>> +                       ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
>> +
>> +               if (ret) {
>> +                       pr_err("%s: Failed to start governor\n", __func__);
>> +                       return ret;
>> +               }
>> +       }
>> +
>>          return 0;
>>   }
>>
>> @@ -1410,15 +1425,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>>                  return -EINVAL;
>>          }
>>
>> -       down_write(&policy->rwsem);
>> +       down_read(&policy->rwsem);
>>          cpus = cpumask_weight(policy->cpus);
>> -
>> -       if (cpus > 1)
>> -               cpumask_clear_cpu(cpu, policy->cpus);
>> -       up_write(&policy->rwsem);
>> +       up_read(&policy->rwsem);
>>
>>          /* If cpu is last user of policy, free policy */
>> -       if (cpus == 1) {
>> +       if (cpus == 0) {
>>                  if (has_target()) {
>>                          ret = __cpufreq_governor(policy,
>>                                          CPUFREQ_GOV_POLICY_EXIT);
>> @@ -1447,15 +1459,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>>
>>                  if (!cpufreq_suspended)
>>                          cpufreq_policy_free(policy);
>> -       } else if (has_target()) {
>> -               ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
>> -               if (!ret)
>> -                       ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
>> -
>> -               if (ret) {
>> -                       pr_err("%s: Failed to start governor\n", __func__);
>> -                       return ret;
>> -               }
>>          }
>
> Also, you must mention in the log about an important change you are making.
> Don't know if there are any side effects...
>
> You are emptying policy->cpus on removal of last CPU of a policy, which wasn't
> the case earlier.

You mean the log in the cover letter? Will do.

-Saravana

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

Re: [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

[Viresh Kumar]

On 12 August 2014 03:41, Saravana Kannan <skannan@codeaurora.org> wrote:

> Suggestions welcome. I think the current one explains the main point of this change.

Atleast we need a s/going/go

>> There is a down_read() present early in this routine and we better update this
>> at that place only.
>
>
> I would rather not. My v1 patch series was super refactored to allow a lot of reuse, etc. But you guys complained about the diffs being confusing (which was a valid point).
>
> Also, if we are talking about refactoring this, there's room for much better refactor at the end of the series. I will add a patch to the series to do the refactoring.

The kind of change I am suggesting you can be done in the original
patch only. What we told you in the earlier reviews was to break patches
into meaningful sections instead of doing everything in a single patch.

> That only runs if cpu != policy->cpu. This needs to run irrespective of that.

Oh yes, correct.

> You mean the log in the cover letter? Will do.

Coverletter isn't the right place for mentioning such important things
as it never gets commited. I was talking about the commit log.

[PATCH v4 0/5] Simplify hotplug/suspend handling

[Saravana Kannan]

Series of patchs to simplify policy/sysfs/kobj/locking handling across
suspend/resume

The following have been tested so far on a 2x2 cluster environment:
- Boot with 2 cpus and no cpufreq driver.
- mod probe driver and see cpufreq sysfs files show up only for the 1st cluster.
- Online the rest of the 2 CPUs and have files show up correctly.
- rmmod the driver and see the files go away.
- modprobe again (or back and forth multiples times) and see it work.
- suspend/resume works as expected.
- When a cluster is offline, all read/writes to its sysfs files return an error

v4
- Split it up into smaller patches
- Will handle physical CPU removal correctly
- Fixed earlier mistake of deleting code under !recover_policy
- Dropped some code refactor that reuses a lot of code between add/remove
- Dropped fix for exiting hotplug race with cpufreq driver probe/rmmod
- Dropped changes will come later once this series is acked.


Saravana Kannan (5):
  cpufreq: Don't wait for CPU to going offline to restart governor
  cpufreq: Keep track of which CPU owns the kobj/sysfs nodes separately
  cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend
  cpufreq: Properly handle physical CPU hot-add/hot-remove
  cpufreq: Delete dead code related to policy save/restore

 drivers/cpufreq/cpufreq.c | 238 ++++++++++++++++++----------------------------
 include/linux/cpufreq.h   |   1 +
 2 files changed, 93 insertions(+), 146 deletions(-)

-- 
1.8.2.1

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

[PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

[Saravana Kannan]

There's no need to wait for the CPU going down to fully go offline to
restart the governor. We can stop the governor, change policy->cpus and
immediately restart the governor. This should reduce the time without any
CPUfreq monitoring and also help future patches with simplifying the code.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 62259d2..ee0eb7b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1390,6 +1390,21 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 		cpufreq_driver->stop_cpu(policy);
 	}
 
+	down_write(&policy->rwsem);
+	cpumask_clear_cpu(cpu, policy->cpus);
+	up_write(&policy->rwsem);
+
+	if (cpus > 1 && has_target()) {
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+		if (!ret)
+			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+
+		if (ret) {
+			pr_err("%s: Failed to start governor\n", __func__);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -1410,15 +1425,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 		return -EINVAL;
 	}
 
-	down_write(&policy->rwsem);
+	down_read(&policy->rwsem);
 	cpus = cpumask_weight(policy->cpus);
-
-	if (cpus > 1)
-		cpumask_clear_cpu(cpu, policy->cpus);
-	up_write(&policy->rwsem);
+	up_read(&policy->rwsem);
 
 	/* If cpu is last user of policy, free policy */
-	if (cpus == 1) {
+	if (cpus == 0) {
 		if (has_target()) {
 			ret = __cpufreq_governor(policy,
 					CPUFREQ_GOV_POLICY_EXIT);
@@ -1447,15 +1459,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 
 		if (!cpufreq_suspended)
 			cpufreq_policy_free(policy);
-	} else if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
-		if (!ret)
-			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
-
-		if (ret) {
-			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
 	}
 
 	per_cpu(cpufreq_cpu_data, cpu) = NULL;
-- 
1.8.2.1

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

Re: [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor

[Saravana Kannan]

On 07/24/2014 06:07 PM, Saravana Kannan wrote:
> There's no need to wait for the CPU going down to fully go offline to
> restart the governor. We can stop the governor, change policy->cpus and
> immediately restart the governor. This should reduce the time without any
> CPUfreq monitoring and also help future patches with simplifying the code.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>   drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 62259d2..ee0eb7b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1390,6 +1390,21 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>   		cpufreq_driver->stop_cpu(policy);
>   	}
>
> +	down_write(&policy->rwsem);
> +	cpumask_clear_cpu(cpu, policy->cpus);
> +	up_write(&policy->rwsem);
> +
> +	if (cpus > 1 && has_target()) {
> +		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
> +		if (!ret)
> +			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> +
> +		if (ret) {
> +			pr_err("%s: Failed to start governor\n", __func__);
> +			return ret;
> +		}
> +	}
> +
>   	return 0;
>   }
>
> @@ -1410,15 +1425,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>   		return -EINVAL;
>   	}
>
> -	down_write(&policy->rwsem);
> +	down_read(&policy->rwsem);
>   	cpus = cpumask_weight(policy->cpus);
> -
> -	if (cpus > 1)
> -		cpumask_clear_cpu(cpu, policy->cpus);
> -	up_write(&policy->rwsem);
> +	up_read(&policy->rwsem);
>
>   	/* If cpu is last user of policy, free policy */
> -	if (cpus == 1) {
> +	if (cpus == 0) {
>   		if (has_target()) {
>   			ret = __cpufreq_governor(policy,
>   					CPUFREQ_GOV_POLICY_EXIT);
> @@ -1447,15 +1459,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>
>   		if (!cpufreq_suspended)
>   			cpufreq_policy_free(policy);
> -	} else if (has_target()) {
> -		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
> -		if (!ret)
> -			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> -
> -		if (ret) {
> -			pr_err("%s: Failed to start governor\n", __func__);
> -			return ret;
> -		}
>   	}
>
>   	per_cpu(cpufreq_cpu_data, cpu) = NULL;
>

This patch should also fix another issue reported in-house recently. 
cpufreq_update_policy() fails for an ONLINE CPU. This is the scenario 
that triggers it:

Thead A
- Cluster with 4 CPUs
- CPU3 is going down.
- Governor is STOPed.
- CPU3 is removed, but governor not STARTed yet.

Thread B
- get_online_cpus()
- We cross this hotplug barrier since since POST_DEAD is sent AFTER 
releasing the hotplug lock.
- cpufreq_update_policy(CPU0) does a bunch of stuff
- Then sends GOV_LIMITS to governor.
- governor is currently STOPed, so it returns an error and 
cpufreq_update_policy() fails.

Thread A
- In POST_DEAD notifier, STARTs the governor again.

So, a perfectly valid call (doing get_online_cpus() and checking for 
cpu_online() on a CPU before calling) fails.

-Saravana

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