| 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
| ||
|
Message-ID: <5c6259a2-fadd-41c6-aa41-91f3af01272c@linux.ibm.com>
Date: Mon, 4 Mar 2024 13:54:06 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Qais Yousef <qyousef@...alina.io>, dietmar.eggemann@....com,
pierre.gondois@....com
Cc: mingo@...nel.org, peterz@...radead.org, vincent.guittot@...aro.org,
yu.c.chen@...el.com, linux-kernel@...r.kernel.org, nysal@...ux.ibm.com,
aboorvad@...ux.ibm.com, srikar@...ux.ibm.com, vschneid@...hat.com,
morten.rasmussen@....com
Subject: Re: [PATCH v4 1/2] sched/fair: Add EAS checks before updating
overutilized
On 3/4/24 12:20 AM, Qais Yousef wrote:
> On 03/01/24 20:47, Shrikanth Hegde wrote:
>> Overutilized field of root domain is only used for EAS(energy aware scheduler)
[...]
Hi Qais, Thanks for taking a look.
>> ---
>> kernel/sched/fair.c | 49 +++++++++++++++++++++++++++------------------
>> 1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 6a16129f9a5c..a71f8a1506e4 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6670,15 +6670,29 @@ static inline bool cpu_overutilized(int cpu)
>> return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>> }
>>
>> -static inline void update_overutilized_status(struct rq *rq)
>> +static inline void set_rd_overutilized_status(struct root_domain *rd,
>> + unsigned int status)
>> {
>> - if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu)) {
>> - WRITE_ONCE(rq->rd->overutilized, SG_OVERUTILIZED);
>> - trace_sched_overutilized_tp(rq->rd, SG_OVERUTILIZED);
>> - }
>
> Can we add
>
> if (!sched_energy_enabled())
> return;
This is very close to what i had till v2. But it was pointed out that, it
would end up calling sched_energy_enabled twice in check_update_overutilized_status.
In check_update_overutilized_status, it would be better to avoid access to
overutilized and computing cpu_overutilized if EAS is not enabled.
I am okay with either code. keeping sched_energy_enabled in set_rd_overutilized_status
would be less code and more readable. But would call sched_energy_enabled twice.
Dietmar, Pierre,
Could you please provide your inputs here?
>
> here and avoid sprinkling the condition in other various places instead?
>
>> + WRITE_ONCE(rd->overutilized, status);
>> + trace_sched_overutilized_tp(rd, !!status);
>> +}
>> +
>> +static inline void check_update_overutilized_status(struct rq *rq)
>> +{
>> + /*
>> + * overutilized field is used for load balancing decisions only
>> + * if energy aware scheduler is being used
>> + */
>
> nit: I think this comment is unnecessary but I don't mind keeping it
>
>> + if (!sched_energy_enabled())
>> + return;
>> +
>> + if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu))
>> + set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
>> }
>> #else
>> -static inline void update_overutilized_status(struct rq *rq) { }
>> +static inline void check_update_overutilized_status(struct rq *rq) { }
>> +static inline void set_rd_overutilized_status(struct root_domain *rd,
>> + unsigned int status) { }
>> #endif
>>
>> /* Runqueue only has SCHED_IDLE tasks enqueued */
>> @@ -6779,7 +6793,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>> * and the following generally works well enough in practice.
>> */
>> if (!task_new)
>> - update_overutilized_status(rq);
>> + check_update_overutilized_status(rq);
>>
>> enqueue_throttle:
>> assert_list_leaf_cfs_rq(rq);
>> @@ -9902,7 +9916,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>> if (nr_running > 1)
>> *sg_status |= SG_OVERLOAD;
>>
>> - if (cpu_overutilized(i))
>> + if (sched_energy_enabled() && cpu_overutilized(i))
>
> I think we can drop sched_energy_enable() here if we add it to
> set_rd_overutilized_status()
we can avoid additional call to cpu_overutilized. So we should keep it.
>
>> *sg_status |= SG_OVERUTILIZED;
>>
>> #ifdef CONFIG_NUMA_BALANCING
>> @@ -10596,19 +10610,16 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>> env->fbq_type = fbq_classify_group(&sds->busiest_stat);
>>
>> if (!env->sd->parent) {
>> - struct root_domain *rd = env->dst_rq->rd;
>> -
>> /* update overload indicator if we are at root domain */
>> - WRITE_ONCE(rd->overload, sg_status & SG_OVERLOAD);
>> + WRITE_ONCE(env->dst_rq->rd->overload, sg_status & SG_OVERLOAD);
>>
>> /* Update over-utilization (tipping point, U >= 0) indicator */
>> - WRITE_ONCE(rd->overutilized, sg_status & SG_OVERUTILIZED);
>> - trace_sched_overutilized_tp(rd, sg_status & SG_OVERUTILIZED);
>> - } else if (sg_status & SG_OVERUTILIZED) {
>> - struct root_domain *rd = env->dst_rq->rd;
>> -
>> - WRITE_ONCE(rd->overutilized, SG_OVERUTILIZED);
>> - trace_sched_overutilized_tp(rd, SG_OVERUTILIZED);
>> + if (sched_energy_enabled()) {
>
> ditto
First comment would apply for these two.
>> + set_rd_overutilized_status(env->dst_rq->rd,
>> + sg_status & SG_OVERUTILIZED);
>> + }
>> + } else if (sched_energy_enabled() && (sg_status & SG_OVERUTILIZED)) {
>
> ditto
>
>> + set_rd_overutilized_status(env->dst_rq->rd, SG_OVERUTILIZED);
>> }
>>
>> update_idle_cpu_scan(env, sum_util);
>> @@ -12609,7 +12620,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
>> task_tick_numa(rq, curr);
>>
>> update_misfit_status(curr, rq);
>> - update_overutilized_status(task_rq(curr));
>> + check_update_overutilized_status(task_rq(curr));
>>
>> task_tick_core(rq, curr);
>> }
>> --
>> 2.39.3
>>
Powered by blists - more mailing lists