Messages in this thread | | | From | Vincent Guittot <> | Date | Wed, 6 Oct 2021 10:16:53 +0200 | Subject | Re: [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);
| |