cpufreq_cooling: Get effective CPU utilization from scheduler

There is nothing schedutil specific in schedutil_cpu_util() and is used by fair.c as well. Allow it to be used by other parts of the kernel as well.

Move it to core.c and rename it to effective_cpu_util(). While at it, rename "enum schedutil_type" as well.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/sched/core.c              | 106 ++++++++++++++++++++++++++++++
 kernel/sched/cpufreq_schedutil.c | 108 +------------------------------
 kernel/sched/fair.c              |   6 +-
 kernel/sched/sched.h             |  20 ++----
 4 files changed, 115 insertions(+), 125 deletions(-)

kernel/sched/core.c +106 -0

index a2a244af9a53..c5b345fdf81d 100644
@@ -4879,6 +4879,112 @@ struct task_struct *idle_task(int cpu)
        return cpu_rq(cpu)->idle;
 }
 
+/*
+ * This function computes an effective utilization for the given CPU, to be
+ * used for frequency selection given the linear relation: f = u * f_max.
+ *
+ * The scheduler tracks the following metrics:
+ *
+ *   cpu_util_{cfs,rt,dl,irq}()
+ *   cpu_bw_dl()
+ *
+ * Where the cfs,rt and dl util numbers are tracked with the same metric and
+ * synchronized windows and are thus directly comparable.
+ *
+ * The cfs,rt,dl utilization are the running times measured with rq->clock_task
+ * which excludes things like IRQ and steal-time. These latter are then accrued
+ * in the irq utilization.
+ *
+ * The DL bandwidth number otoh is not a measured metric but a value computed
+ * based on the task model parameters and gives the minimal utilization
+ * required to meet deadlines.
+ */
+unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
+                                unsigned long max, enum cpu_util_type type,
+                                struct task_struct *p)
+{
+       unsigned long dl_util, util, irq;
+       struct rq *rq = cpu_rq(cpu);
+
+       if (!uclamp_is_used() &&
+           type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt)) {
+               return max;
+       }
+
+       /*
+        * Early check to see if IRQ/steal time saturates the CPU, can be
+        * because of inaccuracies in how we track these -- see
+        * update_irq_load_avg().
+        */
+       irq = cpu_util_irq(rq);
+       if (unlikely(irq >= max))
+               return max;
+
+       /*
+        * Because the time spend on RT/DL tasks is visible as 'lost' time to
+        * CFS tasks and we use the same metric to track the effective
+        * utilization (PELT windows are synchronized) we can directly add them
+        * to obtain the CPU's actual utilization.
+        *
+        * CFS and RT utilization can be boosted or capped, depending on
+        * utilization clamp constraints requested by currently RUNNABLE
+        * tasks.
+        * When there are no CFS RUNNABLE tasks, clamps are released and
+        * frequency will be gracefully reduced with the utilization decay.
+        */
+       util = util_cfs + cpu_util_rt(rq);
+       if (type == FREQUENCY_UTIL)
+               util = uclamp_rq_util_with(rq, util, p);
+
+       dl_util = cpu_util_dl(rq);
+
+       /*
+        * For frequency selection we do not make cpu_util_dl() a permanent part
+        * of this sum because we want to use cpu_bw_dl() later on, but we need
+        * to check if the CFS+RT+DL sum is saturated (ie. no idle time) such
+        * that we select f_max when there is no idle time.
+        *
+        * NOTE: numerical errors or stop class might cause us to not quite hit
+        * saturation when we should -- something for later.
+        */
+       if (util + dl_util >= max)
+               return max;
+
+       /*
+        * OTOH, for energy computation we need the estimated running time, so
+        * include util_dl and ignore dl_bw.
+        */
+       if (type == ENERGY_UTIL)
+               util += dl_util;
+
+       /*
+        * There is still idle time; further improve the number by using the
+        * irq metric. Because IRQ/steal time is hidden from the task clock we
+        * need to scale the task numbers:
+        *
+        *              max - irq
+        *   U' = irq + --------- * U
+        *                 max
+        */
+       util = scale_irq_capacity(util, irq, max);
+       util += irq;
+
+       /*
+        * Bandwidth required by DEADLINE must always be granted while, for
+        * FAIR and RT, we use blocked utilization of IDLE CPUs as a mechanism
+        * to gracefully reduce the frequency when no tasks show up for longer
+        * periods of time.
+        *
+        * Ideally we would like to set bw_dl as min/guaranteed freq and util +
+        * bw_dl as requested freq. However, cpufreq is not yet ready for such
+        * an interface. So, we only do the latter for now.
+        */
+       if (type == FREQUENCY_UTIL)
+               util += cpu_bw_dl(rq);
+
+       return min(max, util);
+}
+
 /**
  * find_process_by_pid - find a process with a matching PID value.
  * @pid: the pid in question.

kernel/sched/cpufreq_schedutil.c +1 -107

index dc6835bc6490..e9623527741b 100644
@@ -183,112 +183,6 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
        return cpufreq_driver_resolve_freq(policy, freq);
 }
 
-/*
- * This function computes an effective utilization for the given CPU, to be
- * used for frequency selection given the linear relation: f = u * f_max.
- *
- * The scheduler tracks the following metrics:
- *
- *   cpu_util_{cfs,rt,dl,irq}()
- *   cpu_bw_dl()
- *
- * Where the cfs,rt and dl util numbers are tracked with the same metric and
- * synchronized windows and are thus directly comparable.
- *
- * The cfs,rt,dl utilization are the running times measured with rq->clock_task
- * which excludes things like IRQ and steal-time. These latter are then accrued
- * in the irq utilization.
- *
- * The DL bandwidth number otoh is not a measured metric but a value computed
- * based on the task model parameters and gives the minimal utilization
- * required to meet deadlines.
- */
-unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
-                                unsigned long max, enum schedutil_type type,
-                                struct task_struct *p)
-{
-       unsigned long dl_util, util, irq;
-       struct rq *rq = cpu_rq(cpu);
-
-       if (!uclamp_is_used() &&
-           type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt)) {
-               return max;
-       }
-
-       /*
-        * Early check to see if IRQ/steal time saturates the CPU, can be
-        * because of inaccuracies in how we track these -- see
-        * update_irq_load_avg().
-        */
-       irq = cpu_util_irq(rq);
-       if (unlikely(irq >= max))
-               return max;
-
-       /*
-        * Because the time spend on RT/DL tasks is visible as 'lost' time to
-        * CFS tasks and we use the same metric to track the effective
-        * utilization (PELT windows are synchronized) we can directly add them
-        * to obtain the CPU's actual utilization.
-        *
-        * CFS and RT utilization can be boosted or capped, depending on
-        * utilization clamp constraints requested by currently RUNNABLE
-        * tasks.
-        * When there are no CFS RUNNABLE tasks, clamps are released and
-        * frequency will be gracefully reduced with the utilization decay.
-        */
-       util = util_cfs + cpu_util_rt(rq);
-       if (type == FREQUENCY_UTIL)
-               util = uclamp_rq_util_with(rq, util, p);
-
-       dl_util = cpu_util_dl(rq);
-
-       /*
-        * For frequency selection we do not make cpu_util_dl() a permanent part
-        * of this sum because we want to use cpu_bw_dl() later on, but we need
-        * to check if the CFS+RT+DL sum is saturated (ie. no idle time) such
-        * that we select f_max when there is no idle time.
-        *
-        * NOTE: numerical errors or stop class might cause us to not quite hit
-        * saturation when we should -- something for later.
-        */
-       if (util + dl_util >= max)
-               return max;
-
-       /*
-        * OTOH, for energy computation we need the estimated running time, so
-        * include util_dl and ignore dl_bw.
-        */
-       if (type == ENERGY_UTIL)
-               util += dl_util;
-
-       /*
-        * There is still idle time; further improve the number by using the
-        * irq metric. Because IRQ/steal time is hidden from the task clock we
-        * need to scale the task numbers:
-        *
-        *              max - irq
-        *   U' = irq + --------- * U
-        *                 max
-        */
-       util = scale_irq_capacity(util, irq, max);
-       util += irq;
-
-       /*
-        * Bandwidth required by DEADLINE must always be granted while, for
-        * FAIR and RT, we use blocked utilization of IDLE CPUs as a mechanism
-        * to gracefully reduce the frequency when no tasks show up for longer
-        * periods of time.
-        *
-        * Ideally we would like to set bw_dl as min/guaranteed freq and util +
-        * bw_dl as requested freq. However, cpufreq is not yet ready for such
-        * an interface. So, we only do the latter for now.
-        */
-       if (type == FREQUENCY_UTIL)
-               util += cpu_bw_dl(rq);
-
-       return min(max, util);
-}
-
 static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
 {
        struct rq *rq = cpu_rq(sg_cpu->cpu);

@@ -298,7 +192,7 @@ static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
        sg_cpu->max = max;
        sg_cpu->bw_dl = cpu_bw_dl(rq);
 
-       return schedutil_cpu_util(sg_cpu->cpu, util, max, FREQUENCY_UTIL, NULL);
+       return effective_cpu_util(sg_cpu->cpu, util, max, FREQUENCY_UTIL, NULL);
 }
 
 /**

kernel/sched/fair.c +3 -3

index 3213cb247aff..94d564745499 100644
@@ -6490,7 +6490,7 @@ compute_energy(struct task_struct *p, int dst_cpu, struct perf_domain *pd)
                 * is already enough to scale the EM reported power
                 * consumption at the (eventually clamped) cpu_capacity.
                 */
