Message-ID: <20230703152440.GC67396@lorien.usersys.redhat.com>
Date:   Mon, 3 Jul 2023 11:24:40 -0400
From:   Phil Auld <pauld@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Juri Lelli <juri.lelli@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Valentin Schneider <vschneid@...hat.com>,
        Ben Segall <bsegall@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mel Gorman <mgorman@...e.de>,
        Frederic Weisbecker <frederic@...nel.org>
Subject: Re: [PATCH v4] Sched/fair: Block nohz tick_stop when cfs bandwidth
 in use

On Mon, Jul 03, 2023 at 04:32:57PM +0200 Peter Zijlstra wrote:
> On Mon, Jul 03, 2023 at 10:10:56AM -0400, Phil Auld wrote:
> > On Mon, Jul 03, 2023 at 02:10:09PM +0200 Peter Zijlstra wrote:
> > > On Fri, Jun 30, 2023 at 12:29:10PM -0400, Phil Auld wrote:
> > > 
> > > > I think you are agreeing that I need the pick next code but need to remove
> > > > the hierarchy walks, right?
> > > 
> > > Yeah, the dequeue case makes we have to care about pick, not sure we
> > > then also need to care about sched_update_tick_dependency() though.
> > > There is indeed a window where these two will 'race', but afaict it is
> > > benign.
> > > 
> > 
> > Hm, that's confusing.
> > 
> > As I see it it's the enqueue case (0->1 mostly) where we need the check
> > in pick.  At that point in enqueue we only have a handle on ->curr which
> > is the idle thread.
> 
> Well, the 0->1 case is trivial, we'll run the task that's enqueued, and
> as such everything can DTRT and be simple.
>

Right, but we have to do it (check for bw_constraint and set the TICK_DEP bit)
in pick because we don't have a handle on the task that's enqueued in
sched_update_tick_dependency(). Simple :)

> > For the dequeue case (2->1) we need the check in the
> > sched_update_tick_dependency() path because if the 1 is the task on the
> > cpu (and is staying there) then we'd otherwise clear the bit when we
> > shouldn't (since we aren't going to go back through pick).
> 
> The 2->1 case OTOH is tricky, because then we'll end up running a task
> we've not recently seen. sub_nr_running() will hit the ==1 case and
> clear TICK_DEP_BIT_SCHED.
> 
> But then pick will come and set it again, no harm done, right?
> 
> .oO Ah!, You're worried about the case where a task is already running,
> a second task comes in, (1->2) and then quickly leaves again (2->1)
> without passing through schedule(). And you don't want to disable the
> tick if that running task needs it.
> 
> Mooo :-(
>

Yeah, Ben pointed that out and then I was able to rt-app a way to hit it
reliably.

> > I'm thinking that I'll try to set the bit in pick since we only care about
> > it when it's the task on the cpu.  That, I think, will simplify the
> > code needed to update the bit when the quota is changed (to or from
> > RUNTIME_INF).
> > 
> > Setting the bit in enqueue/dequeue means updating it on all the queued
> > task if it changes. Although I may clear it in dequeue just to not leave
> > it around stale.
> 
> Hmm, no you have to set on enqueue (1->2), otherwise the running task
> doesn't get preempted when it runs out of slice.

Sorry, I'm not sure I'm following. I meant the bw_constrained bit in the
task not the actual TICK_DEP bit. 

So in this case we don't go through pick because we may be preempting
from say a wakeup? If we stay at 2 none of this matters because
the existing tick_dependency stuff will work (nr_running > 1)

That's why I wanted to clarify which bit I was talking about where.

Ah... If we go from 1->2 via a wakeup and preempt rather than pick_next
then the task would not get the bw_constrained bit set if we
then drop from 2->1.  Right, okay. Will need to set it in enqueue
and update all queued tasks if bandwidth changes. Or also update it
in pick, maybe. I.e. make sure task::bw_constrained is still right when
we actually land on the cpu because the only place we really care about
it is when we are ->curr.

> 
> And I don't suppose you want to delay clearing to the first tick after,
> because NOHZ_FULL doesn't want spurious ticks :/

Here you mean clearing the TICK_DEP yes?

>
> What a mess.
> 
> Please document all these stupid cases in a comment, otherwise we'll go
> bananas trying to make sense of the code later on.
> 

Will do. 

Thanks for your input.


Cheers,
Phil


-- 

Powered by blists - more mailing lists