Re: [PATCH] sched/fair: Prevent cfs_rq from being unthrottled with zero runtime_remaining

[Hao Jia <jiahao.kernel@gmail.com>]

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.

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().

Thanks,
Hao