Messages in this thread | | | Date | Mon, 17 Apr 2023 22:35:32 +0800 | From | Aaron Lu <> | Subject | Re: [RFC PATCH v7] sched: Fix performance regression introduced by mm_cid |
| |
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
| |