-               sum_util += schedutil_cpu_util(cpu, util_cfs, cpu_cap,
+               sum_util += effective_cpu_util(cpu, util_cfs, cpu_cap,
                                               ENERGY_UTIL, NULL);
 
                /*

@@ -6500,7 +6500,7 @@ compute_energy(struct task_struct *p, int dst_cpu, struct perf_domain *pd)
                 * NOTE: in case RT tasks are running, by default the
                 * FREQUENCY_UTIL's utilization can be max OPP.
                 */
-               cpu_util = schedutil_cpu_util(cpu, util_cfs, cpu_cap,
+               cpu_util = effective_cpu_util(cpu, util_cfs, cpu_cap,
                                              FREQUENCY_UTIL, tsk);
                max_util = max(max_util, cpu_util);
        }

@@ -6597,7 +6597,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
                         * IOW, placing the task there would make the CPU
                         * overutilized. Take uclamp into account to see how
                         * much capacity we can get out of the CPU; this is
-                        * aligned with schedutil_cpu_util().
+                        * aligned with effective_cpu_util().
                         */
                        util = uclamp_rq_util_with(cpu_rq(cpu), util, p);
                        if (!fits_capacity(util, cpu_cap))

kernel/sched/sched.h +5 -16

index 65b72e0487bf..dabfc7fa1270 100644
@@ -2465,24 +2465,22 @@ static inline unsigned long capacity_orig_of(int cpu)
 #endif
 
 /**
- * enum schedutil_type - CPU utilization type
+ * enum cpu_util_type - CPU utilization type
  * @FREQUENCY_UTIL:    Utilization used to select frequency
  * @ENERGY_UTIL:       Utilization used during energy calculation
  *
  * The utilization signals of all scheduling classes (CFS/RT/DL) and IRQ time
  * need to be aggregated differently depending on the usage made of them. This
- * enum is used within schedutil_freq_util() to differentiate the types of
+ * enum is used within effective_cpu_util() to differentiate the types of
  * utilization expected by the callers, and adjust the aggregation accordingly.
  */
-enum schedutil_type {
+enum cpu_util_type {
        FREQUENCY_UTIL,
        ENERGY_UTIL,
 };
 
-#ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL
-
-unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
-                                unsigned long max, enum schedutil_type type,
+unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
+                                unsigned long max, enum cpu_util_type type,
                                 struct task_struct *p);
 
 static inline unsigned long cpu_bw_dl(struct rq *rq)

@@ -2511,14 +2509,6 @@ static inline unsigned long cpu_util_rt(struct rq *rq)
 {
        return READ_ONCE(rq->avg_rt.util_avg);
 }
-#else /* CONFIG_CPU_FREQ_GOV_SCHEDUTIL */
-static inline unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
-                                unsigned long max, enum schedutil_type type,
-                                struct task_struct *p)
-{
-       return 0;
-}
-#endif /* CONFIG_CPU_FREQ_GOV_SCHEDUTIL */
 
 #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
 static inline unsigned long cpu_util_irq(struct rq *rq)
