linux-kernel - Re: [PATCH] sched/core: Do not migrate ineligible tasks in sched_balance

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]

Message-ID: <20241128091929.GA35539@noisy.programming.kicks-ass.net>
Date: Thu, 28 Nov 2024 10:19:29 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Hao Jia <jiahao.kernel@...il.com>
Cc: mingo@...hat.com, mingo@...nel.org, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, dietmar.eggemann@....com,
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
	bristot@...hat.com, vschneid@...hat.com,
	linux-kernel@...r.kernel.org, Hao Jia <jiahao1@...iang.com>
Subject: Re: [PATCH] sched/core: Do not migrate ineligible tasks in
 sched_balance_rq()

On Thu, Nov 28, 2024 at 04:48:58PM +0800, Hao Jia wrote:
> From: Hao Jia <jiahao1@...iang.com>
> 
> When the PLACE_LAG scheduling feature is enabled, if a task
> is ineligible (lag < 0) on the source cpu runqueue, it will
> also be ineligible when it is migrated to the destination
> cpu runqueue.
> 
> Because we will keep the original equivalent lag of
> the task in place_entity(). So if the task was ineligible
> before, it will still be ineligible after migration.

This is not accurate, it will be eleigible, irrespective of lag, if
there are no other tasks. I think your patch tries to do this, but I'm
fairly sure it got it wrong.

> Therefore, we should skip the migration of ineligible tasks
> to reduce ineffective task migrations, just like the task
> throttled by cfs_bandwidth, until they become eligible.

And this misses an important case too -- load-balancing will try very
hard to balance load. If you disallow migrating tasks it might fail to
reach this goal. Since this is not a hard contraint it should eventually
give in and migrate it anyway.

That is, I would suggest allowing it when nr_balance_failed is non-zero.

> Signed-off-by: Hao Jia <jiahao1@...iang.com>
> ---
>  kernel/sched/fair.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index fbdca89c677f..5564e16b6fdb 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9358,13 +9358,14 @@ static inline int migrate_degrades_locality(struct task_struct *p,
>  static
>  int can_migrate_task(struct task_struct *p, struct lb_env *env)
>  {
> +	struct cfs_rq *cfs_rq = task_cfs_rq(p);
>  	int tsk_cache_hot;
>  
>  	lockdep_assert_rq_held(env->src_rq);
>  
>  	/*
>  	 * We do not migrate tasks that are:
> -	 * 1) throttled_lb_pair, or
> +	 * 1) throttled_lb_pair, or task ineligible, or
>  	 * 2) cannot be migrated to this CPU due to cpus_ptr, or
>  	 * 3) running (obviously), or
>  	 * 4) are cache-hot on their current CPU.
> @@ -9372,6 +9373,10 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env)
>  	if (throttled_lb_pair(task_group(p), env->src_cpu, env->dst_cpu))
>  		return 0;
>  
> +	if (sched_feat(PLACE_LAG) && cfs_rq->nr_running &&
> +			!entity_eligible(cfs_rq, &p->se))

Your indenting it wrong, please use: cino=(0:0

> +		return 0;
> +
>  	/* Disregard percpu kthreads; they are where they need to be. */
>  	if (kthread_is_per_cpu(p))
>  		return 0;
> -- 
> 2.34.1
>