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

[Chengming Zhou]

On 2025/6/18 16:19, Aaron Lu wrote:
> From: Valentin Schneider <vschneid@xxxxxxxxxx>
>
> 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@xxxxxxxxx> # tag on pick
> Signed-off-by: Valentin Schneider <vschneid@xxxxxxxxxx>
> Signed-off-by: Aaron Lu <ziqianlu@xxxxxxxxxxxxx>
> ---
>   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.