--

2.25.0.rc1.19.g042ed3e048af

Hi Viresh,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/sched/core]
[cannot apply to tip/auto-latest pm/linux-next linus/master v5.8-rc5 next-20200713] [If your patch is applied to the wrong git tree, kindly drop us a note.

And when submitting patch, we suggest to use as documented in

Quoted 7 lines · kernel test robot

Hi Viresh,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/sched/core]
[cannot apply to tip/auto-latest pm/linux-next linus/master v5.8-rc5 next-20200713] [If your patch is applied to the wrong git tree, kindly drop us a note.

Yeah, I based it on tip/master.

Quoted 2 lines

And when submitting patch, we suggest to use as documented in

I think you are missing something between "use" and "as" here ? --base ?

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 <[email protected]>

---
 drivers/thermal/cpufreq_cooling.c | 65 +++++++------------------------
 1 file changed, 15 insertions(+), 50 deletions(-)

drivers/thermal/cpufreq_cooling.c +0 -0

index 6c0e1b053126..74340b2b0da7 100644

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 <[email protected]>

---
 drivers/thermal/cpufreq_cooling.c | 65 +++++++------------------------
 1 file changed, 15 insertions(+), 50 deletions(-)

drivers/thermal/cpufreq_cooling.c +6 -23

index 6c0e1b053126..74340b2b0da7 100644
@@ -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);
 

drivers/thermal/cpufreq_cooling.c +1 -0

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

Quoted 13 lines · Viresh Kumar

static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c +2 -7

-       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);
 

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 137

Quoted 10 lines

drivers/thermal/cpufreq_cooling.c +1 -9

-       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).

Quoted 27 lines · Rafael J. Wysocki

Quoted 13 lines · Viresh Kumar

static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c +2 -7

-       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);
 

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 137

Quoted 10 lines

drivers/thermal/cpufreq_cooling.c +1 -9

-       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.

Quoted 2 lines

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?

Right.

Quoted 3 lines

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?

There isn't a lot happening in this routine right now TBH and am not sure if it is really worth it to have a separate routine for this (unless we can get rid of something for all the callers, like avoiding a call to arch_scale_cpu_capacity() and then naming it effective_cpu_load().

Quoted 2 lines

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).

I am not sure what you are suggesting here. What will be the value of max if not arch_scale_cpu_capacity() ?

/**
+ * 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
*
+ * Return: The current load of cpu @cpu in percentage.
*/
static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,

Quoted 11 lines · Viresh Kumar

/**
+ * 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
*
+ * Return: The current load of cpu @cpu in percentage.
*/
static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 139

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +4 -0

+	unsigned long util = cpu_util_cfs(cpu_rq(cpu));
+	unsigned long max = arch_scale_cpu_capacity(cpu);
 
+	util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);
+	return (util * 100) / max;
  }

So there's a number of things... let me recap a bunch of things that got mentioned on IRC earlier this week and then continue from there..

So IPA* (or any other thermal governor) needs energy estimates for the various managed devices, cpufreq_cooling, being the driver for the CPU device, needs to provide that and in return receives feedback on how much energy it is allowed to consume, cpufreq_cooling then dynamically enables/disables OPP states.

There are actually two methods the thermal governor will use:
get_real_power() and get_requested_power().

The first isn't used anywhere in mainline, but could be implemented on hardware that has energy counters (like say x86 RAPL).

The second attempts to guesstimate power, and is the subject of this patch.

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

This patch then attempts to improve upon that by using the scheduler's cpu_util(ENERGY_UTIL) estimate, which is also used to select OPP state and improves upon avg idle. This should be a big improvement as higher

frequency consumes more energy, but should we not also consider that:

	E = C V^2 f

The EAS energy model has tables for the OPPs that contain this, but in this case we seem to be assuming a linear enery/frequency curve, which is just not the case.

I suppose we could do something like **:

	100 * util^3 / max^3

which assumes V~f.

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback. And then cpufreq-cooling can use whatever is provided (through the cpufreq interfaces).

That way, we:

1) don't have to export anything
2) get arch drivers to provide something 'better'

Does that sounds like something sensible?

[*] I always want a beer when I see that name :-)

[**] I despise code that uses percentages, computers suck at /100 and there is no reason not to use any other random fraction, so why pick a bad one.

Quoted 3 lines · Peter Zijlstra

The second attempts to guesstimate power, and is the subject of this patch.

Quoted 3 lines

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

In EM we keep power values in the array and these values grow exponentially. Each OPP has it corresponding

P_x = C (V_x)^2 f_x , where x is the OPP id thus corresponding V,f

so we have discrete power values, growing like:

^(power)

|
|
|                          *
|
|
|                       *
|                       |
|                   *   |
|                       | <----- power estimation function
|            *          |        should not use linear 'util/max_util'
|   *                   |        relation here *
|_______________________|_____________> (freq)

