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

[Aaron Lu <ziqianlu@bytedance.com>]

On Wed, Sep 03, 2025 at 01:55:36PM -0700, Benjamin Segall wrote:
> Aaron Lu <ziqianlu@bytedance.com> writes:
> 
> > +static bool enqueue_throttled_task(struct task_struct *p)
> > +{
> > +	struct cfs_rq *cfs_rq = cfs_rq_of(&p->se);
> > +
> > +	/* @p should have gone through dequeue_throttled_task() first */
> > +	WARN_ON_ONCE(!list_empty(&p->throttle_node));
> > +
> > +	/*
> > +	 * 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 rq's
> > +	 * cfs_tasks list, despite being throttled:
> > +	 *
> > +	 *     cpuX                       cpuY
> > +	 *   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()
> > +	 *
> > +	 * 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)) {
> > +		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;
> > +	return false;
> > +}
> > +
> 
> Is there a reason that __set_next_task_fair cannot check p->se.on_rq as
> well as (or instead of) task_on_rq_queued()? All of the _entity parts of
> set_next/put_prev check se.on_rq for this sort of thing, so that seems
> fairly standard. And se.on_rq should exactly match if the task is on
> cfs_tasks since that add/remove is done in account_entity_{en,de}queue.

Makes sense to me.

Only thing that feels a little strange is, a throttled/dequeued task is
set as next now. Maybe not a big deal. I booted a VM and run some tests,
didn't notice anything wrong but I could very well miss some cases.