From: Vincent Guittot <vincent.guittot@linaro.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>,
Juri Lelli <juri.lelli@redhat.com>,
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>,
linux-kernel <linux-kernel@vger.kernel.org>,
Tim Chen <tim.c.chen@linux.intel.com>
Subject: Re: [PATCH 1/2] sched/fair: account update_blocked_averages in newidle_balance cost
Date: Wed, 6 Oct 2021 10:16:53 +0200 [thread overview]
Message-ID: <CAKfTPtA9DWZsG8o3hujD6cLo3m6ZTNraqkp7Za1RPYhsymH7vw@mail.gmail.com> (raw)
In-Reply-To: <CAKfTPtB1mS5OsFs+46jzWt-KSgkYGHrTyn1u2qt_k1qrf=4RCw@mail.gmail.com>
On Wed, 6 Oct 2021 at 09:52, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> On Tue, 5 Oct 2021 at 22:41, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Oct 04, 2021 at 07:14:50PM +0200, Vincent Guittot wrote:
> > > The time spent to update the blocked load can be significant depending of
> > > the complexity fo the cgroup hierarchy. Take this time into account when
> > > deciding to stop newidle_balance() because it exceeds the expected idle
> > > time.
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > > kernel/sched/fair.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 8943dbb94365..1f78b2e3b71c 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -10810,7 +10810,7 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> > > int this_cpu = this_rq->cpu;
> > > struct sched_domain *sd;
> > > int pulled_task = 0;
> > > - u64 curr_cost = 0;
> > > + u64 t0, domain_cost, curr_cost = 0;
> > >
> > > update_misfit_status(NULL, this_rq);
> > >
> > > @@ -10855,11 +10855,14 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf)
> > >
> > > raw_spin_rq_unlock(this_rq);
> > >
> > > + t0 = sched_clock_cpu(this_cpu);
> > > update_blocked_averages(this_cpu);
> > > + domain_cost = sched_clock_cpu(this_cpu) - t0;
> > > + curr_cost += domain_cost;
> > > +
> > > rcu_read_lock();
> > > for_each_domain(this_cpu, sd) {
> > > int continue_balancing = 1;
> > > - u64 t0, domain_cost;
> > >
> > > if (this_rq->avg_idle < curr_cost + sd->max_newidle_lb_cost) {
> > > update_next_balance(sd, &next_balance);
> >
> > Does this make sense? It avoids a bunch of clock calls (and thereby
> > accounts more actual time).
>
> Originally, I didn't want to modify the current accounting of
> sched_domain but only account the sometime large
> update_blocked_averages(). but i agree that we can ensure to account
> more actual time
> >
> > Also, perhaps we should some asymmetric IIR instead of a strict MAX
> > filter for max_newidle_lb_cost.
>
> Ok. I'm going to look at this and see how all this goes
>
> >
> > ---
> > Index: linux-2.6/kernel/sched/fair.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched/fair.c
> > +++ linux-2.6/kernel/sched/fair.c
> > @@ -10759,9 +10759,9 @@ static int newidle_balance(struct rq *th
> > {
> > unsigned long next_balance = jiffies + HZ;
> > int this_cpu = this_rq->cpu;
> > + u64 t0, t1, curr_cost = 0;
> > struct sched_domain *sd;
> > int pulled_task = 0;
> > - u64 t0, domain_cost, curr_cost = 0;
> >
> > update_misfit_status(NULL, this_rq);
> >
> > @@ -10808,8 +10808,9 @@ static int newidle_balance(struct rq *th
> >
> > t0 = sched_clock_cpu(this_cpu);
> > update_blocked_averages(this_cpu);
> > - domain_cost = sched_clock_cpu(this_cpu) - t0;
I wonder if we should not include the duration of
update_blocked_averages() in the 1st domain cost ?
To make sure that we will not update it but finally abort before
running the 1st domain because there is not enough remaining time
> > - curr_cost += domain_cost;
> > + t1 = sched_clock_cpu(this_cpu);
> > + curr_cost += t1 - t0;
> > + t0 = t1;
> >
> > rcu_read_lock();
> > for_each_domain(this_cpu, sd) {
> > @@ -10821,17 +10822,19 @@ static int newidle_balance(struct rq *th
> > }
> >
> > if (sd->flags & SD_BALANCE_NEWIDLE) {
> > - t0 = sched_clock_cpu(this_cpu);
> > + u64 domain_cost;
> >
> > pulled_task = load_balance(this_cpu, this_rq,
> > sd, CPU_NEWLY_IDLE,
> > &continue_balancing);
> >
> > - domain_cost = sched_clock_cpu(this_cpu) - t0;
> > + t1 = sched_clock_cpu(this_cpu);
> > + domain_cost = t1 - t0;
> > if (domain_cost > sd->max_newidle_lb_cost)
> > sd->max_newidle_lb_cost = domain_cost;
> >
> > curr_cost += domain_cost;
> > + t0 = t1;
> > }
> >
> > update_next_balance(sd, &next_balance);
next prev parent reply other threads:[~2021-10-06 8:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-04 17:14 [PATCH 0/2] sched/fair: Improve cost accounting of newidle_balance Vincent Guittot
2021-10-04 17:14 ` [PATCH 1/2] sched/fair: account update_blocked_averages in newidle_balance cost Vincent Guittot
2021-10-05 20:40 ` Peter Zijlstra
2021-10-06 7:52 ` Vincent Guittot
2021-10-06 8:16 ` Vincent Guittot [this message]
2021-10-04 17:14 ` [PATCH 2/2] sched/fair: Skip update_blocked_averages if we are defering load balance Vincent Guittot
2021-10-05 20:49 ` Peter Zijlstra
2021-10-06 8:12 ` Vincent Guittot
2021-10-04 23:06 ` [PATCH 0/2] sched/fair: Improve cost accounting of newidle_balance Tim Chen
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=CAKfTPtA9DWZsG8o3hujD6cLo3m6ZTNraqkp7Za1RPYhsymH7vw@mail.gmail.com \
--to=vincent.guittot@linaro.org \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.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=tim.c.chen@linux.intel.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).