Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 15 Mar 2019 16:59:33 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Phil Auld <pauld@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Ben Segall <bsegall@google.com>,
        Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] sched/fair: Limit sched_cfs_period_timer loop to avoid
 hard lockup
Message-ID: <20190315155933.GY5996@hirez.programming.kicks-ass.net>
References: <20190313150826.16862-1-pauld@redhat.com>
 <20190315101150.GV5996@hirez.programming.kicks-ass.net>
 <20190315103357.GC6521@hirez.programming.kicks-ass.net>
 <20190315135124.GC27131@pauld.bos.csb>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190315135124.GC27131@pauld.bos.csb>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 15, 2019 at 09:51:25AM -0400, Phil Auld wrote:
> On Fri, Mar 15, 2019 at 11:33:57AM +0100 Peter Zijlstra wrote:
> > On Fri, Mar 15, 2019 at 11:11:50AM +0100, Peter Zijlstra wrote:
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index ea74d43924b2..b71557be6b42 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -4885,6 +4885,8 @@ static enum hrtimer_restart sched_cfs_slack_timer(struct hrtimer *timer)
> > >  	return HRTIMER_NORESTART;
> > >  }
> > >  
> > > +extern const u64 max_cfs_quota_period;
> > > +
> > >  static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer)
> > >  {
> > >  	struct cfs_bandwidth *cfs_b =
> > > @@ -4892,6 +4894,7 @@ static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer)
> > >  	unsigned long flags;
> > >  	int overrun;
> > >  	int idle = 0;
> > > +	int count = 0;
> > >  
> > >  	raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > >  	for (;;) {
> > > @@ -4899,6 +4902,28 @@ static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer)
> > >  		if (!overrun)
> > >  			break;
> > >  
> > > +		if (++count > 3) {
> > > +			u64 new, old = ktime_to_ns(cfs_b->period);
> > > +
> > > +			new = (old * 147) / 128; /* ~115% */
> > > +			new = min(new, max_cfs_quota_period);
> > 
> > Also, we can still engineer things to come unstuck; if we explicitly
> > configure period at 1e9 and then set a really small quota and then
> > create this insane amount of cgroups you have..
> > 
> > this code has no room to manouvre left.
> > 
> > Do we want to do anything about that? Or leave it as is, don't do that
> > then?
> > 
> 
> If the period is 1s it would be hard to make this loop fire repeatedly. I don't think
> it's that dependent on the quota other than getting some rqs throttled. The small quota
> would also mean fewer of them would get unthrottled per distribute call. You'd probably
> need _significantly_ more cgroups than my insane 2500 to hit it. 
> 
> Right now it settles out with a new period of ~12-15ms.  So ~200,000 cgroups?
> 
> Ben and I talked a little about this in another thread. I think hitting this is enough of 
> an edge case that this approach will make the problem go away. The only alternative we 
> came up with to reduce the time taken in unthrottle involved a fair bit of complexity 
> added to the every day code paths.  And might not help if the children all had their
> own quota/period settings active. 

Ah right. I forgot that part. And yes, I remember what was proposed to
avoid the tree walk, that wouldn't have been pretty.