Re: [tip: sched/core] sched/fair: Remove distribute_running from CFS bandwidth

[Phil Auld]

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 <joshdon@xxxxxxxxxx>
> > AuthorDate:    Fri, 10 Apr 2020 15:52:08 -07:00
> > Committer:     Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > 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 <joshdon@xxxxxxxxxx>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > Reviewed-by: Phil Auld <pauld@xxxxxxxxxx>
> > Tested-by: Phil Auld <pauld@xxxxxxxxxx>
> > Link: https://lkml.kernel.org/r/20200410225208.109717-3-joshdon@xxxxxxxxxx
> > ---
> >  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;
> 

--