linux-kernel - Re: [PATCH] sched/fair: Prevent cfs_rq from being unthrottled with zero runtime

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: <20251016092300.GB32@bytedance>
Date: Thu, 16 Oct 2025 17:23:00 +0800
From: Aaron Lu <ziqianlu@...edance.com>
To: Hao Jia <jiahao.kernel@...il.com>
Cc: Valentin Schneider <vschneid@...hat.com>,
	Ben Segall <bsegall@...gle.com>,
	K Prateek Nayak <kprateek.nayak@....com>,
	Peter Zijlstra <peterz@...radead.org>,
	Chengming Zhou <chengming.zhou@...ux.dev>,
	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>,
	Songtang Liu <liusongtang@...edance.com>,
	Chen Yu <yu.c.chen@...el.com>,
	Matteo Martelli <matteo.martelli@...ethink.co.uk>,
	Michal Koutný <mkoutny@...e.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH] sched/fair: Prevent cfs_rq from being unthrottled with
 zero runtime_remaining

On Thu, Oct 16, 2025 at 03:49:15PM +0800, Hao Jia wrote:
> 
> Hi Aaron,
> 
> On 2025/10/16 14:54, Aaron Lu wrote:
> > On Wed, Oct 15, 2025 at 06:21:01PM +0800, Hao Jia wrote:
> > > On 2025/10/15 16:40, Aaron Lu wrote:
> > ... ...
> > > > Hao Jia,
> > > > 
> > > > Do I understand you correctly that you can only hit the newly added
> > > > debug warn in tg_unthrottle_up():
> > > > WARN_ON_ONCE(cfs_rq->runtime_enabled && cfs_rq->runtime_remaining <= 0);
> > > > but not throttle triggered on unthrottle path?
> > > > 
> > > 
> > > yes. but I'm not sure if there are other corner cases where
> > > cfs_rq->runtime_remaining <= 0 and cfs_rq->curr is NULL.
> > > 
> > 
> > Right, I'm not aware of any but might be possible.
> > 
> > > > BTW, I think your change has the advantage of being straightforward and
> > > > easy to reason about. My concern is, it's not efficient to enqueue tasks
> > > > to a cfs_rq that has no runtime left, not sure how big a deal that is
> > > > though.
> > > 
> > > Yes, but that's what we're doing now. The case described above involves
> > > enqueue a task where cfs_rq->runtime_remaining <= 0.
> > > 
> > > I previously tried adding a runtime_remaining check for each level of task
> > > p's cfs_rq in unthrottle_cfs_rq()/tg_unthrottle_up(), but this made the code
> > > strange and complicated.
> > 
> > Agree that adding a runtime_remaining check for each level in
> > unthrottle_cfs_rq() looks too complex.
> > 
> > So I think you approach is fine, feel free to submit a formal patch.
> > With your change, theoretically we do not need to do those
> > runtime_remaining check in unthrottle_cfs_rq() but keeping that check
> > could save us some unnecessary enqueues, so I'll leave it to you to
> > decide if you want to keep it or not. If you want to keep it, please
> > also change its comments because the current comments will be stale
> > then.
> > 
> 
> Thank you for your suggestion. I'll send a formal patch later.
> 
> I'm also happy for you to submit a patch for the next version. This warning
> needs to be fixed, regardless of the method.

With your change, task enqueue in unthrottle path will not call
check_enqueue_path(), thus the warn on non-empty limbo list in
tg_throttle_down() should not happen, so I suppose we do not need
this patch anymore, no?

> 
> However, I've discovered a minor bug in your current patch.
> 
> In kernel/sched/core.c tg_set_cfs_bandwidth()
> 
> ...
> if (cfs_rq->runtime_enabled && !cfs_rq->throttled) {
>     update_rq_clock(rq);   <----
>     throttle_cfs_rq(cfs_rq);
> }
> ...
> 
> Call update_rq_clock() to avoid the warning about using an outdated rq_clock
> in tg_throttle_down()->rq_clock_pelt().

With the above said, this shouldn't matter anymore but just out of
curiosity: did you notice this by inspecting the code or actually
hitting the warning about using an outdated rq clock?

Per my understanding, most likely: __assign_cfs_rq_runtime() in
throttle_cfs_rq(cfs_rq) will grant 1ns runtime to cfs_rq so it won't
reach tg_throttle_down(). The comment I added above that if condition
is kind of misleading though.