[PATCH V4 0/3] cpufreq_cooling: Get effective CPU utilization from scheduler

[PATCH V4 0/3] cpufreq_cooling: Get effective CPU utilization from scheduler

[Viresh Kumar]

Hi,

This patchset makes the cpufreq_cooling driver reuse the CPU utilization
metric provided by the scheduler instead of depending on idle and busy
times of a CPU, which aren't that accurate to measure the busyness of a
CPU for the next cycle. More details can be seen in the commit log of
patch 2/2.

V3->V4:
- Broke the first patch into two parts and used effective_cpu_util() in
  schedutil (Rafael).

- Removed comment about idle-injection in last patch based on feedback
  from Lukasz and added hi Reviewed-by tag.

V2->V3:
- Put the scheduler helpers within ifdef CONFIG_SMP.
- Keep both SMP and !SMP implementations in the cpufreq_cooling driver.
- Improved commit log with testing related information.

--
Viresh

Viresh Kumar (3):
  sched/core: Move schedutil_cpu_util() to core.c
  sched/core: Rename schedutil_cpu_util() and allow rest of the kernel
    to use it
  thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

 drivers/thermal/cpufreq_cooling.c |  68 ++++++++++++++----
 include/linux/sched.h             |  21 ++++++
 kernel/sched/core.c               | 115 ++++++++++++++++++++++++++++++
 kernel/sched/cpufreq_schedutil.c  | 108 +---------------------------
 kernel/sched/fair.c               |   6 +-
 kernel/sched/sched.h              |  31 +-------
 6 files changed, 197 insertions(+), 152 deletions(-)


base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V4 1/3] sched/core: Move schedutil_cpu_util() to core.c

[Viresh Kumar]

There is nothing schedutil specific in schedutil_cpu_util(), move it to
core.c and define it only for CONFIG_SMP.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/core.c              | 108 +++++++++++++++++++++++++++++++
 kernel/sched/cpufreq_schedutil.c | 106 ------------------------------
 kernel/sched/sched.h             |  12 +---
 3 files changed, 109 insertions(+), 117 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d2003a7d5ab5..b81265aec4a0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5117,6 +5117,114 @@ struct task_struct *idle_task(int cpu)
 	return cpu_rq(cpu)->idle;
 }
 
+#ifdef CONFIG_SMP
+/*
+ * 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);
+}
+#endif /* CONFIG_SMP */
+
 /**
  * find_process_by_pid - find a process with a matching PID value.
  * @pid: the pid in question.
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e254745a82cb..2d44befb322b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -169,112 +169,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);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index df80bfcea92e..0db6bcf0881f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2484,7 +2484,6 @@ static inline unsigned long capacity_orig_of(int cpu)
 {
 	return cpu_rq(cpu)->cpu_capacity_orig;
 }
-#endif
 
 /**
  * enum schedutil_type - CPU utilization type
@@ -2501,8 +2500,6 @@ enum schedutil_type {
 	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,
 				 struct task_struct *p);
@@ -2533,14 +2530,7 @@ 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 */
+#endif
 
 #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
 static inline unsigned long cpu_util_irq(struct rq *rq)
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V4 2/3] sched/core: Rename schedutil_cpu_util() and allow rest of the kernel to use it

[Viresh Kumar]

There is nothing schedutil specific in schedutil_cpu_util(), rename it
to effective_cpu_util(). Also create and expose another wrapper
sched_cpu_util() which can be used by other parts of the kernel, like
thermal core (that will be done in a later commit).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/sched.h            | 21 +++++++++++++++++++++
 kernel/sched/core.c              | 11 +++++++++--
 kernel/sched/cpufreq_schedutil.c |  2 +-
 kernel/sched/fair.c              |  6 +++---
 kernel/sched/sched.h             | 19 ++-----------------
 5 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 063cd120b459..926b944dae5e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1926,6 +1926,27 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
 #define TASK_SIZE_OF(tsk)	TASK_SIZE
 #endif
 
+#ifdef CONFIG_SMP
+/**
+ * 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 sched_cpu_util() to differentiate the types of
+ * utilization expected by the callers, and adjust the aggregation accordingly.
+ */
+enum cpu_util_type {
+	FREQUENCY_UTIL,
+	ENERGY_UTIL,
+};
+
+/* Returns effective CPU utilization, as seen by the scheduler */
+unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
+			     unsigned long max);
+#endif /* CONFIG_SMP */
+
 #ifdef CONFIG_RSEQ
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b81265aec4a0..845c976ccd53 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5138,8 +5138,8 @@ struct task_struct *idle_task(int cpu)
  * 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,
