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

[Aaron Lu]

On Mon, Apr 17, 2023 at 09:28:54AM -0400, Mathieu Desnoyers wrote:
> 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 ?

Yes, that's what I meant.

Tasks that have on_rq equals to TASK_ON_RQ_MIGRATING are only those
migrated by load balance etc., it doesn't apply to tasks that migrated
on wake up.

I think your new change looks correct.

Thanks,
Aaron