Re: [PATCH 2/2] thermal: cpufreq_cooling: Reuse effective_cpu_util()

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Amit Daniel Kachhap <amit.kachhap@gmail.com>,
	Javi Merino <javi.merino@kernel.org>,
	Amit Kucheria <amit.kucheria@verdurent.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Quentin Perret <qperret@google.com>,
	Rafael Wysocki <rjw@rjwysocki.net>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH 2/2] thermal: cpufreq_cooling: Reuse effective_cpu_util()
Date: Tue, 14 Jul 2020 15:05:18 +0200	[thread overview]
Message-ID: <CAJZ5v0i=yNH9pGkty2QdeQLcqJcuY=pLx_XcY4VXs8bSqXL=dg@mail.gmail.com> (raw)
In-Reply-To: <b051b42f0c4f36d7177978e090c6a85df17922c6.1594707424.git.viresh.kumar@linaro.org>

On Tue, Jul 14, 2020 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Several parts of the kernel are already using the effective CPU
> utilization 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.
>
> 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/thermal/cpufreq_cooling.c | 65 +++++++------------------------
>  1 file changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> index 6c0e1b053126..74340b2b0da7 100644
> --- a/drivers/thermal/cpufreq_cooling.c
> +++ b/drivers/thermal/cpufreq_cooling.c
> @@ -23,6 +23,7 @@
>  #include <linux/thermal.h>
>
>  #include <trace/events/thermal.h>
> +#include "../../kernel/sched/sched.h"
>
>  /*
>   * Cooling state <-> CPUFreq frequency
> @@ -38,16 +39,6 @@
>   *     ...
>   */
>
> -/**
> - * struct time_in_idle - Idle time stats
> - * @time: previous reading of the absolute time that this cpu was idle
> - * @timestamp: wall time of the last invocation of get_cpu_idle_time_us()
> - */
> -struct time_in_idle {
> -       u64 time;
> -       u64 timestamp;
> -};
> -
>  /**
>   * struct cpufreq_cooling_device - data for cooling device with cpufreq
>   * @id: unique integer value corresponding to each cpufreq_cooling_device
> @@ -62,7 +53,6 @@ struct time_in_idle {
>   *     registered cooling device.
>   * @policy: cpufreq policy.
>   * @node: list_head to link all cpufreq_cooling_device together.
> - * @idle_time: idle time stats
>   * @qos_req: PM QoS contraint to apply
>   *
>   * This structure is required for keeping information of each registered
> @@ -76,7 +66,6 @@ struct cpufreq_cooling_device {
>         struct em_perf_domain *em;
>         struct cpufreq_policy *policy;
>         struct list_head node;
> -       struct time_in_idle *idle_time;
>         struct freq_qos_request qos_req;
>  };
>
> @@ -132,34 +121,21 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev,
>  }
>
>  /**
> - * get_load() - get load for a cpu since last updated
> + * get_load() - get current load for a cpu
>   * @cpufreq_cdev:      &struct cpufreq_cooling_device for this cpu
>   * @cpu:       cpu number
> - * @cpu_idx:   index of the cpu in time_in_idle*
> + * @cpu_idx:   index of the cpu
>   *
> - * Return: The average load of cpu @cpu in percentage since this
> - * function was last called.
> + * Return: The current load of cpu @cpu in percentage.
>   */
>  static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
>                     int cpu_idx)
>  {
> -       u32 load;
> -       u64 now, now_idle, delta_time, delta_idle;
> -       struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu_idx];
> -
> -       now_idle = get_cpu_idle_time(cpu, &now, 0);
> -       delta_idle = now_idle - idle_time->time;
> -       delta_time = now - idle_time->timestamp;
> +       unsigned long util = cpu_util_cfs(cpu_rq(cpu));
> +       unsigned long max = arch_scale_cpu_capacity(cpu);
>
> -       if (delta_time <= delta_idle)
> -               load = 0;
> -       else
> -               load = div64_u64(100 * (delta_time - delta_idle), delta_time);
> -
> -       idle_time->time = now_idle;
> -       idle_time->timestamp = now;
> -
> -       return load;
> +       util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);

Hmm.

It doesn't look like cpufreq_cdev and cpu_idx are needed any more in
this function, so maybe drop them from the arg list?  And then there
won't be anything specific to CPU cooling in this function, so maybe
move it to sched and export it from there properly?

Also it looks like max could be passed to it along with the CPU number
instead of being always taken as arch_scale_cpu_capacity(cpu).

> +       return (util * 100) / max;
>  }
>
>  /**