[PATCH v3 0/2] Fix uclamp race against non atomic static key enable

[PATCH v3 0/2] Fix uclamp race against non atomic static key enable

[Qais Yousef]

Changes since v2:
	- Use uclamp_rq_is_idle() instead of scanning buckets to detect
	  imbalance
	- Do some cleanup on accessing the flag as an additional patch

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);
		}
	 }

Qais Yousef (2):
  sched/uclamp: Fix race condition leading to imbalanced uclamp_rq
  sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE

 kernel/sched/core.c  | 16 ++++++++--------
 kernel/sched/idle.c  | 17 +++++++++++++++++
 kernel/sched/sched.h | 13 +++++++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH v3 1/2] 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>
---
 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);
+	}
 }
 
 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..d87bdbd2121df 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'
@@ -3802,6 +3803,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)
-- 
2.53.0

[PATCH v3 2/2] sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE

[Qais Yousef]

We already have uclamp_rq_is_idle(), but it wasn't used everywhere.
Replace open coding with the function and add new wrapper to set and
clear the flag which enhances readability.

Signed-off-by: Qais Yousef <qyousef@layalina.io>
---
 kernel/sched/core.c  | 14 +++++++-------
 kernel/sched/sched.h | 10 ++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c6780358a7975..a2035d66270b9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1626,7 +1626,7 @@ uclamp_idle_value(struct rq *rq, enum uclamp_id clamp_id,
 	 * max-clamp.
 	 */
 	if (clamp_id == UCLAMP_MAX) {
-		rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
+		uclamp_rq_set_idle(rq);
 		return clamp_value;
 	}
 
@@ -1637,7 +1637,7 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
 				     unsigned int clamp_value)
 {
 	/* Reset max-clamp retention only on idle exit */
-	if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
+	if (!uclamp_rq_is_idle(rq))
 		return;
 
 	uclamp_rq_set(rq, clamp_id, clamp_value);
@@ -1890,8 +1890,8 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags
 		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;
+	if (uclamp_rq_is_idle(rq))
+		uclamp_rq_clear_idle(rq);
 }
 
 static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
@@ -1930,8 +1930,8 @@ static inline void uclamp_rq_reinc_id(struct rq *rq, struct task_struct *p,
 	 * Make sure to clear the idle flag if we've transiently reached 0
 	 * active tasks on rq.
 	 */
-	if (clamp_id == UCLAMP_MAX && (rq->uclamp_flags & UCLAMP_FLAG_IDLE))
-		rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
+	if (clamp_id == UCLAMP_MAX && uclamp_rq_is_idle(rq))
+		uclamp_rq_clear_idle(rq);
 }
 
 static inline void
@@ -2122,7 +2122,7 @@ void init_uclamp_rq(struct rq *rq)
 		};
 	}
 
-	rq->uclamp_flags = UCLAMP_FLAG_IDLE;
+	uclamp_rq_set_idle(rq);
 }
 
 static void __init init_uclamp(void)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index d87bdbd2121df..4706f73838a86 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3743,6 +3743,16 @@ static inline void uclamp_rq_set(struct rq *rq, enum uclamp_id clamp_id,
 	WRITE_ONCE(rq->uclamp[clamp_id].value, value);
 }
 
+static inline void uclamp_rq_set_idle(struct rq *rq)
+{
+	rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
+}
+
+static inline void uclamp_rq_clear_idle(struct rq *rq)
+{
+	rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
+}
+
 static inline bool uclamp_rq_is_idle(struct rq *rq)
 {
 	return rq->uclamp_flags & UCLAMP_FLAG_IDLE;
-- 
2.53.0

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

[Peter Zijlstra]

On Wed, Oct 28, 2026 at 02:05:11AM +0000, Qais Yousef wrote:

> --- 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);
> +	}
>  }

Much nicer! Would you mind terribly if I make that:

	if (unlikely(uclamp_is_used() && !uclamp_rq_is_idle(rq)))
		init_uclamp_rq(rq);

?

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

[Qais Yousef]

On October 28, 2026 10:53:36 AM UTC, Peter Zijlstra <peterz@infradead.org> wrote:
>On Wed, Oct 28, 2026 at 02:05:11AM +0000, Qais Yousef wrote:
>
>> --- 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);
>> +	}
>>  }
>
>Much nicer! Would you mind terribly if I make that:
>
>	if (unlikely(uclamp_is_used() && !uclamp_rq_is_idle(rq)))
>		init_uclamp_rq(rq);
>
>?

Not at all, it just evolved organically :)

Thanks!

--
Qais Yousef

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

[Christian Loehle]

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?

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

[Qais Yousef]

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

--
Qais Yousef

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

[John Stultz]

On Tue, Oct 27, 2026 at 7:05 PM Qais Yousef <qyousef@layalina.io> wrote:
>
> Static keys are not guaranteed to be enabled atomically across CPUs.

Nit: Not only across CPUs, but also across jump_label callsites!  I
believe the enqueue/dequeue race Wei reported happened on the same
cpu.

thanks
-john

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

[Qais Yousef]

On 10/28/26 17:18, John Stultz wrote:
> On Tue, Oct 27, 2026 at 7:05 PM Qais Yousef <qyousef@layalina.io> wrote:
>>
>> Static keys are not guaranteed to be enabled atomically across CPUs.
> 
> Nit: Not only across CPUs, but also across jump_label callsites!  I
> believe the enqueue/dequeue race Wei reported happened on the same
> cpu.
> 
> thanks
> -john

Right, this is news to me. I'll update the text. Thanks!

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

[Qais Yousef]

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!