Re: [PATCH 1/2] sched/fair: account update_blocked_averages in newidle_balance cost

[Vincent Guittot <vincent.guittot@linaro.org>]

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);