lkml.org 
[lkml]   [2021]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/2] sched/fair: account update_blocked_averages in newidle_balance cost
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);

\
 
 \ /
  Last update: 2021-10-06 10:17    [W:0.041 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site