linux-kernel - Re: [PATCH v2 1/2] sched/fair: Only compute base_energy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 May 2021 14:52:13 +0200
From:   Dietmar Eggemann <dietmar.eggemann@....com>
To:     Pierre.Gondois@....com, linux-kernel@...r.kernel.org,
        xuewen.yan@...soc.com, qperret@...rret.net
Cc:     Lukasz.Luba@....com, Vincent.Donnefort@....com, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, rostedt@...dmis.org,
        bsegall@...gle.com, mgorman@...e.de, bristot@...hat.com
Subject: Re: [PATCH v2 1/2] sched/fair: Only compute base_energy_pd if
 necessary

On 29/04/2021 12:19, Pierre.Gondois@....com wrote:
> From: Pierre Gondois <Pierre.Gondois@....com>
> 
> 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@....com>