opp0 opp1 opp2 opp3 opp4

What is the problem
First:
We need to pick the right Power from the array. I would suggest to pick the max allowed frequency for that whole period, because we don't know if the CPUs were using it (it's likely). Second:
Then we have the utilization, which can be considered as:
'idle period & running period with various freq inside', lets call it avg performance in that whole period.
Third:
Try to estimate the power used in that whole period having the avg performance and max performance.

What you are suggesting is to travel that [*] line in non-linear fashion, but in (util^3)/(max_util^3). Which means it goes down faster when the utilization drops.
I think it is too aggressive, e.g.
500^3 / 1024^3 = 0.116 <--- very little, ~12% 200^3 / 300^3 = 0.296

Peter could you confirm if I understood you correct?

This is quite important bit for me.

Quoted 16 lines

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback. And then cpufreq-cooling can use whatever is provided (through the cpufreq interfaces).

That way, we:

1) don't have to export anything
2) get arch drivers to provide something 'better'

Quoted 2 lines

Does that sounds like something sensible?

Yes, make sense. Please also keep in mind that this utilization somehow must be mapped into power in a proper way. I am currently working on addressing all of these problems (including this correlation).

Quoted 5 lines · Lukasz Luba

Quoted 3 lines · Peter Zijlstra

The second attempts to guesstimate power, and is the subject of this patch.

Quoted 7 lines

Quoted 3 lines

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

*whee* :-)

...

Quoted 42 lines

In EM we keep power values in the array and these values grow exponentially. Each OPP has it corresponding

P_x = C (V_x)^2 f_x , where x is the OPP id thus corresponding V,f

so we have discrete power values, growing like:

^(power)

|
|
|                          *
|
|
|                       *
|                       |
|                   *   |
|                       | <----- power estimation function
|            *          |        should not use linear 'util/max_util'
|   *                   |        relation here *
|_______________________|_____________> (freq)

opp0 opp1 opp2 opp3 opp4

What is the problem
First:
We need to pick the right Power from the array. I would suggest to pick the max allowed frequency for that whole period, because we don't know if the CPUs were using it (it's likely). Second:
Then we have the utilization, which can be considered as:
'idle period & running period with various freq inside', lets call it avg performance in that whole period.
Third:
Try to estimate the power used in that whole period having the avg performance and max performance.

What you are suggesting is to travel that [*] line in non-linear fashion, but in (util^3)/(max_util^3). Which means it goes down faster when the utilization drops.
I think it is too aggressive, e.g.
500^3 / 1024^3 = 0.116 <--- very little, ~12% 200^3 / 300^3 = 0.296

Quoted 1 line

Peter could you confirm if I understood you correct?

Correct, with the caveat that we might try and regression fit a 3rd order polynomial to a bunch of EM data to see if there's a 'better' function to be had than a raw 'f(x) := x^3'.

Quoted 2 lines

This is quite important bit for me.

So, if we assume schedutil + EM, we can actually have schedutil calculate a running power sum. That is, something like: \Int P_x dt. Because we know the points where OPP changes.

Although, thinking more, I suspect we need tighter integration with cpuidle, because we don't actually have idle times here, but that should be doable.

But for anything other than schedutil + EM, things become more interesting, because then we need to guesstimate power usage without the benefit of having actual power numbers.

We can of course still do that running power sum, with whatever P(u) or P(f) end up with, I suppose.

Quoted 16 lines

Quoted 16 lines

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback. And then cpufreq-cooling can use whatever is provided (through the cpufreq interfaces).

That way, we:

1) don't have to export anything
2) get arch drivers to provide something 'better'

Quoted 7 lines

Quoted 2 lines

Does that sounds like something sensible?

Yes, make sense. Please also keep in mind that this utilization somehow must be mapped into power in a proper way. I am currently working on addressing all of these problems (including this correlation).

Right, so that mapping util to power was what I was missing and suggesting we do. So for 'simple' hardware we have cpufreq events for frequency change, and cpuidle events for idle, and with EM we can simply sum the relevant power numbers.

For hardware lacking EM, or hardware managed DVFS, we'll have to fudge things a little. How best to do that is up in the air a little, but virtual power curves seem a useful tool to me.

The next problem for IPA is having all the devices report power in the same virtual unit I suppose, but I'll leave that to others ;-)

Hi Peter,

Thank you for summarizing this. I've put my comments below.

Quoted 19 lines · Peter Zijlstra

Quoted 11 lines · Viresh Kumar

/**
+ * 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
*
+ * Return: The current load of cpu @cpu in percentage.
*/
static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 139

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +4 -0

+    unsigned long util = cpu_util_cfs(cpu_rq(cpu));
+    unsigned long max = arch_scale_cpu_capacity(cpu);
 
+    util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);
+    return (util * 100) / max;
  }

Quoted 8 lines

So there's a number of things... let me recap a bunch of things that got mentioned on IRC earlier this week and then continue from there..

So IPA* (or any other thermal governor) needs energy estimates for the various managed devices, cpufreq_cooling, being the driver for the CPU device, needs to provide that and in return receives feedback on how much energy it is allowed to consume, cpufreq_cooling then dynamically enables/disables OPP states.

Currently, only IPA uses the power estimation, other governors don't use these API functions in cpufreq_cooling.

Quoted 6 lines

There are actually two methods the thermal governor will use:
get_real_power() and get_requested_power().

The first isn't used anywhere in mainline, but could be implemented on hardware that has energy counters (like say x86 RAPL).

The first is only present as callback for registered devfreq cooling, which is registered by devfreq driver. If that driver provides the get_real_power(), it will be called from get_requested_power(). Thus, it's likely that IPA would get real power value from HW.

I was planning to add it also to cpufreq_cooling callbacks years ago...

Quoted 4 lines

The second attempts to guesstimate power, and is the subject of this patch.

