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

[Hao Jia]

On 2024/11/28 17:19, Peter Zijlstra wrote:
> On Thu, Nov 28, 2024 at 04:48:58PM +0800, Hao Jia wrote:
> > From: Hao Jia <jiahao1@xxxxxxxxxxx>
> >
> > 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.


Thank you for your reply. The expression in my commit message is 
inaccurate, and I will correct it in the patch v2. If I understand 
correctly, a task meeting the following conditions:

 sched_feat(PLACE_LAG) && cfs_rq->nr_running && 
!entity_eligible(cfs_rq, &p->se),

will remain ineligible both before and after migration.

If I am wrong, please correct me. Thank you!



> > 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.

Thank you for your suggestion, I will do it in the patch v2.

Thanks,
Hao

> > Signed-off-by: Hao Jia <jiahao1@xxxxxxxxxxx>
> > ---
> >   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

I will do it in the patch v2.

Thanks,
Hao

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