Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'
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'
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'
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'
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(&cooling_list_lock); list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { - if (!cpumask_test_cpu(policy->cpu, &cpufreq_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'
'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(&cooling_list_lock); list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { - if (!cpumask_test_cpu(policy->cpu, &cpufreq_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: &struct 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 = &cpufreq_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(&cpufreq_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(&cpufreq_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(&cpufreq_cdev->allowed_cpus); + u32 ncpus = cpumask_weight(policy->related_cpus); load_cpu = kcalloc(ncpus, sizeof(*load_cpu), GFP_KERNEL); } - for_each_cpu(cpu, &cpufreq_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( - &cpufreq_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-