From: Hao Luo <haoluo@google.com>
To: Josh Don <joshdon@google.com>
Cc: Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Juri Lelli <juri.lelli@redhat.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Joel Fernandes <joel@joelfernandes.org>,
Vineeth Pillai <vineethrp@gmail.com>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sched/core: forced idle accounting
Date: Thu, 14 Oct 2021 16:58:15 -0700 [thread overview]
Message-ID: <CA+khW7gKffoo3w4trftC2D1ewPc4YpK4G3S6sZb5JtOOQrRUKA@mail.gmail.com> (raw)
In-Reply-To: <CABk29NtBU4EiruRtvmjrw+AKTQZEpvrY3JSVH0HR_ef9Wj3BfA@mail.gmail.com>
On Thu, Oct 14, 2021 at 4:29 PM Josh Don <joshdon@google.com> wrote:
>
> On Thu, Oct 14, 2021 at 10:58 AM Hao Luo <haoluo@google.com> wrote:
> >
> > 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.
>
> I'd argue against bubbling up above deactivate_task(); makes things
> much more brittle if a new use for deactivate_task() is added in the
> future.
>
> Tried both ways; IMO it seems slightly better to leave in dequeue() vs
> deactivate(); less confusing to one hook instead of two for coresched
> to handle dequeuing a task.
>
Ack. No problem. I don't have strong objections here.
> > > > > /*
> > > > > @@ -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.
>
> Added a comment here, but the logic flow is:
> - rq->clock is always updated on entry to pick_next_task()
> - rq->core->clock _may_ be updated by the time we get to this part of
> pick_next_task(). We have to be careful to avoid a double update,
> hence the need for the core_clock_updated var.
Yeah. Sync'ed offline and that cleared my confusion. Thanks.
Hao
prev parent reply other threads:[~2021-10-14 23:58 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-08 0:08 [PATCH] sched/core: forced idle accounting Josh Don
2021-10-08 21:04 ` Josh Don
2021-10-09 15:54 ` Peter Zijlstra
2021-10-12 0:12 ` Josh Don
2021-10-12 12:27 ` Peter Zijlstra
2021-10-12 19:45 ` Josh Don
2021-10-14 14:24 ` Peter Zijlstra
2021-10-14 23:18 ` Josh Don
2021-10-09 18:11 ` Tao Zhou
2021-10-12 0:14 ` Josh Don
2021-10-11 17:33 ` Hao Luo
2021-10-12 0:31 ` Josh Don
2021-10-14 17:57 ` Hao Luo
2021-10-14 23:29 ` Josh Don
2021-10-14 23:58 ` Hao Luo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+khW7gKffoo3w4trftC2D1ewPc4YpK4G3S6sZb5JtOOQrRUKA@mail.gmail.com \
--to=haoluo@google.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=joel@joelfernandes.org \
--cc=joshdon@google.com \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=vincent.guittot@linaro.org \
--cc=vineethrp@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).