Re: [RFC PATCH v7] sched: Fix performance regression introduced by mm_cid

[Mathieu Desnoyers]

On 2023-04-17 06:18, Aaron Lu wrote:
> On Sun, Apr 16, 2023 at 06:32:17PM -0400, Mathieu Desnoyers wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 0d18c3969f90..9532cf1a2a44 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -2084,8 +2084,10 @@ static inline void dequeue_task(struct rq *rq, struct task_struct *p, int flags)
> >   
> >   void activate_task(struct rq *rq, struct task_struct *p, int flags)
> >   {
> > -	if (task_on_rq_migrating(p))
> > +	if (task_on_rq_migrating(p)) {
> >   		flags |= ENQUEUE_MIGRATED;
> > +		sched_mm_cid_migrate_to(rq, p);
>
>
> I noticed you did this in previous version too but forgot to ask:
> is it your intention to only invoke sched_mm_cid_migrate_to() for queued
> tasks, i.e. tasks that are being migrated due to load/idle balance etc,
> but not those tasks that migrated on wakeup?

My intent is to also cover tasks migrated on wakeup.

> I saw you did the corresonding sched_mm_cid_migrate_from() in
> set_task_cpu(), which on the other hand includes tasks that migrated due
> to wakeup, so it kind of feel weird.

I'm probably missing something here. AFAIU, when try_to_wake_up() moves 
the target process to a different cpu:

        cpu = select_task_rq(p, p->wake_cpu, wake_flags | WF_TTWU);
        if (task_cpu(p) != cpu) {

it ends up calling ttwu_queue() with wake_flags |= WF_MIGRATED bit set.

Then ttwu_queue() ends up calling ttwu_queue_wakelist(), which may 
initiate an IPI to the target cpu which executes sched_ttwu_pending().
This function will take the target cpu's runqueue lock and call 
ttwu_do_activate() with wake_flags=WF_MIGRATED.

The other path that ttwu_queue() can take is to issue ttwu_do_activate() 
with the target cpu's rq lock held.

ttwu_do_activate() calls activate_task() with flags having 
ENQUEUE_MIGRATED set.

OK I think I see what I missed here, I should change this to:

void activate_task(struct rq *rq, struct task_struct *p, int flags)
{
        if (task_on_rq_migrating(p))
                flags |= ENQUEUE_MIGRATED;
	if (flags & ENQUEUE_MIGRATED)
                sched_mm_cid_migrate_to(rq, p);
[...]

Because flags is received as input parameter as well.

Do I get your meaning correctly ?

Thanks!

Mathieu

> > +	}
> >   
> >   	enqueue_task(rq, p, flags);
> >   
> > @@ -3195,6 +3197,7 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> >   			p->sched_class->migrate_task_rq(p, new_cpu);
> >   		p->se.nr_migrations++;
> >   		rseq_migrate(p);
> > +		sched_mm_cid_migrate_from(p);
> >   		perf_event_task_migrate(p);
> >   	}
>
> Thanks,
> Aaron

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com