Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

On 25-04-17, 11:19, Lukasz Luba wrote:
> Hi Viresh,
> 
> I have run the newest version (6a883ddf73cd).
> It looks correct (tests are passing so far).
> Feel free to add
> Tested-by: Lukasz Luba 
> 
> I would like to go through the code before
> it got merged, though.
> 
> If you are planing to post v4, I can test and review it
> this week.

Thanks a lot. I have just sent that.

-- 
viresh

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

On 25-04-17, 11:19, Lukasz Luba wrote:
> Hi Viresh,
> 
> I have run the newest version (6a883ddf73cd).
> It looks correct (tests are passing so far).
> Feel free to add
> Tested-by: Lukasz Luba 
> 
> I would like to go through the code before
> it got merged, though.
> 
> If you are planing to post v4, I can test and review it
> this week.

Thanks a lot. I have just sent that.

-- 
viresh

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Lukasz Luba]

Hi Viresh,

I have run the newest version (6a883ddf73cd).
It looks correct (tests are passing so far).
Feel free to add
Tested-by: Lukasz Luba 

I would like to go through the code before
it got merged, though.

If you are planing to post v4, I can test and review it
this week.

Regards,
Lukasz


On 25/04/17 05:57, Viresh Kumar wrote:

On 24-04-17, 17:53, Lukasz Luba wrote:

The policy pointer forwarded from cpufreq_update_policy()
is a local variable 'new_policy' so cannot be compared with pinned
policy pointer in the cooling device.
You should do the cpumask test like before:
if (!cpumask_test_cpu(policy->cpu,
  cpufreq_cdev->policy->related_cpus))


Right. I have fixed it a bit differently now.


But there is something still in the patch set...
I will try to check it tomorrow.


I reviewed all the patches very carefully again, trying to find out the culprit
(I don't have the right hardware to test it like you have).

Found out that max_level isn't used properly at few places, fixed and pushed my
branch now. See if it works fine now.

HEAD: 6a883ddf73cd

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Lukasz Luba]

Hi Viresh,

I have run the newest version (6a883ddf73cd).
It looks correct (tests are passing so far).
Feel free to add
Tested-by: Lukasz Luba 

I would like to go through the code before
it got merged, though.

If you are planing to post v4, I can test and review it
this week.

Regards,
Lukasz


On 25/04/17 05:57, Viresh Kumar wrote:

On 24-04-17, 17:53, Lukasz Luba wrote:

The policy pointer forwarded from cpufreq_update_policy()
is a local variable 'new_policy' so cannot be compared with pinned
policy pointer in the cooling device.
You should do the cpumask test like before:
if (!cpumask_test_cpu(policy->cpu,
  cpufreq_cdev->policy->related_cpus))


Right. I have fixed it a bit differently now.


But there is something still in the patch set...
I will try to check it tomorrow.