Quoted 3 lines

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

Quoted 5 lines

This patch then attempts to improve upon that by using the scheduler's cpu_util(ENERGY_UTIL) estimate, which is also used to select OPP state and improves upon avg idle. This should be a big improvement as higher

IMHO this patch set doesn't address the real problem: 'sampling freq problem' described above. There was no issue with getting idle period. The avg freq was the problem, in that period when the

CPUs were running. The model implemented in alg was also a problem.

The whole period (e.g. CPU freqs which were used or idle state)

^(CPU freq)

|
|                            sampling the current freq
|                _______        |
|               |      |        |
|________       |      |        |
|       |       |      |        |
|       | idle  |      |________v________...
|_ _____|_______|__________________________> (time)

start of period end
|<------- (typically 100ms)-->|

Quoted 7 lines

frequency consumes more energy, but should we not also consider that:

      E = C V^2 f

The EAS energy model has tables for the OPPs that contain this, but in this case we seem to be assuming a linear enery/frequency curve, which is just not the case.

I am not sure if I got your point. To understand your point better I think some drawing would be required. I will skip this patch and old mainline code and focus on your proposed solution (because this patch set does not address 'sampling freq problem').

Quoted 6 lines

I suppose we could do something like **:

      100 * util^3 / max^3

which assumes V~f.

In EM we keep power values in the array and these values grow exponentially. Each OPP has it corresponding

P_x = C (V_x)^2 f_x , where x is the OPP id thus corresponding V,f

so we have discrete power values, growing like:

^(power)

|
|
|                          *
|
|
|                       *
|                       |
|                   *   |
|                       | <----- power estimation function
|            *          |        should not use linear 'util/max_util'
|   *                   |        relation here *
|_______________________|_____________> (freq)

opp0 opp1 opp2 opp3 opp4

What is the problem
First:
We need to pick the right Power from the array. I would suggest to pick the max allowed frequency for that whole period, because we don't know if the CPUs were using it (it's likely). Second:
Then we have the utilization, which can be considered as:
'idle period & running period with various freq inside', lets call it avg performance in that whole period.
Third:
Try to estimate the power used in that whole period having the avg performance and max performance.

What you are suggesting is to travel that [*] line in non-linear fashion, but in (util^3)/(max_util^3). Which means it goes down faster when the utilization drops.
I think it is too aggressive, e.g.
500^3 / 1024^3 = 0.116 <--- very little, ~12% 200^3 / 300^3 = 0.296

Peter could you confirm if I understood you correct? This is quite important bit for me.

Quoted 11 lines

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback. And then cpufreq-cooling can use whatever is provided (through the cpufreq interfaces).

Quoted 8 lines

That way, we:

1) don't have to export anything
2) get arch drivers to provide something 'better'

Does that sounds like something sensible?

Yes, make sense. Please also keep in mind that this utilization somehow must be mapped into power in a proper way. I am currently working on addressing all of these problems (including this correlation).

Thank you for your time spending on it and your suggestions.

Regards,
Lukasz

Quoted 9 lines

[*] I always want a beer when I see that name :-)

[**] I despise code that uses percentages, computers suck at /100 and there is no reason not to use any other random fraction, so why pick a bad one.

Quoted 56 lines · Lukasz Luba

Hi Peter,

Thank you for summarizing this. I've put my comments below.

Quoted 19 lines · Peter Zijlstra

Quoted 11 lines · Viresh Kumar

   /**

+ * 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

    *
+ * Return: The current load of cpu @cpu in percentage.
    */

static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 139

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +4 -0

+    unsigned long util = cpu_util_cfs(cpu_rq(cpu));
+    unsigned long max = arch_scale_cpu_capacity(cpu);
 
+    util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);
+    return (util * 100) / max;
   }

Quoted 8 lines

So there's a number of things... let me recap a bunch of things that got mentioned on IRC earlier this week and then continue from there..

So IPA* (or any other thermal governor) needs energy estimates for the various managed devices, cpufreq_cooling, being the driver for the CPU device, needs to provide that and in return receives feedback on how much energy it is allowed to consume, cpufreq_cooling then dynamically enables/disables OPP states.

Currently, only IPA uses the power estimation, other governors don't use these API functions in cpufreq_cooling.

Quoted 6 lines

There are actually two methods the thermal governor will use:
get_real_power() and get_requested_power().

The first isn't used anywhere in mainline, but could be implemented on hardware that has energy counters (like say x86 RAPL).

The first is only present as callback for registered devfreq cooling, which is registered by devfreq driver. If that driver provides the get_real_power(), it will be called from get_requested_power(). Thus, it's likely that IPA would get real power value from HW.

I was planning to add it also to cpufreq_cooling callbacks years ago...

Quoted 4 lines

The second attempts to guesstimate power, and is the subject of this patch.

Quoted 7 lines

Quoted 3 lines

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant
- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

Quoted 9 lines

Quoted 5 lines

This patch then attempts to improve upon that by using the scheduler's cpu_util(ENERGY_UTIL) estimate, which is also used to select OPP state and improves upon avg idle. This should be a big improvement as higher

IMHO this patch set doesn't address the real problem: 'sampling freq problem' described above. There was no issue with getting idle period. The avg freq was the problem, in that period when the

Not sure that you can say that avg freq is a bigger problem than getting the load because there is a real issue with tracking idle period for estimating load because running slower reduces the idle time and increases artificially the load. That's why we implemented frequency invariance in PELT.

At the opposite when the thermal mitigation happens, the frequency will be most probably capped by cpu cooling device and will most probably stay at the capped value

Quoted 61 lines

CPUs were running. The model implemented in alg was also a problem.

The whole period (e.g. CPU freqs which were used or idle state)

^(CPU freq)

