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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddca03a3-14d2-4d71-8070-8f9b8de9b7eb@linux.dev>
Date: Wed, 18 Jun 2025 17:55:08 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Aaron Lu <ziqianlu@...edance.com>,
 Valentin Schneider <vschneid@...hat.com>, Ben Segall <bsegall@...gle.com>,
 K Prateek Nayak <kprateek.nayak@....com>,
 Peter Zijlstra <peterz@...radead.org>, Josh Don <joshdon@...gle.com>,
 Ingo Molnar <mingo@...hat.com>, Vincent Guittot
 <vincent.guittot@...aro.org>, Xi Wang <xii@...gle.com>
Cc: linux-kernel@...r.kernel.org, Juri Lelli <juri.lelli@...hat.com>,
 Dietmar Eggemann <dietmar.eggemann@....com>,
 Steven Rostedt <rostedt@...dmis.org>, Mel Gorman <mgorman@...e.de>,
 Chuyi Zhou <zhouchuyi@...edance.com>, Jan Kiszka <jan.kiszka@...mens.com>,
 Florian Bezdeka <florian.bezdeka@...mens.com>
Subject: Re: [PATCH v2 3/5] sched/fair: Switch to task based throttle model

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