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

[Qais Yousef <qyousef@layalina.io>]

On 10/28/26 12:09, Qais Yousef wrote:
> On October 28, 2026 11:13:55 AM UTC, Christian Loehle <christian.loehle@arm.com> wrote:
>> On 10/28/26 02:05, Qais Yousef wrote:
>>> Static keys are not guaranteed to be enabled atomically across CPUs.
>>> Which means when sched_uclamp_used is enabled, some CPUs will return
>>> true for uclamp_is_used() while others won't, leading to asymmetric
>>> ulcamp_rq_{inc, dec}_id. If we decrement without increment this is safe
>>> as documented in the code. But an increment without a decrement is
>>> a problem and will leave the uclamp_rq in a corrupt state.
>>>
>>> The race for increment without a decrement is interesting as it means
>>> a CPU has seen the static key flipped and incremented while enqueueing
>>> a task, but the task was dequeued on another CPU that hasn't seen the
>>> static key flipped - we need to be wary this might be a symptom of
>>> something else going wrong.
>>>
>>> Plugging the race is hard as discussed in [1]. Take it for granted and
>>> do correction actions on going to idle to guarantee this state is
>>> transient for a short period after uclamp_sched_used is enabled.
>>>
>>> [1] https://lore.kernel.org/lkml/CANDhNCqyXsZ6G77z+9s=rTO6BZifRG9+hVxMTah+tAv6pB6h3w@mail.gmail.com/
>>>
>>> Reported-by: Wei Wang <wvw@google.com>
>>> Fixes: 46609ce227039 ("sched/uclamp: Protect uclamp fast path code with static key")
>>> Signed-off-by: Qais Yousef <qyousef@layalina.io>
>>> ---
>>>   kernel/sched/core.c  |  2 +-
>>>   kernel/sched/idle.c  | 17 +++++++++++++++++
>>>   kernel/sched/sched.h |  3 +++
>>>   3 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index d7c3b43158dea..c6780358a7975 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -2111,7 +2111,7 @@ static void uclamp_post_fork(struct task_struct *p)
>>>   	uclamp_update_util_min_rt_default(p);
>>>   }
>>>   
>>> -static void __init init_uclamp_rq(struct rq *rq)
>>> +void init_uclamp_rq(struct rq *rq)
>>>   {
>>>   	enum uclamp_id clamp_id;
>>>   	struct uclamp_rq *uc_rq = rq->uclamp;
>>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>>> index 76f3c84ca6846..c722b23e07dc2 100644
>>> --- a/kernel/sched/idle.c
>>> +++ b/kernel/sched/idle.c
>>> @@ -502,6 +502,23 @@ static void set_next_task_idle(struct rq *rq, struct task_struct *next, enum snt
>>>   	 * lost_idle_time of clock_pelt
>>>   	 */
>>>   	update_idle_rq_clock_pelt(rq);
>>> +
>>> +	/*
>>> +	 * Due to a race condition when enabling sched_uclamp_used static key,
>>> +	 * we can end up with imbalance where uclamp_rq_inc_id() has happened
>>> +	 * on a CPU without uclamp_rq_dec_id() on another CPU that hasn't seen
>>> +	 * the static key flipped. The race condition is unlikely, but ensure
>>> +	 * the uclamp_rq() state is set properly on each entry to idle.
>>> +	 *
>>> +	 * The reset will cause the uclamp_max from uclamp_idle_value() to be
>>> +	 * lost. Since we are imbalanced, hard to know what is right. So just
>>> +	 * cleanup. This should be a transient error state once we recover
>>> +	 * everything should work as expected.
>>> +	 */
>>> +	if (uclamp_is_used()) {
>>> +		if (unlikely(!uclamp_rq_is_idle(rq)))
>>> +			init_uclamp_rq(rq);
>>> +	}
>>>   }
>>>   
>>
>>
>> I haven't checked the other cases, but sashiko notes a few issues with this.
>> The sched_ext one and the core-scheduling cases seem legitimate, i.e. the idle
>> task may be scheduled while the rq actually isn't idle and clamps should be
>> maintained?
>>
> 
> I need to check in more details, but we can add rq->nr_running guard. Might need to audit other updates here too since we update clocks and load avgs, which might be fine

It seems we can end up with force_idle paths, yes.. I added a guard for 
rq->nr_running. Thanks!