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

Chengming Zhou
2025-06-18T09:55:24Z

On 2025/6/18 16:19, 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.
>
> Throttled cfs_rq's leaf_cfs_rq_list is handled differently now: since a
> task can be enqueued to a throttled cfs_rq and gets to run, to not break
> the assert_list_leaf_cfs_rq() in enqueue_task_fair(), always add it to
> leaf cfs_rq list when it has its first entity enqueued and delete it
> from leaf cfs_rq list when it has no tasks enqueued.
>
> 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>
> ---
> kernel/sched/fair.c | 325 +++++++++++++++++++++-----------------------
> 1 file changed, 153 insertions(+), 172 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8226120b8771a..59b372ffae18c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5291,18 +5291,17 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>
> if (cfs_rq->nr_queued == 1) {
> check_enqueue_throttle(cfs_rq);
> - if (!throttled_hierarchy(cfs_rq)) {
> - list_add_leaf_cfs_rq(cfs_rq);
> - } else {
> + list_add_leaf_cfs_rq(cfs_rq);
> #ifdef CONFIG_CFS_BANDWIDTH
> + if (throttled_hierarchy(cfs_rq)) {
> struct rq *rq = rq_of(cfs_rq);
>
> if (cfs_rq_throttled(cfs_rq) && !cfs_rq->throttled_clock)
> cfs_rq->throttled_clock = rq_clock(rq);
> if (!cfs_rq->throttled_clock_self)
> cfs_rq->throttled_clock_self = rq_clock(rq);
> -#endif
> }
> +#endif
> }
> }
>
> @@ -5341,8 +5340,6 @@ static void set_delayed(struct sched_entity *se)
> struct cfs_rq *cfs_rq = cfs_rq_of(se);
>
> cfs_rq->h_nr_runnable--;
> - if (cfs_rq_throttled(cfs_rq))
> - break;
> }
> }
>
> @@ -5363,8 +5360,6 @@ static void clear_delayed(struct sched_entity *se)
> struct cfs_rq *cfs_rq = cfs_rq_of(se);
>
> cfs_rq->h_nr_runnable++;
> - if (cfs_rq_throttled(cfs_rq))
> - break;
> }
> }
>
> @@ -5450,8 +5445,11 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> if (flags & DEQUEUE_DELAYED)
> finish_delayed_dequeue_entity(se);
>
> - if (cfs_rq->nr_queued == 0)
> + if (cfs_rq->nr_queued == 0) {
> update_idle_cfs_rq_clock_pelt(cfs_rq);
> + if (throttled_hierarchy(cfs_rq))
> + list_del_leaf_cfs_rq(cfs_rq);

The cfs_rq should be removed from leaf list only after
it has been fully decayed, not here.

Thanks.