Re: [PATCH] sched/core: Do not migrate ineligible tasks in sched_balance_rq()

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Nov 28, 2024 at 04:48:58PM +0800, Hao Jia wrote:
> From: Hao Jia <jiahao1@lixiang.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@lixiang.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
>