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

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

[Qais Yousef]

Changes since v3:
	- Fix Sashiko report about force_idle paths where
	  rq->nr_running is not 0 highlighted by Christian
	- Update patch 1 commit message to reflect jump_label call
	  sites on the same CPU might not be updated atomically too

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 a2035d66270b9..14f9f1e419e02 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 (uclamp_rq_is_idle(rq))
			uclamp_rq_clear_idle(rq);
	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 84301fc49e699..dbc3f5e1a7e01 100644
	--- a/kernel/sched/idle.c
	+++ b/kernel/sched/idle.c
	@@ -519,7 +519,7 @@ static void set_next_task_idle(struct rq *rq, struct task_struct *next, enum snt
		 * are still queued.
		 */
		if (uclamp_is_used() &&
	-           !rq->nr_running && unlikely(!uclamp_rq_is_idle(rq)))
	+           !rq->nr_running && unlikely(WARN_ON(!uclamp_rq_is_idle(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  | 19 +++++++++++++++++++
 kernel/sched/sched.h | 13 +++++++++++++
 3 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH v4 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.  Or
across callsites on the same CPUs. Which means when sched_uclamp_used is
enabled, some CPUs/callsites will return true for uclamp_is_used() while
others won't, leading to asymmetric uclamp_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 reported case seems to happen due to callsites on the same CPU not
seeing the flipped state.

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  | 19 +++++++++++++++++++
 kernel/sched/sched.h |  3 +++
 3 files changed, 23 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..84301fc49e699 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -502,6 +502,25 @@ 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.
+	 *
+	 * We need to be careful of some paths that can force idle with tasks
+	 * are still queued.
+	 */
+	if (uclamp_is_used() &&
+	    !rq->nr_running && 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 v4 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 v4 1/2] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq

[Christian Loehle]

On 10/29/26 13:13, Qais Yousef wrote:
> Static keys are not guaranteed to be enabled atomically across CPUs.  Or
> across callsites on the same CPUs. Which means when sched_uclamp_used is
> enabled, some CPUs/callsites will return true for uclamp_is_used() while
> others won't, leading to asymmetric uclamp_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 reported case seems to happen due to callsites on the same CPU not
> seeing the flipped state.
> 
> 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  | 19 +++++++++++++++++++
>  kernel/sched/sched.h |  3 +++
>  3 files changed, 23 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..84301fc49e699 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -502,6 +502,25 @@ 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.
> +	 *
> +	 * We need to be careful of some paths that can force idle with tasks
> +	 * are still queued.
> +	 */
> +	if (uclamp_is_used() &&
> +	    !rq->nr_running && unlikely(!uclamp_rq_is_idle(rq)))

just checking nr_running isn't enough as CFS bandwidth throttling (a fun one
recently!) will decrement it through dequeue_task_fair() without calling
uclamp_rq_dec(), i.e. it's clamps must remain valid AFAICS.

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

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

[Qais Yousef]

On 10/29/26 15:58, Christian Loehle wrote:

>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>> index 76f3c84ca6846..84301fc49e699 100644
>> --- a/kernel/sched/idle.c
>> +++ b/kernel/sched/idle.c
>> @@ -502,6 +502,25 @@ 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.
>> +	 *
>> +	 * We need to be careful of some paths that can force idle with tasks
>> +	 * are still queued.
>> +	 */
>> +	if (uclamp_is_used() &&
>> +	    !rq->nr_running && unlikely(!uclamp_rq_is_idle(rq)))
> 
> just checking nr_running isn't enough as CFS bandwidth throttling (a fun one
> recently!) will decrement it through dequeue_task_fair() without calling
> uclamp_rq_dec(), i.e. it's clamps must remain valid AFAICS.

I think this and the rt group throttling are false positives. Do they 
actually force idle or just under contention dequeue the task 
temporarily to allow other queued tasks to run?

If this is the last running task and throttling can force it off the 
queue partially then enter idle, I don't think the bug is here. It 
should do a full dequeue then. But I am not awfully familiar with this 
code, so could be missing something.

For context, below is Sashiko's full report. Proxy and core scheduling 
seems to cause temporarily context switch to idle. No idea about 
sched_ext. 3 and 4 seem false positive to me.

--->8---

Can calling init_uclamp_rq(rq) in set_next_task_idle() corrupt rq->uclamp
when the runqueue schedules the idle task while uclamp-active tasks are 
still
queued on rq?
In kernel/sched/core.c, init_uclamp_rq() zeroes all bucket->tasks counters
and resets rq->uclamp[clamp_id].value without clearing
p->uclamp[clamp_id].active on tasks that remain on rq:
kernel/sched/core.c:init_uclamp_rq() {
	for_each_clamp_id(clamp_id) {
		uc_rq[clamp_id] = (struct uclamp_rq) {
			.value = uclamp_none(clamp_id)
		};
	}
	rq->uclamp_flags = UCLAMP_FLAG_IDLE;
}
When those still-queued tasks are later dequeued, uclamp_rq_dec_id() in
kernel/sched/core.c sees uc_se->active is still true while bucket->tasks was
zeroed (or decrements another task's bucket reference count):
kernel/sched/core.c:uclamp_rq_dec_id() {
	if (unlikely(!uc_se->active))
		return;
	bucket = &uc_rq->bucket[uc_se->bucket_id];
	WARN_ON_ONCE(!bucket->tasks);
	if (likely(bucket->tasks))
		bucket->tasks--;
}
There appear to be several normal scheduling paths that reach
set_next_task_idle() while tasks remain queued on rq with
p->uclamp[clamp_id].active set and UCLAMP_FLAG_IDLE cleared:
1. With proxy execution enabled, proxy_deactivate() and proxy_migrate_task()
in kernel/sched/core.c call proxy_resched_idle(rq) ->
put_prev_set_next_task(rq, rq->donor, rq->idle) -> set_next_task_idle()
immediately before calling block_task() or deactivate_task():
kernel/sched/core.c:proxy_deactivate() {
	proxy_resched_idle(rq);
	block_task(rq, donor, state);
}
Here set_next_task_idle() zeroes bucket->tasks via init_uclamp_rq(rq), 
and on
the very next line block_task() -> dequeue_task() -> uclamp_rq_dec() ->
uclamp_rq_dec_id() triggers WARN_ON_ONCE(!bucket->tasks). Similarly,
find_proxy_task() calls proxy_resched_idle(rq) when a mutex-blocked 
donor and
other runnable tasks remain queued on rq.
2. With core scheduling enabled, pick_next_task() in kernel/sched/core.c can
force-idle an SMT sibling rq_i that still has runnable tasks queued when no
task matches the core cookie:
kernel/sched/core.c:pick_next_task() {
		if (!cookie_equals(p, cookie)) {
			p = NULL;
			if (cookie)
				p = sched_core_find(rq_i, cookie);
			if (!p)
				p = idle_sched_class.pick_task(rq_i, rf);
		}
}
3. With CFS bandwidth throttling enabled, throttle_cfs_rq_work() in
kernel/sched/fair.c dequeues tasks via dequeue_task_fair() directly without
going through dequeue_task() and uclamp_rq_dec():
kernel/sched/fair.c:throttle_cfs_rq_work() {
		dequeue_task_fair(rq, p, DEQUEUE_SLEEP | DEQUEUE_THROTTLE);
		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
}
4. With RT group bandwidth throttling enabled, 
sched_rt_runtime_exceeded() in
kernel/sched/rt.c dequeues the throttled rt_rq via sched_rt_rq_dequeue() ->
dequeue_top_rt_rq() without calling dequeue_task() and uclamp_rq_dec() 
on the
queued RT tasks:
kernel/sched/rt.c:sched_rt_runtime_exceeded() {
		if (rt_rq_throttled(rt_rq)) {
			sched_rt_rq_dequeue(rt_rq);
			return 1;
		}
}
5. With sched_ext enabled (where ext_sched_class sets uclamp_enabled),
do_pick_task_scx() in kernel/sched/ext/ext.c returns NULL when the local DSQ
is empty even if tasks enqueued on rq reside in a non-local DSQ or BPF
custody:
kernel/sched/ext/ext.c:do_pick_task_scx() {
		p = first_local_task(rq);
		if (!p)
			return NULL;
}
When any of these paths causes rq to schedule rq->idle while tasks with
p->uclamp[clamp_id].active remain on rq, wouldn't init_uclamp_rq(rq) wipe
active frequency clamps and corrupt the bucket task counts?