+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;
@@ -5223,6 +5223,13 @@ unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
 
 	return min(max, util);
 }
+
+unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
+			     unsigned long max)
+{
+	return effective_cpu_util(cpu, cpu_util_cfs(cpu_rq(cpu)), max, type,
+				  NULL);
+}
 #endif /* CONFIG_SMP */
 
 /**
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 2d44befb322b..e71627a3792b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -178,7 +178,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);
 }
 
 /**
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 290f9e38378c..0e1c8eb7ad53 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6499,7 +6499,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);
 
 		/*
@@ -6509,7 +6509,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);
 	}
@@ -6607,7 +6607,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 sched_cpu_util().
 			 */
 			util = uclamp_rq_util_with(cpu_rq(cpu), util, p);
 			if (!fits_capacity(util, cpu_cap))
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0db6bcf0881f..4fab3b930ace 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2485,23 +2485,8 @@ static inline unsigned long capacity_orig_of(int cpu)
 	return cpu_rq(cpu)->cpu_capacity_orig;
 }
 
-/**
- * enum schedutil_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
- * utilization expected by the callers, and adjust the aggregation accordingly.
- */
-enum schedutil_type {
-	FREQUENCY_UTIL,
-	ENERGY_UTIL,
-};
-
-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)
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Viresh Kumar]

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.

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.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
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..5aff2ac4b77f 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);
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V4 2/3] sched/core: Rename schedutil_cpu_util() and allow rest of the kernel to use it

[Quentin Perret]

Hey Viresh,

On Tuesday 24 Nov 2020 at 11:56:15 (+0530), Viresh Kumar wrote:
> There is nothing schedutil specific in schedutil_cpu_util(), rename it
> to effective_cpu_util(). Also create and expose another wrapper
> sched_cpu_util() which can be used by other parts of the kernel, like
> thermal core (that will be done in a later commit).
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  include/linux/sched.h            | 21 +++++++++++++++++++++
>  kernel/sched/core.c              | 11 +++++++++--
>  kernel/sched/cpufreq_schedutil.c |  2 +-
>  kernel/sched/fair.c              |  6 +++---
>  kernel/sched/sched.h             | 19 ++-----------------
>  5 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 063cd120b459..926b944dae5e 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1926,6 +1926,27 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
>  #define TASK_SIZE_OF(tsk)	TASK_SIZE
>  #endif
>  
> +#ifdef CONFIG_SMP
> +/**
> + * 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 sched_cpu_util() to differentiate the types of
> + * utilization expected by the callers, and adjust the aggregation accordingly.
> + */
> +enum cpu_util_type {
> +	FREQUENCY_UTIL,
> +	ENERGY_UTIL,
> +};
> +
> +/* Returns effective CPU utilization, as seen by the scheduler */
> +unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
> +			     unsigned long max);

Are 'type' and 'max' useful to anybody outside of kernel/sched ?
If not then how about we hide them, keep the cpu_util_type enum in
kernel/sched/sched.h and evaluate arch_scale_cpu_capacity() in
sched_cpu_util() directly?

Thanks,
Quentin

Re: [PATCH V4 2/3] sched/core: Rename schedutil_cpu_util() and allow rest of the kernel to use it

[Viresh Kumar]

