On Sun, Jun 07, 2020 at 09:25:58AM +0800 Tao Zhou wrote:
> Hi,
>
> On Fri, May 01, 2020 at 06:22:12PM -0000, tip-bot2 for Josh Don wrote:
> > The following commit has been merged into the sched/core branch of tip:
> >
> > Commit-ID: ab93a4bc955b3980c699430bc0b633f0d8b607be
> > Gitweb:
> > https://git.kernel.org/tip/ab93a4bc955b3980c699430bc0b633f0d8b607be
> > Author: Josh Don <josh...@google.com>
> > AuthorDate: Fri, 10 Apr 2020 15:52:08 -07:00
> > Committer: Peter Zijlstra <pet...@infradead.org>
> > CommitterDate: Thu, 30 Apr 2020 20:14:38 +02:00
> >
> > sched/fair: Remove distribute_running from CFS bandwidth
> >
> > This is mostly a revert of commit:
> >
> > baa9be4ffb55 ("sched/fair: Fix throttle_list starvation with low CFS
> > quota")
> >
> > 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 <josh...@google.com>
> > Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> > Reviewed-by: Phil Auld <pa...@redhat.com>
> > Tested-by: Phil Auld <pa...@redhat.com>
> > Link: https://lkml.kernel.org/r/20200410225208.109717-3-josh...@google.com
> > ---
> > 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 0c13a41..3d6ce75 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4931,14 +4931,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);
> > }
> >
> > @@ -5052,10 +5050,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);
> > @@ -5065,9 +5059,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)
> > @@ -5076,7 +5067,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);
> > }
>
> When I read the tip code, I found nothing between above lock/unlock.
> This commit removed distribute_running. Is there any reason to remain
> that lock/unlock there ? I feel that it is not necessary now, no ?
>
Yeah, that looks pretty useless :)
Do you want to spin up a patch?
Cheers,
Phil
> Thanks
>
> > @@ -5218,7 +5208,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 db3a576..7198683 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;
>
--
