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

[Phil Auld <pauld@redhat.com>]

On Tue, Jun 09, 2020 at 07:05:38AM +0800 Tao Zhou wrote:
> Hi Phil,
> 
> On Mon, Jun 08, 2020 at 10:53:04AM -0400, Phil Auld wrote:
> > 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@google.com>
> > > > AuthorDate:    Fri, 10 Apr 2020 15:52:08 -07:00
> > > > Committer:     Peter Zijlstra <peterz@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 <joshdon@google.com>
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > Reviewed-by: Phil Auld <pauld@redhat.com>
> > > > Tested-by: Phil Auld <pauld@redhat.com>
> > > > Link: https://lkml.kernel.org/r/20200410225208.109717-3-joshdon@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?
> 
> Thanks for your reply, I prepared a patch for that indeed.
> 
> Attached below:
> 
> From 9ce12d6ab5542041e5adab7b200874c789cfd6e6 Mon Sep 17 00:00:00 2001
> From: Tao Zhou <ouwen210@hotmail.com>
> Date: Sat, 6 Jun 2020 23:08:58 +0800
> Subject: [PATCH] sched/fair: remove the residual cfs_b lock/unlock
> 
> After commit ab93a4bc955b, cfs_b->distribute_running is not used and
> removed. The lock/unlock protecting it are not removed and remain in
> the code. One benefit of removing them is that it can elimite the code
> size a little.
> 
> Fixes: ab93a4bc955b ("sched/fair: Remove distribute_running from CFS bandwidth")
> ---
>  kernel/sched/fair.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 35f4cc024dcf..cc2e1e839e03 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5089,9 +5089,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
>  		return;
>  
>  	distribute_cfs_runtime(cfs_b);
> -
> -	raw_spin_lock_irqsave(&cfs_b->lock, flags);
> -	raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
>  }
>  
>  /*
> --


Thanks Tao.

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

> 
> Thanks,
> Tao
> 
> > 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;
> > > 
> > 
> > -- 
> 

--