On 24-11-20, 09:10, Quentin Perret wrote:
> Hey Viresh,
> 
> On Tuesday 24 Nov 2020 at 11:56:15 (+0530), Viresh Kumar wrote:
> > There is nothing schedutil specific in schedutil_cpu_util(), rename it
> > to effective_cpu_util(). Also create and expose another wrapper
> > sched_cpu_util() which can be used by other parts of the kernel, like
> > thermal core (that will be done in a later commit).
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  include/linux/sched.h            | 21 +++++++++++++++++++++
> >  kernel/sched/core.c              | 11 +++++++++--
> >  kernel/sched/cpufreq_schedutil.c |  2 +-
> >  kernel/sched/fair.c              |  6 +++---
> >  kernel/sched/sched.h             | 19 ++-----------------
> >  5 files changed, 36 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 063cd120b459..926b944dae5e 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1926,6 +1926,27 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
> >  #define TASK_SIZE_OF(tsk)	TASK_SIZE
> >  #endif
> >  
> > +#ifdef CONFIG_SMP
> > +/**
> > + * 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 sched_cpu_util() to differentiate the types of
> > + * utilization expected by the callers, and adjust the aggregation accordingly.
> > + */
> > +enum cpu_util_type {
> > +	FREQUENCY_UTIL,
> > +	ENERGY_UTIL,
> > +};
> > +
> > +/* Returns effective CPU utilization, as seen by the scheduler */
> > +unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
> > +			     unsigned long max);
> 
> Are 'type' and 'max' useful to anybody outside of kernel/sched ?
> If not then how about we hide them, keep the cpu_util_type enum in
> kernel/sched/sched.h and evaluate arch_scale_cpu_capacity() in
> sched_cpu_util() directly?

cpufreq_cooling uses 'max' (as can be seen in the next patch).

-- 
viresh

Re: [PATCH V4 1/3] sched/core: Move schedutil_cpu_util() to core.c

[Rafael J. Wysocki]

On Tue, Nov 24, 2020 at 7:26 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> There is nothing schedutil specific in schedutil_cpu_util(), move it to
> core.c and define it only for CONFIG_SMP.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

For the schedutil part:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  kernel/sched/core.c              | 108 +++++++++++++++++++++++++++++++
>  kernel/sched/cpufreq_schedutil.c | 106 ------------------------------
>  kernel/sched/sched.h             |  12 +---
>  3 files changed, 109 insertions(+), 117 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index d2003a7d5ab5..b81265aec4a0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5117,6 +5117,114 @@ struct task_struct *idle_task(int cpu)
>         return cpu_rq(cpu)->idle;
>  }
>
> +#ifdef CONFIG_SMP
> +/*
> + * 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);
> +}
> +#endif /* CONFIG_SMP */
> +
>  /**
>   * find_process_by_pid - find a process with a matching PID value.
>   * @pid: the pid in question.
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e254745a82cb..2d44befb322b 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -169,112 +169,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);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index df80bfcea92e..0db6bcf0881f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2484,7 +2484,6 @@ static inline unsigned long capacity_orig_of(int cpu)
>  {
>         return cpu_rq(cpu)->cpu_capacity_orig;
>  }
> -#endif
>
>  /**
>   * enum schedutil_type - CPU utilization type
> @@ -2501,8 +2500,6 @@ enum schedutil_type {
>         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,
>                                  struct task_struct *p);
> @@ -2533,14 +2530,7 @@ 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 */
> +#endif
>
>  #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
>  static inline unsigned long cpu_util_irq(struct rq *rq)
> --
> 2.25.0.rc1.19.g042ed3e048af
>

Re: [PATCH V4 2/3] sched/core: Rename schedutil_cpu_util() and allow rest of the kernel to use it

[Rafael J. Wysocki]

On Tue, Nov 24, 2020 at 7:26 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> There is nothing schedutil specific in schedutil_cpu_util(), rename it
> to effective_cpu_util(). Also create and expose another wrapper
> sched_cpu_util() which can be used by other parts of the kernel, like
> thermal core (that will be done in a later commit).
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

