| 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: Fri, 23 Feb 2024 10:30:31 +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 v6 2/4] sched/fair: Check a task has a fitting cpu when
updating misfit
Le mardi 20 févr. 2024 à 22:56:20 (+0000), Qais Yousef a écrit :
> 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 by removing redundant checks.
> misfit_task_load will be 0 if the task can't move to a bigger CPU. And
> nohz_load_balance() already checks for cpu_check_capacity() before
> calling check_misfit_status().
>
> 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>
I have a comment below but anyway
Reviewed-by: Vincent Guittot <vincent.guittot@...aro.org>
> ---
> include/linux/sched.h | 1 +
> init/init_task.c | 1 +
> kernel/sched/fair.c | 77 +++++++++++++++++++++++++++++++++----------
> 3 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index ffe8f618ab86..774cddbeab09 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -835,6 +835,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 7ecb458eb3da..b3dbab4c959e 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -77,6 +77,7 @@ struct task_struct init_task __aligned(L1_CACHE_BYTES) = {
> .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 8e30e2bb77a0..20006fcf7df2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5092,24 +5092,35 @@ 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);
> +
> + /*
> + * Affinity allows us to go somewhere higher? Or are we on biggest
> + * available CPU already?
> + */
> + if (cpu_cap == p->max_allowed_capacity)
> + goto out;
> +
> + if (task_fits_cpu(p, cpu))
> + goto out;
I think the below is easier to read:
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 209f5da17d79..0929e6704b44 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5095,35 +5095,25 @@ 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)
- goto out;
-
- cpu_cap = arch_scale_cpu_capacity(cpu);
-
/*
* Affinity allows us to go somewhere higher? Or are we on biggest
- * available CPU already?
+ * available CPU already? Or do we fit into this CPU ?
*/
- 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;
+ if (!p || (p->nr_cpus_allowed == 1) ||
+ (arch_scale_cpu_capacity(cpu) == p->max_allowed_capacity) ||
+ task_fits_cpu(p, cpu))
+ rq->misfit_task_load = 0;
+ else
+ /*
+ * 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);
}
#else /* CONFIG_SMP */
>
> /*
> * 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 +8252,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)
> {
> @@ -8249,6 +8290,8 @@ balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>
> return newidle_balance(rq, rf) != 0;
> }
> +#else
> +static inline void set_task_max_allowed_capacity(struct task_struct *p) {}
> #endif /* CONFIG_SMP */
>
> static void set_next_buddy(struct sched_entity *se)
> @@ -9601,16 +9644,10 @@ 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));
> + return rq->misfit_task_load;
> }
>
> /*
> @@ -12645,6 +12682,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)
> @@ -12768,6 +12807,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
> @@ -13139,7 +13180,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