Re: [PATCH v8 6/7] sched/fair: Remove task_util from effective utilization in feec()

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

+ vdonnefort@gmail.com
- vincent.donnefort@arm.com

On 29/04/2022 16:11, Vincent Donnefort wrote:

[...]

> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

[...]

> -static long
> -compute_energy(struct task_struct *p, int dst_cpu, struct cpumask *cpus,
> -	       struct perf_domain *pd)
> +static inline void eenv_task_busy_time(struct energy_env *eenv,
> +				       struct task_struct *p, int prev_cpu)
>  {
> -	unsigned long max_util = 0, sum_util = 0, cpu_cap;
> +	unsigned long max_cap = arch_scale_cpu_capacity(prev_cpu);
> +	unsigned long irq = cpu_util_irq(cpu_rq(prev_cpu));
> +
> +	if (unlikely(irq >= max_cap)) {
> +		eenv->task_busy_time = max_cap;
> +		return;
> +	}
> +
> +	eenv->task_busy_time =
> +		scale_irq_capacity(task_util_est(p), irq, max_cap);

Nit-pick:

Maybe a little bit lighter:

 static inline void eenv_task_busy_time(struct energy_env *eenv,
                                       struct task_struct *p, int prev_cpu)
 {
-       unsigned long max_cap = arch_scale_cpu_capacity(prev_cpu);
+       unsigned long busy_time, max_cap = arch_scale_cpu_capacity(prev_cpu);
        unsigned long irq = cpu_util_irq(cpu_rq(prev_cpu));
 
-       if (unlikely(irq >= max_cap)) {
-               eenv->task_busy_time = max_cap;
-               return;
-       }
+       if (unlikely(irq >= max_cap))
+               busy_time = max_cap;
+       else
+               busy_time = scale_irq_capacity(task_util_est(p), irq, max_cap);
 
-       eenv->task_busy_time =
-               scale_irq_capacity(task_util_est(p), irq, max_cap);
+       eenv->task_busy_time = busy_time;
 }

[...]

> +static inline unsigned long
> +compute_energy(struct energy_env *eenv, struct perf_domain *pd,
> +	       struct cpumask *pd_cpus, struct task_struct *p, int dst_cpu)
> +{
> +	unsigned long max_util = eenv_pd_max_util(eenv, pd_cpus, p, dst_cpu);
> +	unsigned long busy_time = eenv->pd_busy_time;
> +
> +	if (dst_cpu >= 0)
> +		busy_time = min(eenv->pd_cap,
> +				eenv->pd_busy_time + eenv->task_busy_time);

Nit-pick:

        if (dst_cpu >= 0)
-               busy_time = min(eenv->pd_cap,
-                               eenv->pd_busy_time + eenv->task_busy_time);
+               busy_time = min(eenv->pd_cap, busy_time + eenv->task_busy_time);

[..]

> @@ -6905,12 +6976,14 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>  			continue;
>  
>  		/* Compute the 'base' energy of the pd, without @p */
> -		base_energy_pd = compute_energy(p, -1, cpus, pd);
> +		eenv_pd_busy_time(&eenv, cpus, p);

Nit-pick:

Move comment /* Compute the 'base' energy of the pd, without @p */ here
since eenv->pd_busy_time is also used to calculate prev_delta and cur_delta. 


> +		base_energy_pd = compute_energy(&eenv, pd, cpus, p, -1);>  		base_energy += base_energy_pd;

[...]