From: Phil Auld <pauld@redhat.com>
To: Josh Don <joshdon@google.com>
Cc: Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Juri Lelli <juri.lelli@redhat.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
Paul Turner <pjt@google.com>,
Huaixin Chang <changhuaixin@linux.alibaba.com>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] sched: remove distribute_running from CFS bandwidth
Date: Mon, 13 Apr 2020 10:49:15 -0400 [thread overview]
Message-ID: <20200413144915.GB2517@lorien.usersys.redhat.com> (raw)
In-Reply-To: <CABk29NtBxtJeAowLTGC4xQRD5MNtoiKLTLV7O3zLWp1CtW4nvg@mail.gmail.com>
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@google.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@google.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@redhat.com>
Tested-by: Phil Auld <pauld@redhat.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@redhat.com
>
--
next prev parent reply other threads:[~2020-04-13 14:49 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-10 22:52 [PATCH 0/2] Fix race in CFS bandwidth Josh Don
2020-04-10 22:52 ` [PATCH 1/2] sched: eliminate bandwidth race between throttling and distribution Josh Don
[not found] ` <CABk29NtxG8t6wHM6MDVFun7jxqRpupWr0d5dK57N2ecFbdSumw@mail.gmail.com>
2020-04-13 14:44 ` Phil Auld
2020-04-14 10:52 ` Peter Zijlstra
2020-05-01 18:22 ` [tip: sched/core] sched/fair: Eliminate " tip-bot2 for Paul Turner
2020-04-10 22:52 ` [PATCH 2/2] sched: remove distribute_running from CFS bandwidth Josh Don
2020-04-12 2:01 ` Josh Don
2020-04-13 14:49 ` Phil Auld [this message]
2020-04-14 10:54 ` Peter Zijlstra
2020-05-01 18:22 ` [tip: sched/core] sched/fair: Remove " tip-bot2 for Josh Don
[not found] ` <BL0PR14MB3779C02BB87DC4426C4761639A840@BL0PR14MB3779.namprd14.prod.outlook.com>
2020-06-08 14:53 ` Phil Auld
[not found] ` <BL0PR14MB3779AD967619031948957C549A850@BL0PR14MB3779.namprd14.prod.outlook.com>
2020-06-08 23:44 ` Josh Don
2020-06-09 0:28 ` Phil Auld
2020-04-12 2:03 ` [PATCH 0/2] Fix race in " Josh Don
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200413144915.GB2517@lorien.usersys.redhat.com \
--to=pauld@redhat.com \
--cc=bsegall@google.com \
--cc=changhuaixin@linux.alibaba.com \
--cc=dietmar.eggemann@arm.com \
--cc=joshdon@google.com \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=rostedt@goodmis.org \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).