Messages in this thread | | | Date | Mon, 3 Jul 2023 11:24:40 -0400 | From | Phil Auld <> | 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
--
| |