|
|                            sampling the current freq
|                _______        |
|               |      |        |
|________       |      |        |
|       |       |      |        |
|       | idle  |      |________v________...
|_ _____|_______|__________________________> (time)

start of period end
|<------- (typically 100ms)-->|

Quoted 7 lines

frequency consumes more energy, but should we not also consider that:

       E = C V^2 f

The EAS energy model has tables for the OPPs that contain this, but in this case we seem to be assuming a linear enery/frequency curve, which is just not the case.

I am not sure if I got your point. To understand your point better I think some drawing would be required. I will skip this patch and old mainline code and focus on your proposed solution (because this patch set does not address 'sampling freq problem').

Quoted 6 lines

I suppose we could do something like **:

       100 * util^3 / max^3

which assumes V~f.

In EM we keep power values in the array and these values grow exponentially. Each OPP has it corresponding

P_x = C (V_x)^2 f_x , where x is the OPP id thus corresponding V,f

so we have discrete power values, growing like:

^(power)

|
|
|                          *
|
|
|                       *
|                       |
|                   *   |
|                       | <----- power estimation function
|            *          |        should not use linear 'util/max_util'
|   *                   |        relation here *
|_______________________|_____________> (freq)

opp0 opp1 opp2 opp3 opp4

Quoted 12 lines

What is the problem
First:
We need to pick the right Power from the array. I would suggest to pick the max allowed frequency for that whole period, because we don't know if the CPUs were using it (it's likely). Second:
Then we have the utilization, which can be considered as:
'idle period & running period with various freq inside', lets call it avg performance in that whole period.
Third:
Try to estimate the power used in that whole period having the avg performance and max performance.

We already have a function that is doing such kind of computation based of the utilization of the CPU : em_pd_energy(). And we could reuse some of this function if not exactly this one

Quoted 86 lines · Vincent Guittot

Quoted 56 lines · Lukasz Luba

Hi Peter,

Thank you for summarizing this. I've put my comments below.

Quoted 19 lines · Peter Zijlstra

Quoted 11 lines · Viresh Kumar

   /**

+ * 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

    *
+ * Return: The current load of cpu @cpu in percentage.
    */

static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
int cpu_idx)
{

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 139

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +4 -0

+    unsigned long util = cpu_util_cfs(cpu_rq(cpu));
+    unsigned long max = arch_scale_cpu_capacity(cpu);
 
+    util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);
+    return (util * 100) / max;
   }

Quoted 8 lines

So there's a number of things... let me recap a bunch of things that got mentioned on IRC earlier this week and then continue from there..

So IPA* (or any other thermal governor) needs energy estimates for the various managed devices, cpufreq_cooling, being the driver for the CPU device, needs to provide that and in return receives feedback on how much energy it is allowed to consume, cpufreq_cooling then dynamically enables/disables OPP states.

Currently, only IPA uses the power estimation, other governors don't use these API functions in cpufreq_cooling.

Quoted 6 lines

There are actually two methods the thermal governor will use:
get_real_power() and get_requested_power().

The first isn't used anywhere in mainline, but could be implemented on hardware that has energy counters (like say x86 RAPL).

The first is only present as callback for registered devfreq cooling, which is registered by devfreq driver. If that driver provides the get_real_power(), it will be called from get_requested_power(). Thus, it's likely that IPA would get real power value from HW.

I was planning to add it also to cpufreq_cooling callbacks years ago...

Quoted 4 lines

The second attempts to guesstimate power, and is the subject of this patch.

Quoted 7 lines

Quoted 3 lines

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant
- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

Quoted 9 lines

Quoted 5 lines

This patch then attempts to improve upon that by using the scheduler's cpu_util(ENERGY_UTIL) estimate, which is also used to select OPP state and improves upon avg idle. This should be a big improvement as higher

IMHO this patch set doesn't address the real problem: 'sampling freq problem' described above. There was no issue with getting idle period. The avg freq was the problem, in that period when the

Quoted 5 lines

Not sure that you can say that avg freq is a bigger problem than getting the load because there is a real issue with tracking idle period for estimating load because running slower reduces the idle time and increases artificially the load. That's why we implemented frequency invariance in PELT.

If you take a closer look into the code, you see that wrong freq picks the wrong power value from the EM, the line:
freq = cpufreq_quick_get(policy->cpu)
then check the function cpu_freq_to_power().
The power is calculated by:
(raw_cpu_power * cpufreq_cdev->last_load) / 100

The load estimation error is also an issue, but the comprehensive solution should address possibly all existing issues.

I don't know if you were approached also by the same vendor, complaining on IPA issues. Do you have some requirements? Or deadlines, for which kernel version you have to fix it?

We can discuss this out offline if you like.

Quoted 4 lines

At the opposite when the thermal mitigation happens, the frequency will be most probably capped by cpu cooling device and will most probably stay at the capped value

I don't think that we can rely on such assumption. The whole

cpufreq_get_requested_power() should be changed.

Quoted 74 lines

Quoted 61 lines

CPUs were running. The model implemented in alg was also a problem.

The whole period (e.g. CPU freqs which were used or idle state)

^(CPU freq)

|
|                            sampling the current freq
|                _______        |
|               |      |        |
|________       |      |        |
|       |       |      |        |
|       | idle  |      |________v________...
|_ _____|_______|__________________________> (time)

start of period end
|<------- (typically 100ms)-->|

Quoted 7 lines

frequency consumes more energy, but should we not also consider that:

       E = C V^2 f

The EAS energy model has tables for the OPPs that contain this, but in this case we seem to be assuming a linear enery/frequency curve, which is just not the case.

I am not sure if I got your point. To understand your point better I think some drawing would be required. I will skip this patch and old mainline code and focus on your proposed solution (because this patch set does not address 'sampling freq problem').

Quoted 6 lines

I suppose we could do something like **:

       100 * util^3 / max^3

which assumes V~f.

