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] [day] [month] [year] [list]
Message-ID: <20250618111913.GA646@bytedance>
Date: Wed, 18 Jun 2025 19:19:13 +0800
From: Aaron Lu <ziqianlu@...edance.com>
To: Chengming Zhou <chengming.zhou@...ux.dev>
Cc: 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>, 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

Hi Chengming,

Thanks for your review.

On Wed, Jun 18, 2025 at 05:55:08PM +0800, Chengming Zhou wrote:
> 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.

For a throttled cfs_rq, the intent is to preserve its load while it's
throttled. Its pelt clock is stopped in tg_throttle_down(), there will
be no decay for it if left on leaf list.

I've also described why I chose this behaviour in cover letter:
"
For pelt clock, I chose to keep the current behavior to freeze it on
cfs_rq's throttle time. The assumption is that tasks running in kernel
mode should not last too long, freezing the cfs_rq's pelt clock can keep
its load and its corresponding sched_entity's weight. Hopefully, this can
result in a stable situation for the remaining running tasks to quickly
finish their jobs in kernel mode.
"

Thanks,
Aaron