linux-kernel - Re: [PATCH 2/2] sched: remove distribute

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: <20200413144915.GB2517@lorien.usersys.redhat.com>
Date:   Mon, 13 Apr 2020 10:49:15 -0400
From:   Phil Auld <pauld@...hat.com>
To:     Josh Don <joshdon@...gle.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Paul Turner <pjt@...gle.com>,
        Huaixin Chang <changhuaixin@...ux.alibaba.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] sched: remove distribute_running from CFS bandwidth

Hi Josh,

On Sat, Apr 11, 2020 at 07:01:45PM -0700 Josh Don wrote:
> On Fri, Apr 10, 2020 at 3:52 PM Josh Don <joshdon@...gle.com> wrote:
> >
> > This is mostly a revert of baa9be4ff.
> >
> > The primary use of distribute_running was to determine whether to add
> > throttled entities to the head or the tail of the throttled list. Now
> > that we always add to the tail, we can remove this field.
> >
> > The other use of distribute_running is in the slack_timer, so that we
> > don't start a distribution while one is already running. However, even
> > in the event that this race occurs, it is fine to have two distributions
> > running (especially now that distribute grabs the cfs_b->lock to
> > determine remaining quota before assigning).
> >
> > Signed-off-by: Josh Don <joshdon@...gle.com>

Nice. This was more or less a hack. It's good to be able to remove it.
I re-ran my test cases that originally caused the need for the
distribute_running.  As expected, since there is no adding to the head
of the queue, the behavior is the same as before this series.

Reviewed-by: Phil Auld <pauld@...hat.com>
Tested-by: Phil Auld <pauld@...hat.com>


Cheers,
Phil

> > ---
> >  kernel/sched/fair.c  | 13 +------------
> >  kernel/sched/sched.h |  1 -
> >  2 files changed, 1 insertion(+), 13 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 3fb986c52825..24b5e12efb40 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4930,14 +4930,12 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> >         /*
> >          * This check is repeated as we release cfs_b->lock while we unthrottle.
> >          */
> > -       while (throttled && cfs_b->runtime > 0 && !cfs_b->distribute_running) {
> > -               cfs_b->distribute_running = 1;
> > +       while (throttled && cfs_b->runtime > 0) {
> >                 raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> >                 /* we can't nest cfs_b->lock while distributing bandwidth */
> >                 distribute_cfs_runtime(cfs_b);
> >                 raw_spin_lock_irqsave(&cfs_b->lock, flags);
> >
> > -               cfs_b->distribute_running = 0;
> >                 throttled = !list_empty(&cfs_b->throttled_cfs_rq);
> >         }
> >
> > @@ -5051,10 +5049,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> >         /* confirm we're still not at a refresh boundary */
> >         raw_spin_lock_irqsave(&cfs_b->lock, flags);
> >         cfs_b->slack_started = false;
> > -       if (cfs_b->distribute_running) {
> > -               raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > -               return;
> > -       }
> >
> >         if (runtime_refresh_within(cfs_b, min_bandwidth_expiration)) {
> >                 raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > @@ -5064,9 +5058,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> >         if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
> >                 runtime = cfs_b->runtime;
> >
> > -       if (runtime)
> > -               cfs_b->distribute_running = 1;
> > -
> >         raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> >
> >         if (!runtime)
> > @@ -5075,7 +5066,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> >         distribute_cfs_runtime(cfs_b);
> >
> >         raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > -       cfs_b->distribute_running = 0;
> >         raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> >  }
> >
> > @@ -5217,7 +5207,6 @@ void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> >         cfs_b->period_timer.function = sched_cfs_period_timer;
> >         hrtimer_init(&cfs_b->slack_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >         cfs_b->slack_timer.function = sched_cfs_slack_timer;
> > -       cfs_b->distribute_running = 0;
> >         cfs_b->slack_started = false;
> >  }
> >
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index db3a57675ccf..7198683b2869 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -349,7 +349,6 @@ struct cfs_bandwidth {
> >
> >         u8                      idle;
> >         u8                      period_active;
> > -       u8                      distribute_running;
> >         u8                      slack_started;
> >         struct hrtimer          period_timer;
> >         struct hrtimer          slack_timer;
> > --
> > 2.26.0.110.g2183baf09c-goog
> >
> 
> +pauld@...hat.com
> 

--