Re: [PATCH v4 1/2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Qais Yousef]

On 10/30/26 14:13, Peter Zijlstra wrote:
> On Fri, Oct 30, 2026 at 01:21:30PM +0000, Christian Loehle wrote:
>> On 10/30/26 12:41, Qais Yousef wrote:
>>> On 10/30/26 13:15, Peter Zijlstra wrote:
>>>> On Fri, Oct 30, 2026 at 11:28:25AM +0000, Qais Yousef wrote:
>>>>
>>>>> Peter/Vincent, what do you think? It seems we have a schrodenger task that is
>>>>> both enqueued and dequeued simultaneously in these throttling paths. Shouldn't
>>>>> this be fixed? If yes, any thoughts on the right approach? Should we throttle
>>>>> if there are no other tasks on the rq too? I didn't realize the semantics are
>>>>> that strong for enforcing the quota.
>>>>
>>>> Urgh, nothing is ever straight forward is it ...
>>>
>>> Exactly my thoughts..
>>>
>>>>
>>>> Bah, so this is because the core does uclamp (for classes that have it
>>>> on) but then the class does the throttle without going through the core
>>>> {en,de}queue.
>>>>
>>>> Does it make sense to have throttle_Cfs_rq_work() and tg_unthrottle_up()
>>>> do uclamp_rq_{dec,inc}() respectively, along with the
>>>> {de,en}queue_task_fair() they're already doing?
>>>
>>> Yes it does, just wanted to make sure we agree the bug is there rather than in
>>> the current patchset approach.
>>>
>>> I remember seeing a patch to remove the uclamp_enabled bit and defer to
>>> enqueue/dequeue_task_$class to handle this instead. Maybe I can revive it if it
>>> makes more sense to go that way.
>>>
>>> Let me find the path of least resistance and add it to the stack. Not sure we
>>> want to tag as Fixes for LTS to pick up this independent fix. Note we have rt
>>> group throttling path flagged up - which I also thought was false positive but
>>> not sure anymore - but it seems to do partial dequeue and must be fixed in that
>>> regard anyway even if it doesn't force idle.
>>
>> I can reproduce the idle path without uclamp_rq_dec through sched_rt_rq_dequeue(),>> too, so IMO that needs fixing up as well.
>
> Right, every class that has uclamp_enabled needs to be audited -- this
> seems to include sched_ext.

It would be in theory an easy fix to pair uclamp_rq_{inc, dec}() with
{add, sub}_nr_running(); this should ensure the moment we remove or add is the
moment we inc/dec uclamp too.

There's a mess in RT case though that will require partial revert of
f4ebcbc0d7e00 ("sched/rt: Substract number of tasks of throttled queues from
rq->nr_running"); we should simply do {add, sub}_nr_running() in
{inc, dec}_rt_tasks() without the extra complexity. Also sched_rt_rq_dequeue()
calling dequeue_top_rt_rq() doesn't make sense when RT group isn't compiled in,
the function is a NOP (dead code) in this case.

I am not sure though how to handle when rt_se is a group not a task in
{inc, dec}_rt_tasks(). Should I do something like this?

@@ -1182,6 +1131,9 @@ void inc_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq
 *rt_rq)

        inc_rt_prio(rt_rq, prio);
        inc_rt_group(rt_se, rt_rq);
+
+       if (!group_rt_rq(rt_se))
+               add_nr_running(rq_of_rt_rq(rt_rq), 1);
 }

 static inline
@@ -1194,6 +1146,9 @@ void dec_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq
 *rt_rq)

        dec_rt_prio(rt_rq, rt_se_prio(rt_se));
        dec_rt_group(rt_se, rt_rq);
+
+       if (!group_rt_rq(rt_se))
+               sub_nr_running(rq_of_rt_rq(rt_rq), 1);
 }

I think this should make sure we keep rq->nr_runninng updated correctly. I can
then easily pair uclamp inc/dec for all classes.

I am not sure also I can come up with LTS friendly solution now. We either
have to live with the existing warning in uclamp_rq_dec_id() if active is set
but buctkets is empty for LTS since we should handle it fine anyway. Or drop the
fixes tag and let folks who want it backport. The former seems the less evil of
the two to me.

I have done the work above on this branch and gave it some verification, if you
can give it a quick go Christian to confirm the warnings disappear that'd be
appreciated. I just need to make sure I am handling group rt correctly and would
be ready to post it.

	git clone https://github.com/qais-yousef/linux -b uclamp-idle-fix