[PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Qais Yousef]

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>
---

Changes since v1:
	- Fix kernel test robot report due to missing static inline in dummy
	  definition of init_uclamp_rq()

Tested with the patch below to force imbalance via sched_feat. I could see the
warning appearing and frequencies spiking during imbalance. Correct behavior is
restored when sched_feat is disabled where the idle entry reset fixes things
up.

I had a mix of uclamp_max mainly used in the system via schedqosd.

	diff --git a/kernel/sched/core.c b/kernel/sched/core.c
	index c6780358a7975..d3e73ba88f22b 100644
	--- a/kernel/sched/core.c
	+++ b/kernel/sched/core.c
	@@ -1889,6 +1889,11 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags
		for_each_clamp_id(clamp_id)
			uclamp_rq_inc_id(rq, p, clamp_id);
	 
	+       if (sched_feat(UCLAMP_IMBALANCED)) {
	+               for_each_clamp_id(clamp_id)
	+                       uclamp_rq_inc_id(rq, p, clamp_id);
	+       }
	+
		/* Reset clamp idle holding when there is one RUNNABLE task */
		if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
			rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
	diff --git a/kernel/sched/features.h b/kernel/sched/features.h
	index 8f0dee8fc4755..b59e532500fd9 100644
	--- a/kernel/sched/features.h
	+++ b/kernel/sched/features.h
	@@ -142,3 +142,5 @@ SCHED_FEAT(LATENCY_WARN, false)
	  */
	 SCHED_FEAT(NI_RANDOM, true)
	 SCHED_FEAT(NI_RATE, true)
	+
	+SCHED_FEAT(UCLAMP_IMBALANCED, false)
	diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
	index 950e2885bba63..cb31b3c9df23c 100644
	--- a/kernel/sched/idle.c
	+++ b/kernel/sched/idle.c
	@@ -516,7 +516,7 @@ static void set_next_task_idle(struct rq *rq, struct task_struct *next, enum snt
		 * everything should work as expected.
		*/
		if (uclamp_is_used()) {
	-               if (unlikely(uclamp_is_imbalanced(rq)))
	+               if (unlikely(WARN_ON(uclamp_is_imbalanced(rq))))
				init_uclamp_rq(rq);
		}
	 }

 kernel/sched/core.c  |  2 +-
 kernel/sched/idle.c  | 17 +++++++++++++++++
 kernel/sched/sched.h | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 53 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..950e2885bba63 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_is_imbalanced(rq)))
+			init_uclamp_rq(rq);
+	}
 }
 
 struct task_struct *pick_task_idle(struct rq *rq, struct rq_flags *rf)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7d2ec527b8a25..47dcfd74e150f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3704,6 +3704,7 @@ static inline unsigned long cpu_util_rt(struct rq *rq)
 #ifdef CONFIG_UCLAMP_TASK
 
 unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
+void init_uclamp_rq(struct rq *rq);
 
 /*
  * When uclamp is compiled in, the aggregation at rq level is 'turned off'
@@ -3783,6 +3784,33 @@ static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
 	return min_t(unsigned int, clamp_value / UCLAMP_BUCKET_DELTA, UCLAMP_BUCKETS - 1);
 }
 
+static inline bool __uclamp_is_imbalanced(struct rq *rq, enum uclamp_id clamp_id)
+{
+	struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
+	int bucket_id = UCLAMP_BUCKETS - 1;
+
+	/*
+	 * On idle all bucket.tasks must be 0, or we have hit imbalance
+	 * problem.
+	 */
+	for ( ; bucket_id >= 0; bucket_id--)
+		if (bucket[bucket_id].tasks)
+			return true;
+
+	return false;
+}
+
+static inline bool uclamp_is_imbalanced(struct rq *rq)
+{
+	enum uclamp_id clamp_id;
+
+	for_each_clamp_id(clamp_id)
+		if(__uclamp_is_imbalanced(rq, clamp_id))
+			return true;
+
+	return false;
+}
+
 static inline void
 uclamp_se_set(struct uclamp_se *uc_se, unsigned int value, bool user_defined)
 {
@@ -3802,6 +3830,8 @@ uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id)
 	return SCHED_CAPACITY_SCALE;
 }
 
+static inline void init_uclamp_rq(struct rq *rq) {}
+
 static inline bool uclamp_rq_is_capped(struct rq *rq) { return false; }
 
 static inline bool uclamp_is_used(void)
