On 20-Mar 00:37, Leo Yan wrote:
On Mon, Mar 19, 2018 at 12:33:01PM +0000, Patrick Bellasi wrote:
On 19-Mar 07:07, Daniel Lezcano wrote:
On Sat, Mar 17, 2018 at 08:05:15PM +0800, Leo Yan wrote:
select_energy_cpu_idx() returns the target CPU index, and eenv->next_idx is used to define target CPU index as well; it's pointless to use two methods to return exactly same value.
This patch is to change the definition for return value of select_energy_cpu_idx(), errors return negative value and success returns 0. And later use eenv->next_idx for target CPU index.
Yes there is a duplication of the information. Actually, the change could be the contrary: make select_energy_cpu_idx return the cpu_idx and kill the next_idx field in the structure.
next_idx field is used only in the calling function, it could be stored in a local variable.
Unfortunately it's not merged (yet) in eas-dev, but there is an energy_diff tracepoint which gets eenv only data.
That's the reason why we store next_idx in the eenv, moreover it makes the code more self-contained from a functional standpoint since everywhere you know that the eenv contains all the information which defined a certain energy aware wakeup decision.
Using next_idx as a return value is also another design decision, since it makes the core more streamline to read.
Thus, overall I don't see a big point on this patch if you consider the above two "design decisions".
I personlly prefer to remove redundant variable from data structure as we can, this finally can help us to keep code as simple as possible. So I am bias to Daniel suggestion.
And after quick thinking, after removing 'next_idx' field it should not impact ftrace event recording, right?
Depends on what you trace with the tracepoint. The idea was to have all the energy diff related info tracked by the eenv struct, so that we can know exactly how we got to a certain results.
Here, next_idx represents actually which CPU has been computed to be the most energy efficient candidate, which can be a valuable info to track in a tracepoint.
Thanks, Leo Yan
Change-Id: I57bc9c96566eaf6085c85ea9386d11422cafddbf Signed-off-by: Leo Yan leo.yan@linaro.org
kernel/sched/fair.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 37231b8..0f0b307 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -5850,16 +5850,15 @@ static inline bool cpu_in_sg(struct sched_group *sg, int cpu)
- select_energy_cpu_idx(): estimate the energy impact of changing the
- utilization distribution.
- The eenv parameter specifies the changes: utilisation amount and a pair of
- possible CPU candidates (the previous CPU and a different target CPU).
- This function returns the index of a CPU candidate specified by the
- eenv::next_idx returns the index of a CPU candidate specified by the
- energy_env which corresponds to the first CPU saving energy.
- Thus, 0 (EAS_CPU_PRV) means that non of the CPU candidate is more energy
- efficient than running on prev_cpu. This is also the value returned in case
- of abort due to error conditions during the computations.
- A value greater than zero means that the first energy-efficient CPU is the
- one represented by eenv->cpu[eenv->next_idx].cpu_id.
- efficient than running on prev_cpu. A value greater than zero means that
- the first energy-efficient CPU is the one represented by
- eenv->cpu[eenv->next_idx].cpu_id.
- The function returns negative value in case of abort due to error
*/
- conditions during the computations, success returns 0.
static inline int select_energy_cpu_idx(struct energy_env *eenv) { @@ -5871,8 +5870,10 @@ static inline int select_energy_cpu_idx(struct energy_env *eenv)
sd_cpu = eenv->cpu[EAS_CPU_PRV].cpu_id; sd = rcu_dereference(per_cpu(sd_ea, sd_cpu));
- if (!sd)
return EAS_CPU_PRV;
if (!sd) {
eenv->next_idx = EAS_CPU_PRV;
return -1;
}
sg = sd->groups; do {
@@ -5882,8 +5883,10 @@ static inline int select_energy_cpu_idx(struct energy_env *eenv)
eenv->sg_top = sg; /* energy is unscaled to reduce rounding errors */
if (compute_energy(eenv) == -EINVAL)
return EAS_CPU_PRV;
if (compute_energy(eenv) == -EINVAL) {
eenv->next_idx = EAS_CPU_PRV;
return -EINVAL;
}
} while (sg = sg->next, sg != sd->groups);
@@ -5929,7 +5932,7 @@ static inline int select_energy_cpu_idx(struct energy_env *eenv) } }
- return eenv->next_idx;
- return 0;
}
/* @@ -7145,17 +7148,18 @@ static int select_energy_cpu_brute(struct task_struct *p, int prev_cpu, int sync if (backup_cpu >= 0) cpumask_set_cpu(backup_cpu, &eenv.cpus_mask);
select_energy_cpu_idx(&eenv);
- /* Check if EAS_CPU_NXT is a more energy efficient CPU */
if (select_energy_cpu_idx(&eenv) != EAS_CPU_PRV) {
if (eenv.next_idx != EAS_CPU_PRV) { schedstat_inc(p->se.statistics.nr_wakeups_secb_nrg_sav); schedstat_inc(this_rq()->eas_stats.secb_nrg_sav);
target_cpu = eenv.cpu[eenv.next_idx].cpu_id;
goto unlock;
} else {
schedstat_inc(p->se.statistics.nr_wakeups_secb_no_nrg_sav);
}schedstat_inc(this_rq()->eas_stats.secb_no_nrg_sav);
schedstat_inc(p->se.statistics.nr_wakeups_secb_no_nrg_sav);
schedstat_inc(this_rq()->eas_stats.secb_no_nrg_sav);
target_cpu = prev_cpu;
goto unlock; }target_cpu = eenv.cpu[eenv.next_idx].cpu_id;
-- 1.9.1
--
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook | http://twitter.com/#!/linaroorg Twitter | http://www.linaro.org/linaro-blog/ Blog
-- #include <best/regards.h>
Patrick Bellasi
-- #include <best/regards.h>
Patrick Bellasi