Re: [PATCH v3 3/5] sched/fair: Switch to task based throttle model

[Aaron Lu <ziqianlu@bytedance.com>]

On Fri, Aug 08, 2025 at 11:12:48AM +0200, Valentin Schneider wrote:
> On 15/07/25 15:16, Aaron Lu wrote:
> > From: Valentin Schneider <vschneid@redhat.com>
> >
> > In current throttle model, when a cfs_rq is throttled, its entity will
> > be dequeued from cpu's rq, making tasks attached to it not able to run,
> > thus achiveing the throttle target.
> >
> > This has a drawback though: assume a task is a reader of percpu_rwsem
> > and is waiting. When it gets woken, it can not run till its task group's
> > next period comes, which can be a relatively long time. Waiting writer
> > will have to wait longer due to this and it also makes further reader
> > build up and eventually trigger task hung.
> >
> > To improve this situation, change the throttle model to task based, i.e.
> > when a cfs_rq is throttled, record its throttled status but do not remove
> > it from cpu's rq. Instead, for tasks that belong to this cfs_rq, when
> > they get picked, add a task work to them so that when they return
> > to user, they can be dequeued there. In this way, tasks throttled will
> > not hold any kernel resources. And on unthrottle, enqueue back those
> > tasks so they can continue to run.
> >
> 
> Moving the actual throttle work to pick time is clever. In my previous
> versions I tried really hard to stay out of the enqueue/dequeue/pick paths,
> but this makes the code a lot more palatable. I'd like to see how this
> impacts performance though.
> 

Let me run some scheduler benchmark to see how it impacts performance.

I'm thinking maybe running something like hackbench on server platforms,
first with quota not set and see if performance changes; then also test
with quota set and see how performance changes.

Does this sound good to you? Or do you have any specific benchmark and
test methodology in mind?

> > Throttled cfs_rq's PELT clock is handled differently now: previously the
> > cfs_rq's PELT clock is stopped once it entered throttled state but since
> > now tasks(in kernel mode) can continue to run, change the behaviour to
> > stop PELT clock only when the throttled cfs_rq has no tasks left.
> >
> 
> I haven't spent much time looking at the PELT stuff yet; I'll do that next
> week. Thanks for all the work you've been putting into this, and sorry it
> got me this long to get a proper look at it!
> 

Never mind and thanks for taking a look now :)

> > Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
> > Suggested-by: Chengming Zhou <chengming.zhou@linux.dev> # tag on pick
> > Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> > Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
> 
> > +static bool enqueue_throttled_task(struct task_struct *p)
> > +{
> > +	struct cfs_rq *cfs_rq = cfs_rq_of(&p->se);
> > +
> > +	/*
> > +	 * If the throttled task is enqueued to a throttled cfs_rq,
> > +	 * take the fast path by directly put the task on target
> > +	 * cfs_rq's limbo list, except when p is current because
> > +	 * the following race can cause p's group_node left in rq's
> > +	 * cfs_tasks list when it's throttled:
> > +	 *
> > +	 *        cpuX                       cpuY
> > +	 *   taskA ret2user
> > +	 *  throttle_cfs_rq_work()    sched_move_task(taskA)
> > +	 *  task_rq_lock acquired
> > +	 *  dequeue_task_fair(taskA)
> > +	 *  task_rq_lock released
> > +	 *                            task_rq_lock acquired
> > +	 *                            task_current_donor(taskA) == true
> > +	 *                            task_on_rq_queued(taskA) == true
> > +	 *                            dequeue_task(taskA)
> > +	 *                            put_prev_task(taskA)
> > +	 *                            sched_change_group()
> > +	 *                            enqueue_task(taskA) -> taskA's new cfs_rq
> > +	 *                                                   is throttled, go
> > +	 *                                                   fast path and skip
> > +	 *                                                   actual enqueue
> > +	 *                            set_next_task(taskA)
> > +	 *                          __set_next_task_fair(taskA)
> > +	 *                    list_move(&se->group_node, &rq->cfs_tasks); // bug
> > +	 *  schedule()
> > +	 *
> > +	 * And in the above race case, the task's current cfs_rq is in the same
> > +	 * rq as its previous cfs_rq because sched_move_task() doesn't migrate
> > +	 * task so we can use its current cfs_rq to derive rq and test if the
> > +	 * task is current.
> > +	 */
> 
> OK I think I got this; I slightly rephrased things to match similar
> comments in the sched code:
> 
> --->8---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3a7c86c5b8a2b..8566ee0399527 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5827,37 +5827,38 @@ static bool enqueue_throttled_task(struct task_struct *p)
>  	struct cfs_rq *cfs_rq = cfs_rq_of(&p->se);
>  
>  	/*
> -	 * If the throttled task is enqueued to a throttled cfs_rq,
> -	 * take the fast path by directly put the task on target
> -	 * cfs_rq's limbo list, except when p is current because
> -	 * the following race can cause p's group_node left in rq's
> -	 * cfs_tasks list when it's throttled:
> +	 * If the throttled task @p is enqueued to a throttled cfs_rq,
> +	 * take the fast path by directly putting the task on the
> +	 * target cfs_rq's limbo list.
> +
> +	 * Do not do that when @p is current because the following race can
> +	 * cause @p's group_node to be incorectly re-insterted in its in rq's
> +	 * cfs_tasks list, despite being throttled:
>  	 *
>  	 *        cpuX                       cpuY
> -	 *   taskA ret2user
> -	 *  throttle_cfs_rq_work()    sched_move_task(taskA)
> -	 *  task_rq_lock acquired
> -	 *  dequeue_task_fair(taskA)
> -	 *  task_rq_lock released
> -	 *                            task_rq_lock acquired
> -	 *                            task_current_donor(taskA) == true
> -	 *                            task_on_rq_queued(taskA) == true
> -	 *                            dequeue_task(taskA)
> -	 *                            put_prev_task(taskA)
> -	 *                            sched_change_group()
> -	 *                            enqueue_task(taskA) -> taskA's new cfs_rq
> -	 *                                                   is throttled, go
> -	 *                                                   fast path and skip
> -	 *                                                   actual enqueue
> -	 *                            set_next_task(taskA)
> -	 *                          __set_next_task_fair(taskA)
> -	 *                    list_move(&se->group_node, &rq->cfs_tasks); // bug
> +	 *  p ret2user
> +	 *    throttle_cfs_rq_work()  sched_move_task(p)
> +	 *    LOCK task_rq_lock
> +	 *    dequeue_task_fair(p)
> +	 *    UNLOCK task_rq_lock
> +	 *                              LOCK task_rq_lock
> +	 *                              task_current_donor(p) == true
> +	 *                              task_on_rq_queued(p) == true
> +	 *                              dequeue_task(p)
> +	 *                              put_prev_task(p)
> +	 *                              sched_change_group()
> +	 *                              enqueue_task(p) -> p's new cfs_rq
> +	 *                                                 is throttled, go
> +	 *                                                 fast path and skip
> +	 *                                                 actual enqueue
> +	 *                              set_next_task(p)
> +	 *                                list_move(&se->group_node, &rq->cfs_tasks); // bug
>  	 *  schedule()
>  	 *
> -	 * And in the above race case, the task's current cfs_rq is in the same
> -	 * rq as its previous cfs_rq because sched_move_task() doesn't migrate
> -	 * task so we can use its current cfs_rq to derive rq and test if the
> -	 * task is current.
> +	 * In the above race case, @p current cfs_rq is in the same
> +	 * rq as its previous cfs_rq because sched_move_task() only moves a task
> +	 * to a different group from the same rq, so we can use its current
> +	 * cfs_rq to derive rq and test if the * task is current.
>  	 */
>  	if (throttled_hierarchy(cfs_rq) &&
>  	    !task_current_donor(rq_of(cfs_rq), p)) {
> ---8<---
>

Will follow your suggestion in next version.

> > +	if (throttled_hierarchy(cfs_rq) &&
> > +	    !task_current_donor(rq_of(cfs_rq), p)) {
> > +		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
> > +		return true;
> > +	}
> > +
> > +	/* we can't take the fast path, do an actual enqueue*/
> > +	p->throttled = false;
> 
> So we clear p->throttled but not p->throttle_node? Won't that cause issues
> when @p's previous cfs_rq gets unthrottled?
> 

p->throttle_node is already removed from its previous cfs_rq at dequeue
time in dequeue_throttled_task().

This is done so because in enqueue time, we may not hold its previous
rq's lock so can't touch its previous cfs_rq's limbo list, like when
dealing with affinity changes.

> > +	return false;
> > +}
> > +
> 
> > @@ -7145,6 +7142,11 @@ static int dequeue_entities(struct rq *rq, struct sched_entity *se, int flags)
> >   */
> >  static bool dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >  {
> > +	if (unlikely(task_is_throttled(p))) {
> > +		dequeue_throttled_task(p, flags);
> > +		return true;
> > +	}
> > +
> 
> Handling a throttled task's move pattern at dequeue does simplify things,
> however that's quite a hot path. I think this wants at the very least a
> 
>   if (cfs_bandwidth_used())
> 
> since that has a static key underneath. Some heavy EQ/DQ benchmark wouldn't
> hurt, but we can probably find some people who really care about that to
> run it for us ;)
> 

No problem.

I'm thinking maybe I can add this cfs_bandwidth_used() in
task_is_throttled()? So that other callsites of task_is_throttled() can
also get the benefit of paying less cost when cfs bandwidth is not
enabled.

> >  	if (!p->se.sched_delayed)
> >  		util_est_dequeue(&rq->cfs, p);
> >  
>