@@ -3830,6 +3860,11 @@ static inline bool uclamp_rq_is_idle(struct rq *rq)
 	return false;
 }
 
+static inline bool uclamp_is_imbalanced(struct rq *rq)
+{
+	return false;
+}
+
 #endif /* !CONFIG_UCLAMP_TASK */
 
 #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
-- 
2.53.0

Re: [PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Peter Zijlstra]

On Sun, Oct 25, 2026 at 04:26:12PM +0000, Qais Yousef wrote:

> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 76f3c84ca6846..950e2885bba63 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.
> +	*/

Whitespace fail here.

> +	if (uclamp_is_used()) {
> +		if (unlikely(uclamp_is_imbalanced(rq)))
> +			init_uclamp_rq(rq);
> +	}
>  }
>  
>  struct task_struct *pick_task_idle(struct rq *rq, struct rq_flags *rf)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 7d2ec527b8a25..47dcfd74e150f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3704,6 +3704,7 @@ static inline unsigned long cpu_util_rt(struct rq *rq)
>  #ifdef CONFIG_UCLAMP_TASK
>  
>  unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
> +void init_uclamp_rq(struct rq *rq);
>  
>  /*
>   * When uclamp is compiled in, the aggregation at rq level is 'turned off'
> @@ -3783,6 +3784,33 @@ static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
>  	return min_t(unsigned int, clamp_value / UCLAMP_BUCKET_DELTA, UCLAMP_BUCKETS - 1);
>  }
>  
> +static inline bool __uclamp_is_imbalanced(struct rq *rq, enum uclamp_id clamp_id)
> +{
> +	struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> +	int bucket_id = UCLAMP_BUCKETS - 1;
> +
> +	/*
> +	 * On idle all bucket.tasks must be 0, or we have hit imbalance
> +	 * problem.
> +	 */
> +	for ( ; bucket_id >= 0; bucket_id--)
> +		if (bucket[bucket_id].tasks)
> +			return true;
> +
> +	return false;
> +}
> +
> +static inline bool uclamp_is_imbalanced(struct rq *rq)
> +{
> +	enum uclamp_id clamp_id;
> +
> +	for_each_clamp_id(clamp_id)
> +		if(__uclamp_is_imbalanced(rq, clamp_id))
> +			return true;
> +
> +	return false;
> +}

Anyway; I was thinking, since we have a hole in struct uclamp_rq; and
also uclamp_flags. Can we fold uclamp_flags into the hold, and also use
UCLAMP_FLAG_IDLE to detect if we need this bucket scan at all?

Re: [PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Qais Yousef]

On 10/26/26 10:12, Peter Zijlstra wrote:

