Messages in this thread | | | Subject | Re: [PATCH V3 2/2] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms | From | Lukasz Luba <> | Date | Mon, 23 Nov 2020 15:32:56 +0000 |
| |
On 11/19/20 7:38 AM, Viresh Kumar wrote: > Several parts of the kernel are already using the effective CPU > utilization (as seen by the scheduler) to get the current load on the > CPU, do the same here instead of depending on the idle time of the CPU, > which isn't that accurate comparatively. > > This is also the right thing to do as it makes the cpufreq governor > (schedutil) align better with the cpufreq_cooling driver, as the power > requested by cpufreq_cooling governor will exactly match the next > frequency requested by the schedutil governor since they are both using > the same metric to calculate load. > > Note that, this (and CPU frequency scaling in general) doesn't work that > well with idle injection as that is done from rt threads and is counted > as load while it tries to do quite the opposite. That should be solved > separately though. > > This was tested on ARM Hikey6220 platform with hackbench, sysbench and > schbench. None of them showed any regression or significant > improvements. Schbench is the most important ones out of these as it > creates the scenario where the utilization numbers provide a better > estimate of the future. > > Scenario 1: The CPUs were mostly idle in the previous polling window of > the IPA governor as the tasks were sleeping and here are the details > from traces (load is in %): > > Old: thermal_power_cpu_get_power: cpus=00000000,000000ff freq=1200000 total_load=203 load={{0x35,0x1,0x0,0x31,0x0,0x0,0x64,0x0}} dynamic_power=1339 > New: thermal_power_cpu_get_power: cpus=00000000,000000ff freq=1200000 total_load=600 load={{0x60,0x46,0x45,0x45,0x48,0x3b,0x61,0x44}} dynamic_power=3960 > > Here, the "Old" line gives the load and requested_power (dynamic_power > here) numbers calculated using the idle time based implementation, while > "New" is based on the CPU utilization from scheduler. > > As can be clearly seen, the load and requested_power numbers are simply > incorrect in the idle time based approach and the numbers collected from > CPU's utilization are much closer to the reality. > > Scenario 2: The CPUs were busy in the previous polling window of the IPA > governor: > > Old: thermal_power_cpu_get_power: cpus=00000000,000000ff freq=1200000 total_load=800 load={{0x64,0x64,0x64,0x64,0x64,0x64,0x64,0x64}} dynamic_power=5280 > New: thermal_power_cpu_get_power: cpus=00000000,000000ff freq=1200000 total_load=708 load={{0x4d,0x5c,0x5c,0x5b,0x5c,0x5c,0x51,0x5b}} dynamic_power=4672 > > As can be seen, the idle time based load is 100% for all the CPUs as it > took only the last window into account, but in reality the CPUs aren't > that loaded as shown by the utilization numbers. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/thermal/cpufreq_cooling.c | 68 ++++++++++++++++++++++++------- > 1 file changed, 54 insertions(+), 14 deletions(-) > > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c > index cc2959f22f01..a364a2fd84b1 100644 > --- a/drivers/thermal/cpufreq_cooling.c > +++ b/drivers/thermal/cpufreq_cooling.c > @@ -76,7 +76,9 @@ struct cpufreq_cooling_device { > struct em_perf_domain *em; > struct cpufreq_policy *policy; > struct list_head node; > +#ifndef CONFIG_SMP > struct time_in_idle *idle_time; > +#endif > struct freq_qos_request qos_req; > }; > > @@ -132,14 +134,35 @@ 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 time_in_idle* > + * get_load() - get load for a cpu > + * @cpufreq_cdev: struct cpufreq_cooling_device for the cpu > + * @cpu: cpu number > + * @cpu_idx: index of the cpu in time_in_idle array > * > * Return: The average load of cpu @cpu in percentage since this > * function was last called. > */ > +#ifdef CONFIG_SMP > +static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu, > + int cpu_idx) > +{ > + unsigned long max = arch_scale_cpu_capacity(cpu); > + unsigned long util; > + > + util = sched_cpu_util(cpu, ENERGY_UTIL, max); > + return (util * 100) / max; > +} > + > +static inline int allocate_idle_time(struct cpufreq_cooling_device *cpufreq_cdev) > +{ > + return 0; > +} > + > +static inline void free_idle_time(struct cpufreq_cooling_device *cpufreq_cdev) > +{ > +} > + > +#else /* !CONFIG_SMP */ > static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu, > int cpu_idx) > { > @@ -162,6 +185,26 @@ static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu, > return load; > } > > +static int allocate_idle_time(struct cpufreq_cooling_device *cpufreq_cdev) > +{ > + unsigned int num_cpus = cpumask_weight(cpufreq_cdev->policy->related_cpus); > + > + cpufreq_cdev->idle_time = kcalloc(num_cpus, > + sizeof(*cpufreq_cdev->idle_time), > + GFP_KERNEL); > + if (!cpufreq_cdev->idle_time) > + return -ENOMEM; > + > + return 0; > +} > + > +static void free_idle_time(struct cpufreq_cooling_device *cpufreq_cdev) > +{ > + kfree(cpufreq_cdev->idle_time); > + cpufreq_cdev->idle_time = NULL; > +} > +#endif /* CONFIG_SMP */ > + > /** > * get_dynamic_power() - calculate the dynamic power > * @cpufreq_cdev: &cpufreq_cooling_device for this cdev > @@ -487,7 +530,7 @@ __cpufreq_cooling_register(struct device_node *np, > struct thermal_cooling_device *cdev; > struct cpufreq_cooling_device *cpufreq_cdev; > char dev_name[THERMAL_NAME_LENGTH]; > - unsigned int i, num_cpus; > + unsigned int i; > struct device *dev; > int ret; > struct thermal_cooling_device_ops *cooling_ops; > @@ -498,7 +541,6 @@ __cpufreq_cooling_register(struct device_node *np, > return ERR_PTR(-ENODEV); > } > > - > if (IS_ERR_OR_NULL(policy)) { > pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy); > return ERR_PTR(-EINVAL); > @@ -516,12 +558,10 @@ __cpufreq_cooling_register(struct device_node *np, > return ERR_PTR(-ENOMEM); > > cpufreq_cdev->policy = policy; > - num_cpus = cpumask_weight(policy->related_cpus); > - cpufreq_cdev->idle_time = kcalloc(num_cpus, > - sizeof(*cpufreq_cdev->idle_time), > - GFP_KERNEL); > - if (!cpufreq_cdev->idle_time) { > - cdev = ERR_PTR(-ENOMEM); > + > + ret = allocate_idle_time(cpufreq_cdev); > + if (ret) { > + cdev = ERR_PTR(ret); > goto free_cdev; > } > > @@ -581,7 +621,7 @@ __cpufreq_cooling_register(struct device_node *np, > remove_ida: > ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); > free_idle_time: > - kfree(cpufreq_cdev->idle_time); > + free_idle_time(cpufreq_cdev); > free_cdev: > kfree(cpufreq_cdev); > return cdev; > @@ -674,7 +714,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) > thermal_cooling_device_unregister(cdev); > freq_qos_remove_request(&cpufreq_cdev->qos_req); > ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); > - kfree(cpufreq_cdev->idle_time); > + free_idle_time(cpufreq_cdev); > kfree(cpufreq_cdev); > } > EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); >
LGTM. It has potential. We will see how far we can improve IPA with this model.
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Regards, Lukasz
| |