For the schedutil change:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  include/linux/sched.h            | 21 +++++++++++++++++++++
>  kernel/sched/core.c              | 11 +++++++++--
>  kernel/sched/cpufreq_schedutil.c |  2 +-
>  kernel/sched/fair.c              |  6 +++---
>  kernel/sched/sched.h             | 19 ++-----------------
>  5 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 063cd120b459..926b944dae5e 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1926,6 +1926,27 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
>  #define TASK_SIZE_OF(tsk)      TASK_SIZE
>  #endif
>
> +#ifdef CONFIG_SMP
> +/**
> + * 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 sched_cpu_util() to differentiate the types of
> + * utilization expected by the callers, and adjust the aggregation accordingly.
> + */
> +enum cpu_util_type {
> +       FREQUENCY_UTIL,
> +       ENERGY_UTIL,
> +};
> +
> +/* Returns effective CPU utilization, as seen by the scheduler */
> +unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
> +                            unsigned long max);
> +#endif /* CONFIG_SMP */
> +
>  #ifdef CONFIG_RSEQ
>
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b81265aec4a0..845c976ccd53 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5138,8 +5138,8 @@ struct task_struct *idle_task(int cpu)
>   * 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,
> +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;
> @@ -5223,6 +5223,13 @@ unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
>
>         return min(max, util);
>  }
> +
> +unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
> +                            unsigned long max)
> +{
> +       return effective_cpu_util(cpu, cpu_util_cfs(cpu_rq(cpu)), max, type,
> +                                 NULL);
> +}
>  #endif /* CONFIG_SMP */
>
>  /**
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 2d44befb322b..e71627a3792b 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -178,7 +178,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);
>  }
>
>  /**
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 290f9e38378c..0e1c8eb7ad53 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6499,7 +6499,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);
>
>                 /*
> @@ -6509,7 +6509,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);
>         }
> @@ -6607,7 +6607,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 sched_cpu_util().
>                          */
>                         util = uclamp_rq_util_with(cpu_rq(cpu), util, p);
>                         if (!fits_capacity(util, cpu_cap))
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 0db6bcf0881f..4fab3b930ace 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2485,23 +2485,8 @@ static inline unsigned long capacity_orig_of(int cpu)
>         return cpu_rq(cpu)->cpu_capacity_orig;
>  }
>
> -/**
> - * enum schedutil_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
> - * utilization expected by the callers, and adjust the aggregation accordingly.
> - */
> -enum schedutil_type {
> -       FREQUENCY_UTIL,
> -       ENERGY_UTIL,
> -};
> -
> -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)
> --
> 2.25.0.rc1.19.g042ed3e048af
>

Re: [PATCH V4 2/3] sched/core: Rename schedutil_cpu_util() and allow rest of the kernel to use it

[Dietmar Eggemann]

On 24/11/2020 14:22, Viresh Kumar wrote:
> On 24-11-20, 09:10, Quentin Perret wrote:
>> Hey Viresh,
>>
>> On Tuesday 24 Nov 2020 at 11:56:15 (+0530), Viresh Kumar wrote:
>>> There is nothing schedutil specific in schedutil_cpu_util(), rename it
>>> to effective_cpu_util(). Also create and expose another wrapper
>>> sched_cpu_util() which can be used by other parts of the kernel, like
>>> thermal core (that will be done in a later commit).
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>  include/linux/sched.h            | 21 +++++++++++++++++++++
>>>  kernel/sched/core.c              | 11 +++++++++--
>>>  kernel/sched/cpufreq_schedutil.c |  2 +-
>>>  kernel/sched/fair.c              |  6 +++---
>>>  kernel/sched/sched.h             | 19 ++-----------------
>>>  5 files changed, 36 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index 063cd120b459..926b944dae5e 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -1926,6 +1926,27 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
>>>  #define TASK_SIZE_OF(tsk)	TASK_SIZE
>>>  #endif
>>>  
>>> +#ifdef CONFIG_SMP
>>> +/**
>>> + * 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 sched_cpu_util() to differentiate the types of
>>> + * utilization expected by the callers, and adjust the aggregation accordingly.
>>> + */
>>> +enum cpu_util_type {
>>> +	FREQUENCY_UTIL,
>>> +	ENERGY_UTIL,
>>> +};
>>> +
>>> +/* Returns effective CPU utilization, as seen by the scheduler */
>>> +unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
>>> +			     unsigned long max);
>>
>> Are 'type' and 'max' useful to anybody outside of kernel/sched ?
>> If not then how about we hide them, keep the cpu_util_type enum in
>> kernel/sched/sched.h and evaluate arch_scale_cpu_capacity() in
>> sched_cpu_util() directly?
> 
> cpufreq_cooling uses 'max' (as can be seen in the next patch).

But the enum cpu_util_type type doesn't have to be exported outside the
task scheduler code?

--8<--

diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index 5aff2ac4b77f..cd9d717654a8 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -149,7 +149,7 @@ static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
 	unsigned long max = arch_scale_cpu_capacity(cpu);
 	unsigned long util;
 
-	util = sched_cpu_util(cpu, ENERGY_UTIL, max);
+	util = sched_cpu_util(cpu, max);
 	return (util * 100) / max;
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index fcd70c075349..0511e4fb946f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1954,24 +1954,8 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
 #endif
 
 #ifdef CONFIG_SMP
