Re: [PATCH v2 1/2] sched/fair: Only compute base_energy_pd if necessary

[Dietmar Eggemann]

On 29/04/2021 12:19, Pierre.Gondois@xxxxxxx wrote:
> From: Pierre Gondois <Pierre.Gondois@xxxxxxx>
> 
> find_energy_efficient_cpu() searches the best energy CPU
> to place a task on. To do so, the energy of each performance domain
> (pd) is computed w/ and w/o the task placed in each pd.

s/in each pd/on it ?

> 
> The energy of a pd w/o the task (base_energy_pd) is computed prior
> knowing whether a CPU is available in the pd.
> 
> Move the base_energy_pd computation after looping through the CPUs
> of a pd and only computes it if at least one CPU is available.

s/computes/compute

[...]

> +		if (max_spare_cap_cpu < 0 && !compute_prev_delta)
> +			continue;
> +
> +		/* Compute the 'base' energy of the pd, without @p */
> +		base_energy_pd = compute_energy(p, -1, pd);
> +		base_energy += base_energy_pd;
> +

                /* Evaluate the energy impact of using prev_cpu. */

You agreed on this one during v1 review ;-)

> +		if (compute_prev_delta) {
> +			prev_delta = compute_energy(p, prev_cpu, pd);
> +			prev_delta -= base_energy_pd;
> +			best_delta = min(best_delta, prev_delta);
> +		}
> +
>  		/* Evaluate the energy impact of using this CPU. */

better:

            /* Evaluate the energy impact of using max_spare_cap_cpu. */

'this' has lost its context and people might read it as 'this_cpu' or
smp_processor_id().


> -		if (max_spare_cap_cpu >= 0 && max_spare_cap_cpu != prev_cpu) {
> +		if (max_spare_cap_cpu >= 0) {
>  			cur_delta = compute_energy(p, max_spare_cap_cpu, pd);
>  			cur_delta -= base_energy_pd;
>  			if (cur_delta < best_delta) {
> 

With these minor things sorted:

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