| Message ID | 20240108134843.429769-3-vincent.guittot@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rework system pressure interface to the scheduler | expand |
On 08/01/2024 14:48, Vincent Guittot wrote: > Aggregate the different pressures applied on the capacity of CPUs and > create a new function that returns the actual capacity of the CPU: > get_actual_cpu_capacity() function name scaling (1) arch_scale_cpu_capacity() - uarch (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1) (3) capacity_of() - rt (rt/dl/irq) of (2) (used by fair) Although (1) - (3) are very close to each other from the functional standpoint, their names are not very coherent. I assume this makes it hard to understand all of this when reading the code w/o knowing these patches before. Why is (2) tagged with 'actual'? This is especially visible in feec() where local variable cpu_cap relates to (3) whereas cpu_actual_cap related to (2). [...]
On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 08/01/2024 14:48, Vincent Guittot wrote: > > Aggregate the different pressures applied on the capacity of CPUs and > > create a new function that returns the actual capacity of the CPU: > > get_actual_cpu_capacity() > > function name scaling > > (1) arch_scale_cpu_capacity() - uarch > > (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1) > > (3) capacity_of() - rt (rt/dl/irq) of (2) (used by fair) > > Although (1) - (3) are very close to each other from the functional I don't get your point as name of (1) and (3) have not been changed by the patch > standpoint, their names are not very coherent. > > I assume this makes it hard to understand all of this when reading the > code w/o knowing these patches before. > > Why is (2) tagged with 'actual'? This is the actual max compute capacity of the cpu at now i.e. possibly reduced because of temporary frequency capping So (2) equals (1) minus temporary performance capping and (3) additionally subtracts the time used by other class to (2) > > This is especially visible in feec() where local variable cpu_cap > relates to (3) whereas cpu_actual_cap related to (2). > > [...] >
On 09/01/2024 15:30, Vincent Guittot wrote: > On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >> >> On 08/01/2024 14:48, Vincent Guittot wrote: >>> Aggregate the different pressures applied on the capacity of CPUs and >>> create a new function that returns the actual capacity of the CPU: >>> get_actual_cpu_capacity() >> >> function name scaling >> >> (1) arch_scale_cpu_capacity() - uarch >> >> (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1) >> >> (3) capacity_of() - rt (rt/dl/irq) of (2) (used by fair) >> >> Although (1) - (3) are very close to each other from the functional > > I don't get your point as name of (1) and (3) have not been changed by the patch That's true. But with capacity_orig_of() for (1), we had some coherence in the naming scheme of those cpu_capacity related functions (1) - (3). which helps when trying to understand the code. I can see that actual_capacity_of() (2) sounds awful though. >> standpoint, their names are not very coherent. >> >> I assume this makes it hard to understand all of this when reading the >> code w/o knowing these patches before. >> >> Why is (2) tagged with 'actual'? > > This is the actual max compute capacity of the cpu at now i.e. > possibly reduced because of temporary frequency capping Will the actual max compute capacity also depend on 'user space system pressure' later, i.e. on 'permanent' frequency capping? > So (2) equals (1) minus temporary performance capping and (3) > additionally subtracts the time used by other class to (2) OK. A coherent set of those tags even reflected in those getters would help but can be done later too.
On Wed, 10 Jan 2024 at 14:51, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 09/01/2024 15:30, Vincent Guittot wrote: > > On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > >> > >> On 08/01/2024 14:48, Vincent Guittot wrote: > >>> Aggregate the different pressures applied on the capacity of CPUs and > >>> create a new function that returns the actual capacity of the CPU: > >>> get_actual_cpu_capacity() > >> > >> function name scaling > >> > >> (1) arch_scale_cpu_capacity() - uarch > >> > >> (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1) > >> > >> (3) capacity_of() - rt (rt/dl/irq) of (2) (used by fair) > >> > >> Although (1) - (3) are very close to each other from the functional > > > > I don't get your point as name of (1) and (3) have not been changed by the patch > > That's true. But with capacity_orig_of() for (1), we had some coherence > in the naming scheme of those cpu_capacity related functions (1) - (3). > which helps when trying to understand the code. > > I can see that actual_capacity_of() (2) sounds awful though. > > >> standpoint, their names are not very coherent. > >> > >> I assume this makes it hard to understand all of this when reading the > >> code w/o knowing these patches before. > >> > >> Why is (2) tagged with 'actual'? > > > > This is the actual max compute capacity of the cpu at now i.e. > > possibly reduced because of temporary frequency capping > > Will the actual max compute capacity also depend on 'user space system > pressure' later, i.e. on 'permanent' frequency capping? yes it will > > > So (2) equals (1) minus temporary performance capping and (3) > > additionally subtracts the time used by other class to (2) > > OK. > > A coherent set of those tags even reflected in those getters would help > but can be done later too.
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 9cc20855dc2b..e54bbf8b4936 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4910,13 +4910,22 @@ static inline void util_est_update(struct cfs_rq *cfs_rq, trace_sched_util_est_se_tp(&p->se); } +static inline unsigned long get_actual_cpu_capacity(int cpu) +{ + unsigned long capacity = arch_scale_cpu_capacity(cpu); + + capacity -= max(thermal_load_avg(cpu_rq(cpu)), cpufreq_get_pressure(cpu)); + + return capacity; +} + static inline int util_fits_cpu(unsigned long util, unsigned long uclamp_min, unsigned long uclamp_max, int cpu) { - unsigned long capacity_orig, capacity_orig_thermal; unsigned long capacity = capacity_of(cpu); + unsigned long capacity_orig; bool fits, uclamp_max_fits; /* @@ -4948,7 +4957,6 @@ static inline int util_fits_cpu(unsigned long util, * goal is to cap the task. So it's okay if it's getting less. */ capacity_orig = arch_scale_cpu_capacity(cpu); - capacity_orig_thermal = capacity_orig - arch_scale_thermal_pressure(cpu); /* * We want to force a task to fit a cpu as implied by uclamp_max. @@ -5023,7 +5031,8 @@ static inline int util_fits_cpu(unsigned long util, * handle the case uclamp_min > uclamp_max. */ uclamp_min = min(uclamp_min, uclamp_max); - if (fits && (util < uclamp_min) && (uclamp_min > capacity_orig_thermal)) + if (fits && (util < uclamp_min) && + (uclamp_min > get_actual_cpu_capacity(cpu))) return -1; return fits; @@ -7404,7 +7413,7 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target) * Look for the CPU with best capacity. */ else if (fits < 0) - cpu_cap = arch_scale_cpu_capacity(cpu) - thermal_load_avg(cpu_rq(cpu)); + cpu_cap = get_actual_cpu_capacity(cpu); /* * First, select CPU which fits better (-1 being better than 0). @@ -7897,8 +7906,8 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) struct root_domain *rd = this_rq()->rd; int cpu, best_energy_cpu, target = -1; int prev_fits = -1, best_fits = -1; - unsigned long best_thermal_cap = 0; - unsigned long prev_thermal_cap = 0; + unsigned long best_actual_cap = 0; + unsigned long prev_actual_cap = 0; struct sched_domain *sd; struct perf_domain *pd; struct energy_env eenv; @@ -7928,7 +7937,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) for (; pd; pd = pd->next) { unsigned long util_min = p_util_min, util_max = p_util_max; - unsigned long cpu_cap, cpu_thermal_cap, util; + unsigned long cpu_cap, cpu_actual_cap, util; long prev_spare_cap = -1, max_spare_cap = -1; unsigned long rq_util_min, rq_util_max; unsigned long cur_delta, base_energy; @@ -7940,18 +7949,17 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) if (cpumask_empty(cpus)) continue; - /* Account thermal pressure for the energy estimation */ + /* Account external pressure for the energy estimation */ cpu = cpumask_first(cpus); - cpu_thermal_cap = arch_scale_cpu_capacity(cpu); - cpu_thermal_cap -= arch_scale_thermal_pressure(cpu); + cpu_actual_cap = get_actual_cpu_capacity(cpu); - eenv.cpu_cap = cpu_thermal_cap; + eenv.cpu_cap = cpu_actual_cap; eenv.pd_cap = 0; for_each_cpu(cpu, cpus) { struct rq *rq = cpu_rq(cpu); - eenv.pd_cap += cpu_thermal_cap; + eenv.pd_cap += cpu_actual_cap; if (!cpumask_test_cpu(cpu, sched_domain_span(sd))) continue; @@ -8022,7 +8030,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) if (prev_delta < base_energy) goto unlock; prev_delta -= base_energy; - prev_thermal_cap = cpu_thermal_cap; + prev_actual_cap = cpu_actual_cap; best_delta = min(best_delta, prev_delta); } @@ -8037,7 +8045,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) * but best energy cpu has better capacity. */ if ((max_fits < 0) && - (cpu_thermal_cap <= best_thermal_cap)) + (cpu_actual_cap <= best_actual_cap)) continue; cur_delta = compute_energy(&eenv, pd, cpus, p, @@ -8058,14 +8066,14 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) best_delta = cur_delta; best_energy_cpu = max_spare_cap_cpu; best_fits = max_fits; - best_thermal_cap = cpu_thermal_cap; + best_actual_cap = cpu_actual_cap; } } rcu_read_unlock(); if ((best_fits > prev_fits) || ((best_fits > 0) && (best_delta < prev_delta)) || - ((best_fits < 0) && (best_thermal_cap > prev_thermal_cap))) + ((best_fits < 0) && (best_actual_cap > prev_actual_cap))) target = best_energy_cpu; return target; @@ -9441,8 +9449,8 @@ static inline void init_sd_lb_stats(struct sd_lb_stats *sds) static unsigned long scale_rt_capacity(int cpu) { + unsigned long max = get_actual_cpu_capacity(cpu); struct rq *rq = cpu_rq(cpu); - unsigned long max = arch_scale_cpu_capacity(cpu); unsigned long used, free; unsigned long irq; @@ -9454,12 +9462,9 @@ static unsigned long scale_rt_capacity(int cpu) /* * avg_rt.util_avg and avg_dl.util_avg track binary signals * (running and not running) with weights 0 and 1024 respectively. - * avg_thermal.load_avg tracks thermal pressure and the weighted - * average uses the actual delta max capacity(load). */ used = READ_ONCE(rq->avg_rt.util_avg); used += READ_ONCE(rq->avg_dl.util_avg); - used += thermal_load_avg(rq); if (unlikely(used >= max)) return 1;