linux-kernel - Re: [PATCH 1/2] sched/fair: account update_blocked_averages in newidle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKfTPtB1mS5OsFs+46jzWt-KSgkYGHrTyn1u2qt_k1qrf=4RCw@mail.gmail.com>
Date:   Wed, 6 Oct 2021 09:52:11 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>
Subject: Re: [PATCH 1/2] sched/fair: account update_blocked_averages in
 newidle_balance cost

On Tue, 5 Oct 2021 at 22:41, Peter Zijlstra <peterz@...radead.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@...aro.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;
> -       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);