linux-kernel - Re: [PATCH 2/7] sched/fair: prepare throttle path for task based throttle

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: <20250523080319.GB1168183@bytedance>
Date: Fri, 23 May 2025 16:03:19 +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 2/7] sched/fair: prepare throttle path for task based
 throttle

On Thu, May 22, 2025 at 07:43:40PM +0800, Chengming Zhou wrote:
> On 2025/5/21 17:21, Aaron Lu wrote:
> > On Wed, May 21, 2025 at 05:01:58PM +0800, Chengming Zhou wrote:
> > > On 2025/5/20 18:41, 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 wakeup, 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. In this way, tasks throttled will not
> > > > hold any kernel resources.
> > > > 
> > > > To avoid breaking bisect, preserve the current throttle behavior by
> > > > still dequeuing throttled hierarchy from rq and because of this, no task
> > > > can have that throttle task work added yet. The throttle model will
> > > > switch to task based in a later patch.
> > > > 
> > > > 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>
> > > 
> > > I'm wondering how about put 02-04 patches together, since it's strange
> > > to setup task work in this patch but without changing throttle_cfs_rq(),
> > 
> > Do you mean 02-05?
> > Because the actual change to throttle_cfs_rq() happens in patch5 :)
> 
> Ah, right.
> 
> > 
> > > which makes the reviewing process a bit confused? WDYT?
> > 
> > Yes, I agree it looks a bit confused.
> > 
> > The point is to not break bisect while make review easier; if merging
> > all task based throttle related patches together, that would be to put
> > patch 02-05 together, which seems too big?
> 
> Yeah, a big patch but complete, instead of changing a bit on the same
> function in each patch. Anyway, it's your call :-)
>

Thanks for the suggestion and I can try that in next version.

I'm thinking maybe one patch for newly added data structures and one
patch for throttle related helper functions, than a complete patch to
switch to task based throttle and the rest is the same.

Let's also see if others have opinions on this :)