Messages in this thread | | | From | Hao Luo <> | Date | Thu, 14 Oct 2021 10:57:46 -0700 | Subject | Re: [PATCH] sched/core: forced idle accounting |
| |
On Mon, Oct 11, 2021 at 5:31 PM Josh Don <joshdon@google.com> wrote: > > On Mon, Oct 11, 2021 at 10:33 AM Hao Luo <haoluo@google.com> wrote: > > > > On Thu, Oct 7, 2021 at 5:08 PM Josh Don <joshdon@google.com> wrote: > > > -void sched_core_dequeue(struct rq *rq, struct task_struct *p) > > > +void sched_core_dequeue(struct rq *rq, struct task_struct *p, int flags) > > > { > > > rq->core->core_task_seq++; > > > > > > - if (!sched_core_enqueued(p)) > > > - return; > > > + if (sched_core_enqueued(p)) { > > > + rb_erase(&p->core_node, &rq->core_tree); > > > + RB_CLEAR_NODE(&p->core_node); > > > + } > > > > > > - rb_erase(&p->core_node, &rq->core_tree); > > > - RB_CLEAR_NODE(&p->core_node); > > > + /* > > > + * Migrating the last task off the cpu, with the cpu in forced idle > > > + * state. Reschedule to create an accounting edge for forced idle, > > > + * and re-examine whether the core is still in forced idle state. > > > + */ > > > + if (!(flags & DEQUEUE_SAVE) && rq->nr_running == 1 && > > > + rq->core->core_forceidle && rq->curr == rq->idle) > > > + resched_curr(rq); > > > > Resched_curr is probably an unwanted side effect of dequeue. Maybe we > > could extract the check and resched_curr out into a function, and call > > the function outside of sched_core_dequeue(). In that way, the > > interface of dequeue doesn't need to change. > > This resched is an atypical case; normal load balancing won't steal > the last runnable task off a cpu. The main reasons this resched could > trigger are: migration due to affinity change, and migration due to > sched core doing a cookie_steal. Could bubble this up to > deactivate_task(), but seems less brittle to keep this in dequeue() > with the check against DEQUEUE_SAVE (since this creates an important > accounting edge). Thoughts? >
I prefer bubbling it up to deactivate_task(). Depending on how many callers of deactivate_task() need this resched, IMHO it is even fine to put it in deactivate_task's caller. Wrapping it in a function may help clarify its purpose.
> > > /* > > > @@ -5765,7 +5782,7 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf) > > > for_each_cpu_wrap(i, smt_mask, cpu) { > > > rq_i = cpu_rq(i); > > > > > > - if (i != cpu) > > > + if (i != cpu && (rq_i != rq->core || !core_clock_updated)) > > > update_rq_clock(rq_i); > > > > Do you mean (rq_i != rq->core && !core_clock_updated)? I thought > > rq->core has core_clock updated always. > > rq->clock is updated on entry to pick_next_task(). rq->core is only > updated if rq == rq->core, or if we've done the clock update for > rq->core above.
I meant 'if (i != cpu && rq_i != rq->core)'. Because at this point, core_clock should already have been updated, is that not the case? Anyway, the tracking of clock updates here is too confusing to me.
| |