-/**
- * 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 sched_cpu_util() to differentiate the types of
- * utilization expected by the callers, and adjust the aggregation accordingly.
- */
-enum cpu_util_type {
-	FREQUENCY_UTIL,
-	ENERGY_UTIL,
-};
-
 /* Returns effective CPU utilization, as seen by the scheduler */
-unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
-			     unsigned long max);
+unsigned long sched_cpu_util(int cpu, unsigned long max);
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_RSEQ
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f20dacc2fa7..270f10e01ad2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5757,11 +5757,10 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
 	return min(max, util);
 }
 
-unsigned long sched_cpu_util(int cpu, enum cpu_util_type type,
-			     unsigned long max)
+unsigned long sched_cpu_util(int cpu, unsigned long max)
 {
-	return effective_cpu_util(cpu, cpu_util_cfs(cpu_rq(cpu)), max, type,
-				  NULL);
+	return effective_cpu_util(cpu, cpu_util_cfs(cpu_rq(cpu)), max,
+				  ENERGY_UTIL, NULL);
 }
 #endif /* CONFIG_SMP */
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b7611020d5cf..a49d6c3e9147 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2561,6 +2561,21 @@ static inline unsigned long capacity_orig_of(int cpu)
 	return cpu_rq(cpu)->cpu_capacity_orig;
 }
 
+/**
+ * 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 sched_cpu_util() to differentiate the types of
+ * utilization expected by the callers, and adjust the aggregation accordingly.
+ */
+enum cpu_util_type {
+        FREQUENCY_UTIL,
+        ENERGY_UTIL,
+};
+
 unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
 				 unsigned long max, enum cpu_util_type type,
 				 struct task_struct *p);
-- 
2.17.1

 

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6645 bytes --]

Hi Viresh,

I love your patch! Yet something to improve:

[auto build test ERROR on 3650b228f83adda7e5ee532e2b90429c03f7b9ec]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
base:    3650b228f83adda7e5ee532e2b90429c03f7b9ec
config: powerpc64-randconfig-r025-20201202 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2671fccf0381769276ca8246ec0499adcb9b0355)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/17f0c7f4372070206925c3a10ec0e7a09d03615e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
        git checkout 17f0c7f4372070206925c3a10ec0e7a09d03615e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/thermal/cpufreq_cooling.c:562:8: error: implicit declaration of function 'allocate_idle_time' [-Werror,-Wimplicit-function-declaration]
           ret = allocate_idle_time(cpufreq_cdev);
                 ^
>> drivers/thermal/cpufreq_cooling.c:624:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
           free_idle_time(cpufreq_cdev);
           ^
   drivers/thermal/cpufreq_cooling.c:717:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
           free_idle_time(cpufreq_cdev);
           ^
   3 errors generated.