I reviewed all the patches very carefully again, trying to find out the culprit
(I don't have the right hardware to test it like you have).

Found out that max_level isn't used properly at few places, fixed and pushed my
branch now. See if it works fine now.

HEAD: 6a883ddf73cd

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

On 24-04-17, 17:53, Lukasz Luba wrote:
> The policy pointer forwarded from cpufreq_update_policy()
> is a local variable 'new_policy' so cannot be compared with pinned
> policy pointer in the cooling device.
> You should do the cpumask test like before:
>   if (!cpumask_test_cpu(policy->cpu,
> cpufreq_cdev->policy->related_cpus))

Right. I have fixed it a bit differently now.

> But there is something still in the patch set...
> I will try to check it tomorrow.

I reviewed all the patches very carefully again, trying to find out the culprit
(I don't have the right hardware to test it like you have).

Found out that max_level isn't used properly at few places, fixed and pushed my
branch now. See if it works fine now.

HEAD: 6a883ddf73cd

-- 
viresh

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

On 24-04-17, 17:53, Lukasz Luba wrote:
> The policy pointer forwarded from cpufreq_update_policy()
> is a local variable 'new_policy' so cannot be compared with pinned
> policy pointer in the cooling device.
> You should do the cpumask test like before:
>   if (!cpumask_test_cpu(policy->cpu,
> cpufreq_cdev->policy->related_cpus))

Right. I have fixed it a bit differently now.

> But there is something still in the patch set...
> I will try to check it tomorrow.

I reviewed all the patches very carefully again, trying to find out the culprit
(I don't have the right hardware to test it like you have).

Found out that max_level isn't used properly at few places, fixed and pushed my
branch now. See if it works fine now.

HEAD: 6a883ddf73cd

-- 
viresh

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Lukasz Luba]

Hi Viresh,

I have been testing the patch set and found one of the issues.
Please see the comment below.

On 19/04/17 06:29, Viresh Kumar wrote:

'allowed_cpus' is a copy of policy->related_cpus and can be replaced by
it directly. At some places we are only concerned about online CPUs and
policy->cpus can be used there.

Signed-off-by: Viresh Kumar 
---
 drivers/thermal/cpu_cooling.c | 77 ---
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ce387f62c93e..1097162f7f8a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -74,7 +74,6 @@ struct power_table {
  * frequency.
  * @max_level: maximum cooling level. One less than total number of valid
  * cpufreq frequencies.
- * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  * @node: list_head to link all cpufreq_cooling_device together.
  * @last_load: load measured by the latest call to 
cpufreq_get_requested_power()
  * @time_in_idle: previous reading of the absolute time that this cpu was idle
@@ -97,7 +96,6 @@ struct cpufreq_cooling_device {
unsigned int clipped_freq;
unsigned int max_level;
unsigned int *freq_table;   /* In descending order */
-   struct cpumask allowed_cpus;
struct list_head node;
u32 last_load;
u64 *time_in_idle;
@@ -161,7 +159,7 @@ static int cpufreq_thermal_notifier(struct notifier_block 
*nb,

mutex_lock(_list_lock);
list_for_each_entry(cpufreq_cdev, _cdev_list, node) {
-   if (!cpumask_test_cpu(policy->cpu, _cdev->allowed_cpus))
+   if (policy != cpufreq_cdev->policy)

The policy pointer forwarded from cpufreq_update_policy()
is a local variable 'new_policy' so cannot be compared with pinned
policy pointer in the cooling device.
You should do the cpumask test like before:
if (!cpumask_test_cpu(policy->cpu,
  cpufreq_cdev->policy->related_cpus))


But there is something still in the patch set...
I will try to check it tomorrow.

Best regards,
Lukasz

Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Lukasz Luba]

Hi Viresh,

I have been testing the patch set and found one of the issues.
Please see the comment below.

On 19/04/17 06:29, Viresh Kumar wrote:

'allowed_cpus' is a copy of policy->related_cpus and can be replaced by
it directly. At some places we are only concerned about online CPUs and
policy->cpus can be used there.

Signed-off-by: Viresh Kumar 
---
 drivers/thermal/cpu_cooling.c | 77 ---
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ce387f62c93e..1097162f7f8a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -74,7 +74,6 @@ struct power_table {
  * frequency.
  * @max_level: maximum cooling level. One less than total number of valid
  * cpufreq frequencies.
- * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  * @node: list_head to link all cpufreq_cooling_device together.
  * @last_load: load measured by the latest call to 
cpufreq_get_requested_power()
  * @time_in_idle: previous reading of the absolute time that this cpu was idle
@@ -97,7 +96,6 @@ struct cpufreq_cooling_device {
unsigned int clipped_freq;
unsigned int max_level;
unsigned int *freq_table;   /* In descending order */
-   struct cpumask allowed_cpus;
struct list_head node;
u32 last_load;
u64 *time_in_idle;
@@ -161,7 +159,7 @@ static int cpufreq_thermal_notifier(struct notifier_block 
*nb,

mutex_lock(_list_lock);
list_for_each_entry(cpufreq_cdev, _cdev_list, node) {
-   if (!cpumask_test_cpu(policy->cpu, _cdev->allowed_cpus))
+   if (policy != cpufreq_cdev->policy)

The policy pointer forwarded from cpufreq_update_policy()
is a local variable 'new_policy' so cannot be compared with pinned
policy pointer in the cooling device.
You should do the cpumask test like before:
if (!cpumask_test_cpu(policy->cpu,
  cpufreq_cdev->policy->related_cpus))


But there is something still in the patch set...
I will try to check it tomorrow.

Best regards,
Lukasz

[PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

'allowed_cpus' is a copy of policy->related_cpus and can be replaced by
it directly. At some places we are only concerned about online CPUs and
policy->cpus can be used there.

Signed-off-by: Viresh Kumar 
---
 drivers/thermal/cpu_cooling.c | 77 ---
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ce387f62c93e..1097162f7f8a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -74,7 +74,6 @@ struct power_table {
  * frequency.
  * @max_level: maximum cooling level. One less than total number of valid
  * cpufreq frequencies.
- * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  * @node: list_head to link all cpufreq_cooling_device together.
  * @last_load: load measured by the latest call to 
cpufreq_get_requested_power()
  * @time_in_idle: previous reading of the absolute time that this cpu was idle
@@ -97,7 +96,6 @@ struct cpufreq_cooling_device {
unsigned int clipped_freq;
unsigned int max_level;
unsigned int *freq_table;   /* In descending order */
-   struct cpumask allowed_cpus;
struct list_head node;
u32 last_load;
u64 *time_in_idle;
@@ -161,7 +159,7 @@ static int cpufreq_thermal_notifier(struct notifier_block 
*nb,
 
mutex_lock(_list_lock);
list_for_each_entry(cpufreq_cdev, _cdev_list, node) {
-   if (!cpumask_test_cpu(policy->cpu, _cdev->allowed_cpus))
+   if (policy != cpufreq_cdev->policy)
continue;
 
/*
@@ -304,7 +302,7 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device 
*cpufreq_cdev,
  * get_load() - get load for a cpu since last updated
  * @cpufreq_cdev:   cpufreq_cooling_device for this cpu
  * @cpu:   cpu number
- * @cpu_idx:   index of the cpu in cpufreq_cdev->allowed_cpus
+ * @cpu_idx:   index of the cpu in time_in_idle*
  *
  * Return: The average load of cpu @cpu in percentage since this
  * function was last called.
@@ -351,7 +349,7 @@ static int get_static_power(struct cpufreq_cooling_device 
*cpufreq_cdev,
 {
struct dev_pm_opp *opp;
unsigned long voltage;
-   struct cpumask *cpumask = _cdev->allowed_cpus;
+   struct cpumask *cpumask = cpufreq_cdev->policy->related_cpus;
unsigned long freq_hz = freq * 1000;
 
if (!cpufreq_cdev->plat_get_static_power || !cpufreq_cdev->cpu_dev) {
@@ -468,7 +466,7 @@ static int cpufreq_set_cur_state(struct 
thermal_cooling_device *cdev,
cpufreq_cdev->cpufreq_state = state;
cpufreq_cdev->clipped_freq = clip_freq;
 
-   cpufreq_update_policy(cpumask_any(_cdev->allowed_cpus));
+   cpufreq_update_policy(cpufreq_cdev->policy->cpu);
 
return 0;
 }
@@ -504,28 +502,18 @@ static int cpufreq_get_requested_power(struct 
thermal_cooling_device *cdev,
int i = 0, cpu, ret;
u32 static_power, dynamic_power, total_load = 0;
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
+   struct cpufreq_policy *policy = cpufreq_cdev->policy;
u32 *load_cpu = NULL;
 
-   cpu = cpumask_any_and(_cdev->allowed_cpus, cpu_online_mask);
-
-   /*
-* All the CPUs are offline, thus the requested power by
-* the cdev is 0
-*/
-   if (cpu >= nr_cpu_ids) {
-   *power = 0;
-   return 0;
-   }
-
-   freq = cpufreq_quick_get(cpu);
+   freq = cpufreq_quick_get(policy->cpu);
 
if (trace_thermal_power_cpu_get_power_enabled()) {
-   u32 ncpus = cpumask_weight(_cdev->allowed_cpus);
+   u32 ncpus = cpumask_weight(policy->related_cpus);
 
load_cpu = kcalloc(ncpus, sizeof(*load_cpu), GFP_KERNEL);
}
 
-   for_each_cpu(cpu, _cdev->allowed_cpus) {
+   for_each_cpu(cpu, policy->related_cpus) {
u32 load;
 
if (cpu_online(cpu))
@@ -550,9 +538,9 @@ static int cpufreq_get_requested_power(struct 
thermal_cooling_device *cdev,
}
 
if (load_cpu) {
-   trace_thermal_power_cpu_get_power(
-   _cdev->allowed_cpus,
-   freq, load_cpu, i, dynamic_power, static_power);
+   trace_thermal_power_cpu_get_power(policy->related_cpus, freq,
+ load_cpu, i, dynamic_power,
+ static_power);
 
kfree(load_cpu);
}
@@ -581,38 +569,22 @@ static int cpufreq_state2power(struct 
thermal_cooling_device *cdev,
   unsigned long state, u32 *power)
 {
unsigned int freq, num_cpus;
-   cpumask_var_t cpumask;
u32 static_power, dynamic_power;
int ret;
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
 
-   if (!alloc_cpumask_var(, 

[PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

[Viresh Kumar]

'allowed_cpus' is a copy of policy->related_cpus and can be replaced by
it directly. At some places we are only concerned about online CPUs and
policy->cpus can be used there.

Signed-off-by: Viresh Kumar 
---
 drivers/thermal/cpu_cooling.c | 77 ---
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index ce387f62c93e..1097162f7f8a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -74,7 +74,6 @@ struct power_table {
  * frequency.
  * @max_level: maximum cooling level. One less than total number of valid
  * cpufreq frequencies.
- * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
  * @node: list_head to link all cpufreq_cooling_device together.
  * @last_load: load measured by the latest call to 
cpufreq_get_requested_power()
  * @time_in_idle: previous reading of the absolute time that this cpu was idle
@@ -97,7 +96,6 @@ struct cpufreq_cooling_device {
unsigned int clipped_freq;
unsigned int max_level;
unsigned int *freq_table;   /* In descending order */
-   struct cpumask allowed_cpus;
struct list_head node;
u32 last_load;
u64 *time_in_idle;
@@ -161,7 +159,7 @@ static int cpufreq_thermal_notifier(struct notifier_block 
*nb,
 
mutex_lock(_list_lock);
list_for_each_entry(cpufreq_cdev, _cdev_list, node) {
-   if (!cpumask_test_cpu(policy->cpu, _cdev->allowed_cpus))
+   if (policy != cpufreq_cdev->policy)
continue;
 
/*
@@ -304,7 +302,7 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device 
*cpufreq_cdev,
  * get_load() - get load for a cpu since last updated
  * @cpufreq_cdev:   cpufreq_cooling_device for this cpu
  * @cpu:   cpu number
- * @cpu_idx:   index of the cpu in cpufreq_cdev->allowed_cpus
+ * @cpu_idx:   index of the cpu in time_in_idle*
  *
  * Return: The average load of cpu @cpu in percentage since this
  * function was last called.
@@ -351,7 +349,7 @@ static int get_static_power(struct cpufreq_cooling_device 
*cpufreq_cdev,
 {
struct dev_pm_opp *opp;
unsigned long voltage;
-   struct cpumask *cpumask = _cdev->allowed_cpus;
+   struct cpumask *cpumask = cpufreq_cdev->policy->related_cpus;
unsigned long freq_hz = freq * 1000;
 
if (!cpufreq_cdev->plat_get_static_power || !cpufreq_cdev->cpu_dev) {
@@ -468,7 +466,7 @@ static int cpufreq_set_cur_state(struct 
thermal_cooling_device *cdev,
cpufreq_cdev->cpufreq_state = state;
cpufreq_cdev->clipped_freq = clip_freq;
 
-   cpufreq_update_policy(cpumask_any(_cdev->allowed_cpus));
+   cpufreq_update_policy(cpufreq_cdev->policy->cpu);
 
return 0;
 }
@@ -504,28 +502,18 @@ static int cpufreq_get_requested_power(struct 
thermal_cooling_device *cdev,
int i = 0, cpu, ret;
u32 static_power, dynamic_power, total_load = 0;
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
+   struct cpufreq_policy *policy = cpufreq_cdev->policy;
u32 *load_cpu = NULL;
 
-   cpu = cpumask_any_and(_cdev->allowed_cpus, cpu_online_mask);
-
-   /*
-* All the CPUs are offline, thus the requested power by
-* the cdev is 0
-*/
-   if (cpu >= nr_cpu_ids) {
-   *power = 0;
-   return 0;
-   }
-
-   freq = cpufreq_quick_get(cpu);
+   freq = cpufreq_quick_get(policy->cpu);
 
if (trace_thermal_power_cpu_get_power_enabled()) {
-   u32 ncpus = cpumask_weight(_cdev->allowed_cpus);
+   u32 ncpus = cpumask_weight(policy->related_cpus);
 
load_cpu = kcalloc(ncpus, sizeof(*load_cpu), GFP_KERNEL);
}
 
-   for_each_cpu(cpu, _cdev->allowed_cpus) {
+   for_each_cpu(cpu, policy->related_cpus) {
u32 load;
 
if (cpu_online(cpu))
@@ -550,9 +538,9 @@ static int cpufreq_get_requested_power(struct 
thermal_cooling_device *cdev,
}
 
if (load_cpu) {
-   trace_thermal_power_cpu_get_power(
-   _cdev->allowed_cpus,
-   freq, load_cpu, i, dynamic_power, static_power);
+   trace_thermal_power_cpu_get_power(policy->related_cpus, freq,
+ load_cpu, i, dynamic_power,
+ static_power);
 
kfree(load_cpu);
}
@@ -581,38 +569,22 @@ static int cpufreq_state2power(struct 
thermal_cooling_device *cdev,
   unsigned long state, u32 *power)
 {
unsigned int freq, num_cpus;
-   cpumask_var_t cpumask;
u32 static_power, dynamic_power;
int ret;
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
 
-   if (!alloc_cpumask_var(, GFP_KERNEL))
-