>> +static inline bool __uclamp_is_imbalanced(struct rq *rq, enum uclamp_id clamp_id)
>> +{
>> +	struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
>> +	int bucket_id = UCLAMP_BUCKETS - 1;
>> +
>> +	/*
>> +	 * On idle all bucket.tasks must be 0, or we have hit imbalance
>> +	 * problem.
>> +	 */
>> +	for ( ; bucket_id >= 0; bucket_id--)
>> +		if (bucket[bucket_id].tasks)
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>> +static inline bool uclamp_is_imbalanced(struct rq *rq)
>> +{
>> +	enum uclamp_id clamp_id;
>> +
>> +	for_each_clamp_id(clamp_id)
>> +		if(__uclamp_is_imbalanced(rq, clamp_id))
>> +			return true;
>> +
>> +	return false;
>> +}
> 
> Anyway; I was thinking, since we have a hole in struct uclamp_rq; and
> also uclamp_flags. Can we fold uclamp_flags into the hold, and also use
> UCLAMP_FLAG_IDLE to detect if we need this bucket scan at all?

Actually maybe we can just check if the flag is not set then this means 
we have a corruption without scanning the buckets. I didn't like the 
scan, but seemed safer for a backport. Let me try that and update.

I can fold the uclamp_flags into buckets, I don't think this will grow 
and I do have a path to help us get rid off uclamp_rq (hopefully) in the 
future.

Okay to do this as a separate follow up patch? Since this will go to 
LTS, I think safer keep it minimalist and do the folding bit in a 
separate patch.

Re: [PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Peter Zijlstra]

On Mon, Oct 26, 2026 at 11:24:31AM +0000, Qais Yousef wrote:
> On 10/26/26 10:12, Peter Zijlstra wrote:
> 
> > > +static inline bool __uclamp_is_imbalanced(struct rq *rq, enum uclamp_id clamp_id)
> > > +{
> > > +	struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > > +	int bucket_id = UCLAMP_BUCKETS - 1;
> > > +
> > > +	/*
> > > +	 * On idle all bucket.tasks must be 0, or we have hit imbalance
> > > +	 * problem.
> > > +	 */
> > > +	for ( ; bucket_id >= 0; bucket_id--)
> > > +		if (bucket[bucket_id].tasks)
> > > +			return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static inline bool uclamp_is_imbalanced(struct rq *rq)
> > > +{
> > > +	enum uclamp_id clamp_id;
> > > +
> > > +	for_each_clamp_id(clamp_id)
> > > +		if(__uclamp_is_imbalanced(rq, clamp_id))
> > > +			return true;
> > > +
> > > +	return false;
> > > +}
> > 
> > Anyway; I was thinking, since we have a hole in struct uclamp_rq; and
> > also uclamp_flags. Can we fold uclamp_flags into the hold, and also use
> > UCLAMP_FLAG_IDLE to detect if we need this bucket scan at all?
> 
> Actually maybe we can just check if the flag is not set then this means we
> have a corruption without scanning the buckets. I didn't like the scan, but
> seemed safer for a backport. Let me try that and update.
> 
> I can fold the uclamp_flags into buckets, I don't think this will grow and I
> do have a path to help us get rid off uclamp_rq (hopefully) in the future.
> 
> Okay to do this as a separate follow up patch? Since this will go to LTS, I
> think safer keep it minimalist and do the folding bit in a separate patch.

Sure; I just noticed these separate things while trying to re acquaint
myself with uclamp ;-)

Re: [PATCH v2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Qais Yousef]

On 10/26/26 11:59, Peter Zijlstra wrote:
> On Mon, Oct 26, 2026 at 11:24:31AM +0000, Qais Yousef wrote:
>> On 10/26/26 10:12, Peter Zijlstra wrote:
>>
>>>> +static inline bool __uclamp_is_imbalanced(struct rq *rq, enum uclamp_id clamp_id)
>>>> +{
>>>> +	struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
>>>> +	int bucket_id = UCLAMP_BUCKETS - 1;
>>>> +
>>>> +	/*
>>>> +	 * On idle all bucket.tasks must be 0, or we have hit imbalance
>>>> +	 * problem.
>>>> +	 */
>>>> +	for ( ; bucket_id >= 0; bucket_id--)
>>>> +		if (bucket[bucket_id].tasks)
>>>> +			return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static inline bool uclamp_is_imbalanced(struct rq *rq)
>>>> +{
>>>> +	enum uclamp_id clamp_id;
>>>> +
>>>> +	for_each_clamp_id(clamp_id)
>>>> +		if(__uclamp_is_imbalanced(rq, clamp_id))
>>>> +			return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>
>>> Anyway; I was thinking, since we have a hole in struct uclamp_rq; and
>>> also uclamp_flags. Can we fold uclamp_flags into the hold, and also use
>>> UCLAMP_FLAG_IDLE to detect if we need this bucket scan at all?
>>
>> Actually maybe we can just check if the flag is not set then this means we
>> have a corruption without scanning the buckets. I didn't like the scan, but
>> seemed safer for a backport. Let me try that and update.
>>
>> I can fold the uclamp_flags into buckets, I don't think this will grow and I
>> do have a path to help us get rid off uclamp_rq (hopefully) in the future.
>>
>> Okay to do this as a separate follow up patch? Since this will go to LTS, I
>> think safer keep it minimalist and do the folding bit in a separate patch.
> 
> Sure; I just noticed these separate things while trying to re acquaint
> myself with uclamp ;-)

I posted new version along with some clean up that makes this conversion 
slightly easier, but didn't go all the way in as the flag being per 
uclamp_id didn't make sense and if I moved it to uclamp_rq or 
uclamp_bucket it meant we'll need to set and clear the two IDs which 
didn't feel much better. We can discuss on the new series.

Turns out no need to scan too and can just check if the flag is not set 
to detect the imbalance.