vim +/allocate_idle_time +562 drivers/thermal/cpufreq_cooling.c

   509	
   510	/**
   511	 * __cpufreq_cooling_register - helper function to create cpufreq cooling device
   512	 * @np: a valid struct device_node to the cooling device device tree node
   513	 * @policy: cpufreq policy
   514	 * Normally this should be same as cpufreq policy->related_cpus.
   515	 * @em: Energy Model of the cpufreq policy
   516	 *
   517	 * This interface function registers the cpufreq cooling device with the name
   518	 * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
   519	 * cooling devices. It also gives the opportunity to link the cooling device
   520	 * with a device tree node, in order to bind it via the thermal DT code.
   521	 *
   522	 * Return: a valid struct thermal_cooling_device pointer on success,
   523	 * on failure, it returns a corresponding ERR_PTR().
   524	 */
   525	static struct thermal_cooling_device *
   526	__cpufreq_cooling_register(struct device_node *np,
   527				struct cpufreq_policy *policy,
   528				struct em_perf_domain *em)
   529	{
   530		struct thermal_cooling_device *cdev;
   531		struct cpufreq_cooling_device *cpufreq_cdev;
   532		char dev_name[THERMAL_NAME_LENGTH];
   533		unsigned int i;
   534		struct device *dev;
   535		int ret;
   536		struct thermal_cooling_device_ops *cooling_ops;
   537	
   538		dev = get_cpu_device(policy->cpu);
   539		if (unlikely(!dev)) {
   540			pr_warn("No cpu device for cpu %d\n", policy->cpu);
   541			return ERR_PTR(-ENODEV);
   542		}
   543	
   544		if (IS_ERR_OR_NULL(policy)) {
   545			pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy);
   546			return ERR_PTR(-EINVAL);
   547		}
   548	
   549		i = cpufreq_table_count_valid_entries(policy);
   550		if (!i) {
   551			pr_debug("%s: CPUFreq table not found or has no valid entries\n",
   552				 __func__);
   553			return ERR_PTR(-ENODEV);
   554		}
   555	
   556		cpufreq_cdev = kzalloc(sizeof(*cpufreq_cdev), GFP_KERNEL);
   557		if (!cpufreq_cdev)
   558			return ERR_PTR(-ENOMEM);
   559	
   560		cpufreq_cdev->policy = policy;
   561	
 > 562		ret = allocate_idle_time(cpufreq_cdev);
   563		if (ret) {
   564			cdev = ERR_PTR(ret);
   565			goto free_cdev;
   566		}
   567	
   568		/* max_level is an index, not a counter */
   569		cpufreq_cdev->max_level = i - 1;
   570	
   571		ret = ida_simple_get(&cpufreq_ida, 0, 0, GFP_KERNEL);
   572		if (ret < 0) {
   573			cdev = ERR_PTR(ret);
   574			goto free_idle_time;
   575		}
   576		cpufreq_cdev->id = ret;
   577	
   578		snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
   579			 cpufreq_cdev->id);
   580	
   581		cooling_ops = &cpufreq_cooling_ops;
   582	
   583	#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
   584		if (em_is_sane(cpufreq_cdev, em)) {
   585			cpufreq_cdev->em = em;
   586			cooling_ops->get_requested_power = cpufreq_get_requested_power;
   587			cooling_ops->state2power = cpufreq_state2power;
   588			cooling_ops->power2state = cpufreq_power2state;
   589		} else
   590	#endif
   591		if (policy->freq_table_sorted == CPUFREQ_TABLE_UNSORTED) {
   592			pr_err("%s: unsorted frequency tables are not supported\n",
   593			       __func__);
   594			cdev = ERR_PTR(-EINVAL);
   595			goto remove_ida;
   596		}
   597	
   598		ret = freq_qos_add_request(&policy->constraints,
   599					   &cpufreq_cdev->qos_req, FREQ_QOS_MAX,
   600					   get_state_freq(cpufreq_cdev, 0));
   601		if (ret < 0) {
   602			pr_err("%s: Failed to add freq constraint (%d)\n", __func__,
   603			       ret);
   604			cdev = ERR_PTR(ret);
   605			goto remove_ida;
   606		}
   607	
   608		cdev = thermal_of_cooling_device_register(np, dev_name, cpufreq_cdev,
   609							  cooling_ops);
   610		if (IS_ERR(cdev))
   611			goto remove_qos_req;
   612	
   613		mutex_lock(&cooling_list_lock);
   614		list_add(&cpufreq_cdev->node, &cpufreq_cdev_list);
   615		mutex_unlock(&cooling_list_lock);
   616	
   617		return cdev;
   618	
   619	remove_qos_req:
   620		freq_qos_remove_request(&cpufreq_cdev->qos_req);
   621	remove_ida:
   622		ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id);
   623	free_idle_time:
 > 624		free_idle_time(cpufreq_cdev);
   625	free_cdev:
   626		kfree(cpufreq_cdev);
   627		return cdev;
   628	}
   629	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33784 bytes --]

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Dietmar Eggemann]

On 24/11/2020 07:26, 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.
> 
> 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

When I ran schbench (-t 16 -r 5) on hikey960 I get multiple (~50)
instances of ~80ms task activity phase and then ~20ms idle phase on all
CPUs.

So I assume that scenario 1 is at the beginning (but you mentioned the
task were sleeping?) and scenario 2 is somewhere in the middle of the
testrun?
IMHO, the util-based approach delivers really better results at the
beginning and at the end of the entire testrun.
During the testrun, the util-based and the idle-based approach deliver
similar results.

It's a little bit tricky to compare test results since the IPA sampling
rate is 100ms and the load values you get depend on how the workload
pattern and the IPA sampling align.

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

I assume the IPA sampling is done after ~50ms of the first task activity
phase.

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