In EM we keep power values in the array and these values grow exponentially. Each OPP has it corresponding

P_x = C (V_x)^2 f_x , where x is the OPP id thus corresponding V,f

so we have discrete power values, growing like:

^(power)

|
|
|                          *
|
|
|                       *
|                       |
|                   *   |
|                       | <----- power estimation function
|            *          |        should not use linear 'util/max_util'
|   *                   |        relation here *
|_______________________|_____________> (freq)

opp0 opp1 opp2 opp3 opp4

Quoted 12 lines

What is the problem
First:
We need to pick the right Power from the array. I would suggest to pick the max allowed frequency for that whole period, because we don't know if the CPUs were using it (it's likely). Second:
Then we have the utilization, which can be considered as:
'idle period & running period with various freq inside', lets call it avg performance in that whole period.
Third:
Try to estimate the power used in that whole period having the avg performance and max performance.

Quoted 3 lines

We already have a function that is doing such kind of computation based of the utilization of the CPU : em_pd_energy(). And we could reuse some of this function if not exactly this one

Yes and I think we should use this information. I am going to talk with Daniel about EM evolution (this is one of the topics from my side). Next, it is going to be a LPC event, where we can also discuss with broader audience.

Regards,
Lukasz

Quoted 5 lines · Lukasz Luba

Quoted 3 lines · Peter Zijlstra

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Quoted 3 lines

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant

- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

Quoted 6 lines · Vincent Guittot

Quoted 5 lines · Lukasz Luba

Quoted 3 lines · Peter Zijlstra

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Quoted 8 lines

Quoted 3 lines

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant

Since this patch is targeted only towards fixing this particular problem, should I change something in the patch to make it acceptable ?

Quoted 2 lines

- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

This should be tackled separately I believe.

Hi Viresh,

Quoted 16 lines · Viresh Kumar

Quoted 6 lines · Vincent Guittot

Quoted 5 lines · Lukasz Luba

Quoted 3 lines · Peter Zijlstra

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Quoted 8 lines

Quoted 3 lines

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant

Quoted 9 lines

Since this patch is targeted only towards fixing this particular problem, should I change something in the patch to make it acceptable ?

Quoted 2 lines

- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

This should be tackled separately I believe.

I don't think that these two are separate. Furthermore, I think we would need this kind of information also in future in the powercap. I've discussed with Daniel this possible scenario.

We have a vendor who presented issue with the IPA input power and pointed out these issues. Unfortunately, I don't have this vendor phone but I assume it can last a few minutes without changing the max allowed OPP. Based on their plots the frequency driven by the governor is changing, also the idles are present during the IPA period.

Please give me a few days, because I am also plumbing these stuff and would like to present it. These two interfaces: involving cpufreq driver or fallback mode for utilization and EM.

Quoted 39 lines · Lukasz Luba

Hi Viresh,

Quoted 16 lines · Viresh Kumar

Quoted 6 lines · Vincent Guittot

Quoted 5 lines · Lukasz Luba

Quoted 3 lines · Peter Zijlstra

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

Quoted 8 lines

Quoted 3 lines

Even worse, it then *samples* the *current* CPU frequency at that particular point in time and assumes that when the CPU wasn't idle during that period - it had *this* frequency...

So there is 2 problems in the power calculation of cpufreq cooling device :
- How to get an accurate utilization level of the cpu which is what this patch is trying to fix because using idle time is just wrong whereas scheduler utilization is frequency invariant

Quoted 9 lines

Since this patch is targeted only towards fixing this particular problem, should I change something in the patch to make it acceptable ?

Quoted 2 lines

- How to get power estimate from this utilization level. And as you pointed out, using the current freq which is not accurate.

This should be tackled separately I believe.

I don't think that these two are separate. Furthermore, I think we would need this kind of information also in future in the powercap. I've discussed with Daniel this possible scenario.

We have a vendor who presented issue with the IPA input power and pointed out these issues. Unfortunately, I don't have this vendor phone but I assume it can last a few minutes without changing the max allowed OPP. Based on their plots the frequency driven by the governor is changing, also the idles are present during the IPA period.

Quoted 3 lines

Please give me a few days, because I am also plumbing these stuff and would like to present it. These two interfaces: involving cpufreq driver or fallback mode for utilization and EM.

Its been almost 3 months, do we have any update for this? We really would like to get this patchset merged in some form as it provides a simple update and I think more work can be done by anyone over it in future.

So there's a number of things... let me recap a bunch of things that got mentioned on IRC earlier this week and then continue from there..

So IPA* (or any other thermal governor) needs energy estimates for the various managed devices, cpufreq_cooling, being the driver for the CPU device, needs to provide that and in return receives feedback on how much energy it is allowed to consume, cpufreq_cooling then dynamically enables/disables OPP states.

There are actually two methods the thermal governor will use:
get_real_power() and get_requested_power().

The first isn't used anywhere in mainline, but could be implemented on hardware that has energy counters (like say x86 RAPL).

The second attempts to guesstimate power, and is the subject of this patch.

Currently cpufreq_cooling appears to estimate the CPU energy usage by calculating the percentage of idle time using the per-cpu cpustat stuff, which is pretty horrific.

This patch then attempts to improve upon that by using the scheduler's cpu_util(ENERGY_UTIL) estimate, which is also used to select OPP state and improves upon avg idle. This should be a big improvement as higher frequency consumes more energy

, but should we not also consider that:

	E = C V^2 f

The EAS energy model has tables for the OPPs that contain this, but in this case we seem to be assuming a linear enery/frequency curve, which is just not the case.

I suppose we could do something like **:

	100 * util^3 / max^3

which assumes V~f.

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback.

And then cpufreq-cooling can use
whatever is provided (through the cpufreq interfaces).

That way, we:

1) don't have to export anything

