| 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
| ||
|
Date: Mon, 12 Feb 2024 18:27:09 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Qais Yousef <qyousef@...alina.io>
Cc: Ingo Molnar <mingo@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
Dietmar Eggemann <dietmar.eggemann@....com>, linux-kernel@...r.kernel.org,
Pierre Gondois <Pierre.Gondois@....com>
Subject: Re: [PATCH v5 2/2] sched/fair: Check a task has a fitting cpu when
updating misfit
On Mon, 5 Feb 2024 at 03:11, Qais Yousef <qyousef@...alina.io> wrote:
>
> If a misfit task is affined to a subset of the possible cpus, we need to
> verify that one of these cpus can fit it. Otherwise the load balancer
> code will continuously trigger needlessly leading the balance_interval
> to increase in return and eventually end up with a situation where real
> imbalances take a long time to address because of this impossible
> imbalance situation.
>
> This can happen in Android world where it's common for background tasks
> to be restricted to little cores.
>
> Similarly if we can't fit the biggest core, triggering misfit is
> pointless as it is the best we can ever get on this system.
>
> To be able to detect that; we use asym_cap_list to iterate through
> capacities in the system to see if the task is able to run at a higher
> capacity level based on its p->cpus_ptr. We do that when the affinity
> change, a fair task is forked, or when a task switched to fair policy.
> We store the max_allowed_capacity in task_struct to allow for cheap
> comparison in the fast path.
>
> Improve check_misfit_status() function to be more readable. At one
> iteration of the patch it was thought we can drop one of the checks. The
> current form hopefully should make it more obvious what is being checked
> and why.
>
> Test:
> =====
>
> Add
>
> trace_printk("balance_interval = %lu\n", interval)
>
> in get_sd_balance_interval().
>
> run
> if [ "$MASK" != "0" ]; then
> adb shell "taskset -a $MASK cat /dev/zero > /dev/null"
> fi
> sleep 10
> // parse ftrace buffer counting the occurrence of each valaue
>
> Where MASK is either:
>
> * 0: no busy task running
> * 1: busy task is pinned to 1 cpu; handled today to not cause
> misfit
> * f: busy task pinned to little cores, simulates busy background
> task, demonstrates the problem to be fixed
>
> Results:
> ========
>
> Note how occurrence of balance_interval = 128 overshoots for MASK = f.
>
> BEFORE
> ------
>
> MASK=0
>
> 1 balance_interval = 175
> 120 balance_interval = 128
> 846 balance_interval = 64
> 55 balance_interval = 63
> 215 balance_interval = 32
> 2 balance_interval = 31
> 2 balance_interval = 16
> 4 balance_interval = 8
> 1870 balance_interval = 4
> 65 balance_interval = 2
>
> MASK=1
>
> 27 balance_interval = 175
> 37 balance_interval = 127
> 840 balance_interval = 64
> 167 balance_interval = 63
> 449 balance_interval = 32
> 84 balance_interval = 31
> 304 balance_interval = 16
> 1156 balance_interval = 8
> 2781 balance_interval = 4
> 428 balance_interval = 2
>
> MASK=f
>
> 1 balance_interval = 175
> 1328 balance_interval = 128
> 44 balance_interval = 64
> 101 balance_interval = 63
> 25 balance_interval = 32
> 5 balance_interval = 31
> 23 balance_interval = 16
> 23 balance_interval = 8
> 4306 balance_interval = 4
> 177 balance_interval = 2
>
> AFTER
> -----
>
> Note how the high values almost disappear for all MASK values. The
> system has background tasks that could trigger the problem without
> simulate it even with MASK=0.
>
> MASK=0
>
> 103 balance_interval = 63
> 19 balance_interval = 31
> 194 balance_interval = 8
> 4827 balance_interval = 4
> 179 balance_interval = 2
>
> MASK=1
>
> 131 balance_interval = 63
> 1 balance_interval = 31
> 87 balance_interval = 8
> 3600 balance_interval = 4
> 7 balance_interval = 2
>
> MASK=f
>
> 8 balance_interval = 127
> 182 balance_interval = 63
> 3 balance_interval = 31
> 9 balance_interval = 16
> 415 balance_interval = 8
> 3415 balance_interval = 4
> 21 balance_interval = 2
>
> Signed-off-by: Qais Yousef <qyousef@...alina.io>
> ---
> include/linux/sched.h | 1 +
> init/init_task.c | 1 +
> kernel/sched/fair.c | 84 +++++++++++++++++++++++++++++++++----------
> 3 files changed, 68 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 03bfe9ab2951..1e7bf52de607 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -827,6 +827,7 @@ struct task_struct {
> #endif
>
> unsigned int policy;
> + unsigned long max_allowed_capacity;
> int nr_cpus_allowed;
> const cpumask_t *cpus_ptr;
> cpumask_t *user_cpus_ptr;
> diff --git a/init/init_task.c b/init/init_task.c
> index 5727d42149c3..01b3199d4cde 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -82,6 +82,7 @@ struct task_struct init_task
> .cpus_ptr = &init_task.cpus_mask,
> .user_cpus_ptr = NULL,
> .cpus_mask = CPU_MASK_ALL,
> + .max_allowed_capacity = SCHED_CAPACITY_SCALE,
> .nr_cpus_allowed= NR_CPUS,
> .mm = NULL,
> .active_mm = &init_mm,
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b803030c3a03..8b8035f5c8f6 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5092,24 +5092,36 @@ static inline int task_fits_cpu(struct task_struct *p, int cpu)
>
> static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> {
> + unsigned long cpu_cap;
> + int cpu = cpu_of(rq);
> +
> if (!sched_asym_cpucap_active())
> return;
>
> - if (!p || p->nr_cpus_allowed == 1) {
> - rq->misfit_task_load = 0;
> - return;
> - }
> + if (!p || p->nr_cpus_allowed == 1)
> + goto out;
>
> - if (task_fits_cpu(p, cpu_of(rq))) {
> - rq->misfit_task_load = 0;
> - return;
> - }
> + cpu_cap = arch_scale_cpu_capacity(cpu);
> +
> + /* If we can't fit the biggest CPU, that's the best we can ever get. */
> + if (cpu_cap == rq->rd->max_cpu_capacity)
Isn't the condition above also covered by the condition below and
becomes now useless ?
> + goto out;
> +
> + /* Affinity allows us to go somewhere higher? */
> + if (cpu_cap == p->max_allowed_capacity)
> + goto out;
> +
> + if (task_fits_cpu(p, cpu))
> + goto out;
>
> /*
> * Make sure that misfit_task_load will not be null even if
> * task_h_load() returns 0.
> */
> rq->misfit_task_load = max_t(unsigned long, task_h_load(p), 1);
> + return;
> +out:
> + rq->misfit_task_load = 0;
> }
>
> #else /* CONFIG_SMP */
> @@ -8241,6 +8253,36 @@ static void task_dead_fair(struct task_struct *p)
> remove_entity_load_avg(&p->se);
> }
>
> +/*
> + * Check the max capacity the task is allowed to run at for misfit detection.
> + */
> +static void set_task_max_allowed_capacity(struct task_struct *p)
> +{
> + struct asym_cap_data *entry;
> +
> + if (!sched_asym_cpucap_active())
> + return;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &asym_cap_list, link) {
> + cpumask_t *cpumask;
> +
> + cpumask = cpu_capacity_span(entry);
> + if (!cpumask_intersects(p->cpus_ptr, cpumask))
> + continue;
> +
> + p->max_allowed_capacity = entry->capacity;
> + break;
> + }
> + rcu_read_unlock();
> +}
> +
> +static void set_cpus_allowed_fair(struct task_struct *p, struct affinity_context *ctx)
> +{
> + set_cpus_allowed_common(p, ctx);
> + set_task_max_allowed_capacity(p);
> +}
> +
> static int
> balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> {
> @@ -9601,16 +9643,18 @@ check_cpu_capacity(struct rq *rq, struct sched_domain *sd)
> (arch_scale_cpu_capacity(cpu_of(rq)) * 100));
> }
>
> -/*
> - * Check whether a rq has a misfit task and if it looks like we can actually
> - * help that task: we can migrate the task to a CPU of higher capacity, or
> - * the task's current CPU is heavily pressured.
> - */
> -static inline int check_misfit_status(struct rq *rq, struct sched_domain *sd)
> +/* Check if the rq has a misfit task */
> +static inline bool check_misfit_status(struct rq *rq, struct sched_domain *sd)
> {
> - return rq->misfit_task_load &&
> - (arch_scale_cpu_capacity(rq->cpu) < rq->rd->max_cpu_capacity ||
> - check_cpu_capacity(rq, sd));
> + if (!rq->misfit_task_load)
> + return false;
I think that only the above is enough ...
> +
> + /* Can we migrate to a CPU with higher capacity? */
> + if (arch_scale_cpu_capacity(rq->cpu) < rq->rd->max_cpu_capacity)
because rq->misfit_task_load is set to 0 if
arch_scale_cpu_capacity(rq->cpu) == rq->rd->max_cpu_capacity
That would also mean that we don't need to keep and set
rd->max_cpu_capacity anymore as we remove the 2 uses of it
> + return true;
> +
> + /* Is the task's CPU being heavily pressured? */
> + return check_cpu_capacity(rq, sd);
and this one has already been tested in nohz_balancer_kick() before
calling check_misfit_status()
> }
>
> /*
> @@ -12647,6 +12691,8 @@ static void task_fork_fair(struct task_struct *p)
> rq_lock(rq, &rf);
> update_rq_clock(rq);
>
> + set_task_max_allowed_capacity(p);
> +
> cfs_rq = task_cfs_rq(current);
> curr = cfs_rq->curr;
> if (curr)
> @@ -12770,6 +12816,8 @@ static void switched_to_fair(struct rq *rq, struct task_struct *p)
> {
> attach_task_cfs_rq(p);
>
> + set_task_max_allowed_capacity(p);
> +
> if (task_on_rq_queued(p)) {
> /*
> * We were most likely switched from sched_rt, so
> @@ -13154,7 +13202,7 @@ DEFINE_SCHED_CLASS(fair) = {
> .rq_offline = rq_offline_fair,
>
> .task_dead = task_dead_fair,
> - .set_cpus_allowed = set_cpus_allowed_common,
> + .set_cpus_allowed = set_cpus_allowed_fair,
> #endif
>
> .task_tick = task_tick_fair,
> --
> 2.34.1
>
Powered by blists - more mailing lists