Is this an IPA sampling at the end of the ~20ms idle phase?

[...]

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

On 03-12-20, 02:39, kernel test robot wrote:
> Hi Viresh,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on 3650b228f83adda7e5ee532e2b90429c03f7b9ec]
> 
> url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
> base:    3650b228f83adda7e5ee532e2b90429c03f7b9ec
> config: powerpc64-randconfig-r025-20201202 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2671fccf0381769276ca8246ec0499adcb9b0355)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install powerpc64 cross compiling tool for clang build
>         # apt-get install binutils-powerpc64-linux-gnu
>         # https://github.com/0day-ci/linux/commit/17f0c7f4372070206925c3a10ec0e7a09d03615e
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
>         git checkout 17f0c7f4372070206925c3a10ec0e7a09d03615e
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/thermal/cpufreq_cooling.c:562:8: error: implicit declaration of function 'allocate_idle_time' [-Werror,-Wimplicit-function-declaration]
>            ret = allocate_idle_time(cpufreq_cdev);
>                  ^
> >> drivers/thermal/cpufreq_cooling.c:624:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
>            free_idle_time(cpufreq_cdev);
>            ^
>    drivers/thermal/cpufreq_cooling.c:717:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
>            free_idle_time(cpufreq_cdev);
>            ^
>    3 errors generated.
> 
> vim +/allocate_idle_time +562 drivers/thermal/cpufreq_cooling.c

I am not sure why this should happen here, I don't see any such errors
on my side. Can someone please have another look ?

-- 
viresh

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Viresh Kumar]

On 03-12-20, 12:54, Dietmar Eggemann wrote:
> On 24/11/2020 07:26, 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.
> > 
> > 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
> 
> When I ran schbench (-t 16 -r 5) on hikey960 I get multiple (~50)
> instances of ~80ms task activity phase and then ~20ms idle phase on all
> CPUs.
> 
> So I assume that scenario 1 is at the beginning (but you mentioned the
> task were sleeping?)

I am not able to find the exact values I used, but I did something
like this to create a scenario where the old computations shall find
the CPU as idle in the last IPA window:

- schbench -m 2 -t 4 -s 25000 -c 20000 -r 60

- sampling rate of IPA to 10 ms

With this IPA wakes up many times and finds the CPUs to have been idle
in the last IPA window (i.e. 10ms).

> and scenario 2 is somewhere in the middle of the
> testrun?

This also happens all the time, as there will be cases when the IPA
runs and finds the CPUs to be always running in last 10 ms.

> IMHO, the util-based approach delivers really better results at the
> beginning and at the end of the entire testrun.
> During the testrun, the util-based and the idle-based approach deliver
> similar results.
> 
> It's a little bit tricky to compare test results since the IPA sampling
> rate is 100ms and the load values you get depend on how the workload
> pattern and the IPA sampling align.

Right.

> > 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.
> 
> I assume the IPA sampling is done after ~50ms of the first task activity
> phase.
> 
> > 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.
> 
> Is this an IPA sampling at the end of the ~20ms idle phase?

This is during the phase where the CPUs were fully busy for the last
period.

-- 
viresh

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Nick Desaulniers]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

On Mon, Dec 7, 2020 at 1:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-12-20, 02:39, kernel test robot wrote:
> > Hi Viresh,
> >
> > I love your patch! Yet something to improve:
> >
> > [auto build test ERROR on 3650b228f83adda7e5ee532e2b90429c03f7b9ec]
> >
> > url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
> > base:    3650b228f83adda7e5ee532e2b90429c03f7b9ec
> > config: powerpc64-randconfig-r025-20201202 (attached as .config)

^ Note: randconfig

> > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2671fccf0381769276ca8246ec0499adcb9b0355)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install powerpc64 cross compiling tool for clang build
> >         # apt-get install binutils-powerpc64-linux-gnu
> >         # https://github.com/0day-ci/linux/commit/17f0c7f4372070206925c3a10ec0e7a09d03615e
> >         git remote add linux-review https://github.com/0day-ci/linux
> >         git fetch --no-tags linux-review Viresh-Kumar/cpufreq_cooling-Get-effective-CPU-utilization-from-scheduler/20201124-143027
> >         git checkout 17f0c7f4372070206925c3a10ec0e7a09d03615e
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/thermal/cpufreq_cooling.c:562:8: error: implicit declaration of function 'allocate_idle_time' [-Werror,-Wimplicit-function-declaration]
> >            ret = allocate_idle_time(cpufreq_cdev);
> >                  ^

