Re: [PATCH 01/14] sched: Employ sched_change guards

Peter Zijlstra
2025-09-11T09:55:31Z

On Thu, Sep 11, 2025 at 02:36:21PM +0530, K Prateek Nayak wrote:
> Hello Peter,
>
> On 9/10/2025 9:14 PM, Peter Zijlstra wrote:
> > @@ -9240,8 +9213,9 @@ static void sched_change_group(struct ta
> > */
> > void sched_move_task(struct task_struct *tsk, bool for_autogroup)
> > {
> > - int queued, running, queue_flags =
> > + unsigned int queue_flags =
> > DEQUEUE_SAVE | DEQUEUE_MOVE | DEQUEUE_NOCLOCK;
>
> nit.
>
> Since we don't do a complete dequeue for delayed task in
> sched_move_task(), can we get rid of that DEQUEUE_NOCLOCK and ...
>
> > + bool resched = false;
> > struct rq *rq;
> >
> > CLASS(task_rq_lock, rq_guard)(tsk);
> > @@ -9249,28 +9223,12 @@ void sched_move_task(struct task_struct
> >
> > update_rq_clock(rq);
>
> ... this clock update and instead rely on sched_change_begin() to
> handle it within the guard?

Yeah, I suppose we could. But let me try and do that in a later patch,
on-top of all this.

> > - running = task_current_donor(rq, tsk);
> > - queued = task_on_rq_queued(tsk);
> > -
> > - if (queued)
> > - dequeue_task(rq, tsk, queue_flags);
> > - if (running)
> > - put_prev_task(rq, tsk);
> > -
> > - sched_change_group(tsk);
> > - if (!for_autogroup)
> > - scx_cgroup_move_task(tsk);
> > -
> > - if (queued)
> > - enqueue_task(rq, tsk, queue_flags);
> > - if (running) {
> > - set_next_task(rq, tsk);
> > - /*
> > - * After changing group, the running task may have joined a
> > - * throttled one but it's still the running task. Trigger a
> > - * resched to make sure that task can still run.
> > - */
> > - resched_curr(rq);
> > + scoped_guard (sched_change, tsk, queue_flags) {
> > + sched_change_group(tsk);
> > + if (!for_autogroup)
> > + scx_cgroup_move_task(tsk);
> > + if (scope->running)
> > + resched = true;
> > }
>
> Also, are we missing a:
>
> if (resched)
> resched_curr(rq);
>
> here after the guard? I don't see anything in sched_change_end() at this
> point that would trigger a resched.

Bah, yes. That hunk must've gone missing in one of the many rebases I
did while folding back fixes :/