2) get arch drivers to provide something 'better'

Does that sounds like something sensible?

Quoted 4 lines · Peter Zijlstra

Another point is that cpu_util() vs turbo is a bit iffy, and to that, things like x86-APERF/MPERF and ARM-AMU got mentioned. Those might also have the benefit of giving you values that match your own sampling interval (100ms), where the sched stuff is PELT (64,32.. based).

I believe the above stuff is more around additional improvements that we can do over this change, and probably Lukasz was looking to do that.

Quoted 3 lines

So what I've been thinking is that cpufreq drivers ought to be able to supply this method, and only when they lack, can the cpufreq-governor (schedutil) install a fallback.

One of the issues I see with this is that schedutil may not be available in all configurations and it is still absolutely fine to using the suggested helper to get the energy numbers in such cases, so we shouldn't really make it scheutil dependent.

Quoted 4 lines · Viresh Kumar

One of the issues I see with this is that schedutil may not be available in all configurations and it is still absolutely fine to using the suggested helper to get the energy numbers in such cases, so we shouldn't really make it scheutil dependent.

The only constraint on schedutil is SMP I think; aside from that it should/could always be available.

Given the trainwreck here:

[email protected]

(you're on Cc), I'm starting to lean more and more towards making it unconditionally available (when SMP).

Anybody forcing it off either sets performance (in which case we don't care about energy usage anyway)

or they select one of the old (broken) ondemand/conservative things and I don't give a crap.

Quoted 13 lines · Peter Zijlstra

Quoted 4 lines · Viresh Kumar

One of the issues I see with this is that schedutil may not be available in all configurations and it is still absolutely fine to using the suggested helper to get the energy numbers in such cases, so we shouldn't really make it scheutil dependent.

The only constraint on schedutil is SMP I think; aside from that it should/could always be available.

Given the trainwreck here:

[email protected]

Quoted 5 lines

(you're on Cc), I'm starting to lean more and more towards making it unconditionally available (when SMP).

Anybody forcing it off either sets performance (in which case we don't care about energy usage anyway)

I agree.

Quoted 2 lines

or they select one of the old (broken) ondemand/conservative things and I don't give a crap.

The other kernel layers, for example cpufreq-cooling in question here, don't really need to bother with the governor in use and should be able to get the energy numbers anyway. So for me, the energy number that the cpufreq-cooling stuff gets should be same irrespective of the governor in use, schedutil or ondemand.

Having said that, schedutil really doesn't need to install the fallback (which you suggested earlier), rather the scheduler core can do that directly with cpufreq core and schedutil can also use the same fallback mechanism maybe ? And so we can avoid the exporting of stuff that way.

Many people use intel_pstate in the active mode with HWP enabled too.

Arguably, that doesn't need to compute the effective utilization, so I guess it is not relevant for the discussion here, but it is not negligible in general.

Quoted 1 line · Rafael J. Wysocki

Many people use intel_pstate in the active mode with HWP enabled too.

We now have HWP-passive supported, afaict. So we should discourage that.

That is; I'll care less and less about people not using schedutil as time goes on.

Quoted 3 lines

Arguably, that doesn't need to compute the effective utilization, so I guess it is not relevant for the discussion here, but it is not negligible in general.

Why not? cpufreq-cooling should still be able to throttle the system by setting HWP.Highest_Performance no?

In which case it still needs an energy estimate.

Quoted 11 lines · Peter Zijlstra

Quoted 4 lines · Viresh Kumar

One of the issues I see with this is that schedutil may not be available in all configurations and it is still absolutely fine to using the suggested helper to get the energy numbers in such cases, so we shouldn't really make it scheutil dependent.

The only constraint on schedutil is SMP I think; aside from that it should/could always be available.

Given the trainwreck here:

Quoted 7 lines

[email protected]

(you're on Cc), I'm starting to lean more and more towards making it unconditionally available (when SMP).

Anybody forcing it off either sets performance (in which case we don't care about energy usage anyway)

I agree.

Quoted 2 lines

or they select one of the old (broken) ondemand/conservative things and I don't give a crap.

The other kernel layers, for example cpufreq-cooling in question here, don't really need to bother with the governor in use and should be able to get the energy numbers anyway. So for me, the energy number that the cpufreq-cooling stuff gets should be same irrespective of the governor in use, schedutil or ondemand.

Having said that, schedutil really doesn't need to install the fallback (which you suggested earlier), rather the scheduler core can do that directly with cpufreq core and schedutil can also use the same fallback mechanism maybe ? And so we can avoid the exporting of stuff that way.

/**
- * 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,

drivers/thermal/cpufreq_cooling.c +0 -0

 		    int cpu_idx)

Quoted 18 lines · Viresh Kumar

/**
- * 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,

drivers/thermal/cpufreq_cooling.c +0 -3

 		    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];

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 135

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +2 -4

-
-	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);

Should we subtract the thermal PELT signal from max?

Quoted 19 lines · Quentin Perret

Quoted 18 lines · Viresh Kumar

/**
- * 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,

drivers/thermal/cpufreq_cooling.c +0 -3

 		    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];

Quoted 8 lines

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 135

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +2 -4

-
-	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);

Should we subtract the thermal PELT signal from max?

Actually, that or add it the ENERGY_UTIL case in effective_cpu_util() as this appears to be missing for EAS too ...

Quoted 20 lines · Quentin Perret

Quoted 19 lines · Quentin Perret

Quoted 18 lines · Viresh Kumar

/**
- * 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,

drivers/thermal/cpufreq_cooling.c +0 -3

 		    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];

Quoted 8 lines

Quoted 8 lines

drivers/thermal/cpufreq_cooling.c static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev, · line 135

Quoted 6 lines

drivers/thermal/cpufreq_cooling.c +2 -4

-
-	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);

Should we subtract the thermal PELT signal from max?