I can see in the top commit
(https://github.com/0day-ci/linux/commit/17f0c7f4372070206925c3a10ec0e7a09d03615e)
you've modified __cpufreq_cooling_register to now call
allocate_idle_time. -Wimplicit-function-declaration is observed when
calling a function for which there has been no previous declaration.
Let's look and see where or under what config allocate_idle_time is
declared, and how a randconfig might expose the missing declaration.
(The first thing I suspect is transitive header dependencies, where
some intermediary header changes what it includes based on
#define/CONFIGs, or calling a function before it's been declared).

It looks like allocate_idle_time is declared in the same
commit...while it's defined twice (once for CONFIG_SMP, once without),
it's not defined when CONFIG_THERMAL_GOV_POWER_ALLOCATOR is not
enabled, which is probably what the randconfig has tickled.

> > >> drivers/thermal/cpufreq_cooling.c:624:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
> >            free_idle_time(cpufreq_cdev);
> >            ^
> >    drivers/thermal/cpufreq_cooling.c:717:2: error: implicit declaration of function 'free_idle_time' [-Werror,-Wimplicit-function-declaration]
> >            free_idle_time(cpufreq_cdev);
> >            ^
> >    3 errors generated.
> >
> > vim +/allocate_idle_time +562 drivers/thermal/cpufreq_cooling.c
>
> I am not sure why this should happen here, I don't see any such errors
> on my side. Can someone please have another look ?
>
> --
> viresh
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe(a)googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201207094419.lakxblzsono3nqpf%40vireshk-i7.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 07-12-20, 11:59, Nick Desaulniers wrote:
> I can see in the top commit
> (https://github.com/0day-ci/linux/commit/17f0c7f4372070206925c3a10ec0e7a09d03615e)
> you've modified __cpufreq_cooling_register to now call
> allocate_idle_time. -Wimplicit-function-declaration is observed when
> calling a function for which there has been no previous declaration.
> Let's look and see where or under what config allocate_idle_time is
> declared, and how a randconfig might expose the missing declaration.
> (The first thing I suspect is transitive header dependencies, where
> some intermediary header changes what it includes based on
> #define/CONFIGs, or calling a function before it's been declared).
> 
> It looks like allocate_idle_time is declared in the same
> commit...while it's defined twice (once for CONFIG_SMP, once without),
> it's not defined when CONFIG_THERMAL_GOV_POWER_ALLOCATOR is not
> enabled, which is probably what the randconfig has tickled.

And I was only looking only at CONFIG_SMP to see why we couldn't find
a definition here :(

-- 
viresh

Re: [PATCH V4 3/3] thermal: cpufreq_cooling: Reuse sched_cpu_util() for SMP platforms

[Dietmar Eggemann]

On 07/12/2020 13:17, Viresh Kumar wrote:
> On 03-12-20, 12:54, Dietmar Eggemann wrote:
>> On 24/11/2020 07:26, Viresh Kumar wrote:

[...]

>> When I ran schbench (-t 16 -r 5) on hikey960 I get multiple (~50)
>> instances of ~80ms task activity phase and then ~20ms idle phase on all
>> CPUs.
>>
>> So I assume that scenario 1 is at the beginning (but you mentioned the
>> task were sleeping?)
> 
> I am not able to find the exact values I used, but I did something
> like this to create a scenario where the old computations shall find
> the CPU as idle in the last IPA window:
> 
> - schbench -m 2 -t 4 -s 25000 -c 20000 -r 60
> 
> - sampling rate of IPA to 10 ms
> 
> With this IPA wakes up many times and finds the CPUs to have been idle
> in the last IPA window (i.e. 10ms).

Ah, this makes sense.

So with this there are only 8 worker threads w/ 20ms runtime and 75ms
period (30ms message thread time (-C) and 25 latency (-c)).

So much more idle time between two invocations of the worker/message
threads and more IPA sampling.

[...]

>>>  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.
>>
>> Is this an IPA sampling at the end of the ~20ms idle phase?
> 
> This is during the phase where the CPUs were fully busy for the last
> period.

OK.