Uclamp races due to non-atomic static_branch_likely updates

Uclamp races due to non-atomic static_branch_likely updates

[John Stultz]

Hey all,
  So Wei Wang raised an interesting issue recently: He was seeing a
race where when uclamp is enabled (from calling
sched_uclamp_enable()), the static_branch_enable() call will go
through updating the static_branch_likely(&sched_uclamp_used) sites
where the inlined uclamp_is_used() is checked.

The problem being, while the jump label sites are being modified on
one cpu, another cpu might be executing some of these functions and
get inconsistent results.

Specifically in the case where uclamp_rq_inc() on a task is called
followed by uclamp_rq_dec(), its possible that a
static_branch_enable() call modifies the inlined uclamp_is_used()
check in uclamp_rq_inc() first, then on another cpu, uclamp_rq_inc()
is called, sees uclamp_is_used() is true, and increments the buckets.
Then uclamp_rq_dec() is called, but since we're still racing against
the jump table updates, the inlined uclamp_is_used() in that function
returns false and it doesn't do the decrements, leaving things
imbalanced.

Most of the usage I've seen around static_branch/keys treat them as a
magic-compiler global boolean, so to me it seemed this sublte
non-atomic updates could be more widely problematic then just the
uclamp case.

That said, I'm a bit confused, as looking at the jump_label_update()
usage, we already have a key->enabled guard value which can tell us if
the update is in progress. However, despite the logic table in
include/linux/jump_label.h suggesting enabled is used, I don't see it
in the static_branch_[un]likely() -> arch_static_branch[_jump]() logic
(though I recognize the whole point of the static branch logic is to
avoid the conditional, so adding a guard check would undo the benefit
here).

Maybe should the jump_label_update() logic be using something like
stop_machine? Or should there still be a conditional on key->enabled
in the unlikely case to make the behavior consistent? Or is there just
something about the uclamp usage that is off?

Anyway, Wei has proposed dropping the if (!uclamp_is_used()) check in
uclamp_rq_dec() to ensure we always handle the decrement side to
resolve this, but I wanted to see if there were other thoughts on how
we could still preserve the static branch optimization there.

Thoughts?

thanks
-john

Re: Uclamp races due to non-atomic static_branch_likely updates

[Steven Rostedt]

On Tue, 20 Oct 2026 11:53:47 -0700
John Stultz <jstultz@google.com> wrote:

> Maybe should the jump_label_update() logic be using something like
> stop_machine? Or should there still be a conditional on key->enabled
> in the unlikely case to make the behavior consistent? Or is there just
> something about the uclamp usage that is off?

There's never been any guarantee that all call sites would update
atomically. The first user of jump labels was tracepoints and I really do
not want stop machine involved with them.

Perhaps we could add an api like static_key_enable_atomic() and
static_key_disable_atomic() that would go through a stop_machine() and
update everything that way for those locations that need an atomic view of
the updated jump labels.

-- Steve

Re: Uclamp races due to non-atomic static_branch_likely updates

[Peter Zijlstra]

On Tue, Oct 20, 2026 at 03:29:33PM -0400, Steven Rostedt wrote:
> On Tue, 20 Oct 2026 11:53:47 -0700
> John Stultz <jstultz@google.com> wrote:
> 
> > Maybe should the jump_label_update() logic be using something like
> > stop_machine? Or should there still be a conditional on key->enabled
> > in the unlikely case to make the behavior consistent? Or is there just
> > something about the uclamp usage that is off?
> 
> There's never been any guarantee that all call sites would update
> atomically. The first user of jump labels was tracepoints and I really do
> not want stop machine involved with them.
> 
> Perhaps we could add an api like static_key_enable_atomic() and
> static_key_disable_atomic() that would go through a stop_machine() and
> update everything that way for those locations that need an atomic view of
> the updated jump labels.

Argh, no, we do not want people to 'accidentally' add stop_machine()
usage, which we then have to spend tons of time on removing again.

And yeah, its always been the case that jump_labels (and static_branch)
updates are non-atomic. Most of them are init time, and don't care.

Now, I'm sure we've ran into this particular problem before, and it
always requires care.

Also note that a simple global boolean has the exact same problem.
Adding runtime conditionals isn't going to fix this.

Re: Uclamp races due to non-atomic static_branch_likely updates

[Qais Yousef]

On 10/21/26 13:38, Peter Zijlstra wrote:
> On Tue, Oct 20, 2026 at 03:29:33PM -0400, Steven Rostedt wrote:
> > On Tue, 20 Oct 2026 11:53:47 -0700
> > John Stultz <jstultz@google.com> wrote:
> > 
> > > Maybe should the jump_label_update() logic be using something like
> > > stop_machine? Or should there still be a conditional on key->enabled
> > > in the unlikely case to make the behavior consistent? Or is there just
> > > something about the uclamp usage that is off?
> > 
> > There's never been any guarantee that all call sites would update
> > atomically. The first user of jump labels was tracepoints and I really do
> > not want stop machine involved with them.
> > 
> > Perhaps we could add an api like static_key_enable_atomic() and
> > static_key_disable_atomic() that would go through a stop_machine() and
> > update everything that way for those locations that need an atomic view of
> > the updated jump labels.
> 
> Argh, no, we do not want people to 'accidentally' add stop_machine()
> usage, which we then have to spend tons of time on removing again.
> 
> And yeah, its always been the case that jump_labels (and static_branch)
> updates are non-atomic. Most of them are init time, and don't care.
> 
> Now, I'm sure we've ran into this particular problem before, and it
> always requires care.

My memory is hazy but I remember auditing the code that the non synchronized
enable is fine. A cpu has to see the enable and increment, and another cpu has
to dequeue without a decrement for this to be a problem IIUC - seems a pretty
tight race window and the task has to be super short running for it to be
dequeued before the static key was flipped on the remote cpu.. I wonder if this
is a symptom of something else going wrong..

Decrement without increment should be safe - we'll just bail out and we have
a comment documenting this.

> 
> Also note that a simple global boolean has the exact same problem.
> Adding runtime conditionals isn't going to fix this.

I have something in my memory that could be complete garbage that tells me all
cpus should see the new update after a grace period, did my brain invent this?

If not invented, can we do synchronize_rcu() once on any cpu seeing the code
enabled for the first time to ensure the rest of the cpus has seen the newly
enabled path?

eg (not even compile tested):

	diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
	index 7d2ec527b8a2..a6093ec88103 100644
	--- a/kernel/sched/sched.h
	+++ b/kernel/sched/sched.h
	@@ -3715,7 +3715,16 @@ unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
	  */
	 static inline bool uclamp_is_used(void)
	 {
	-       return static_branch_likely(&sched_uclamp_used);
	+       if (static_branch_likely(&sched_uclamp_used)) {
	+               static bool do_once = true;
	+               if (unlikely(do_once)) {
	+                       do_once = false;
	+                       synchronize_rcu();
	+               }
	+               return true;
	+       }
	+
	+       return false;
	 }

	 /*

The other alternative is to just accept this race can happen and do some sanity
check and reset/cleanup on going to idle - which should be pretty cheap and
straightforward.

Re: Uclamp races due to non-atomic static_branch_likely updates

[Peter Zijlstra]

On Wed, Oct 21, 2026 at 02:20:58PM +0100, Qais Yousef wrote:

> > Also note that a simple global boolean has the exact same problem.
> > Adding runtime conditionals isn't going to fix this.
> 
> I have something in my memory that could be complete garbage that tells me all
> cpus should see the new update after a grace period, did my brain invent this?

This is true; rcu_sync() implies a memory barrier on all CPUs, and
therefore all CPUs must observe the prior state upon completion.

The thing is that a grace period is of 'arbitrary length' and during
that you can still have some CPUs seeing your variable set while others
will not and you get into the very same problem.

> If not invented, can we do synchronize_rcu() once on any cpu seeing the code
> enabled for the first time to ensure the rest of the cpus has seen the newly
> enabled path?
> 
> eg (not even compile tested):
> 
> 	diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> 	index 7d2ec527b8a2..a6093ec88103 100644
> 	--- a/kernel/sched/sched.h
> 	+++ b/kernel/sched/sched.h
> 	@@ -3715,7 +3715,16 @@ unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
> 	  */
> 	 static inline bool uclamp_is_used(void)
> 	 {
> 	-       return static_branch_likely(&sched_uclamp_used);
> 	+       if (static_branch_likely(&sched_uclamp_used)) {
> 	+               static bool do_once = true;
> 	+               if (unlikely(do_once)) {
> 	+                       do_once = false;
> 	+                       synchronize_rcu();
> 	+               }
> 	+               return true;
> 	+       }
> 	+
> 	+       return false;
> 	 }
> 
> 	 /*

This cannot work; two immediate problems:

 - the do_once needs re-evaluating for every switch
 - you cannot do rcu_sync while holding rq->lock ;-)

> The other alternative is to just accept this race can happen and do some sanity
> check and reset/cleanup on going to idle - which should be pretty cheap and
> straightforward.

This seems the sanest option.

Re: Uclamp races due to non-atomic static_branch_likely updates

[Qais Yousef]

On 10/21/26 15:29, Peter Zijlstra wrote:
> On Wed, Oct 21, 2026 at 02:20:58PM +0100, Qais Yousef wrote:
> 
> > > Also note that a simple global boolean has the exact same problem.
> > > Adding runtime conditionals isn't going to fix this.
> > 
> > I have something in my memory that could be complete garbage that tells me all
> > cpus should see the new update after a grace period, did my brain invent this?
> 
> This is true; rcu_sync() implies a memory barrier on all CPUs, and
> therefore all CPUs must observe the prior state upon completion.
> 
> The thing is that a grace period is of 'arbitrary length' and during
> that you can still have some CPUs seeing your variable set while others
> will not and you get into the very same problem.

Maybe this arbitrary length what contributes to the size of the race window
otherwise I struggle to see this being a problem practically.

> 
> > If not invented, can we do synchronize_rcu() once on any cpu seeing the code
> > enabled for the first time to ensure the rest of the cpus has seen the newly
> > enabled path?
> > 
> > eg (not even compile tested):
> > 
> > 	diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > 	index 7d2ec527b8a2..a6093ec88103 100644
> > 	--- a/kernel/sched/sched.h
> > 	+++ b/kernel/sched/sched.h
> > 	@@ -3715,7 +3715,16 @@ unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
> > 	  */
> > 	 static inline bool uclamp_is_used(void)
> > 	 {
> > 	-       return static_branch_likely(&sched_uclamp_used);
> > 	+       if (static_branch_likely(&sched_uclamp_used)) {
> > 	+               static bool do_once = true;
> > 	+               if (unlikely(do_once)) {
> > 	+                       do_once = false;
> > 	+                       synchronize_rcu();
> > 	+               }
> > 	+               return true;
> > 	+       }
> > 	+
> > 	+       return false;
> > 	 }
> > 
> > 	 /*
> 
> This cannot work; two immediate problems:
> 
>  - the do_once needs re-evaluating for every switch

I think uclamp can only be turned on, but yeah for a generic solution this is
a problem and the do_once will need to be reset on switch off.

>  - you cannot do rcu_sync while holding rq->lock ;-)

Uh-oh, right :-)

> 
> > The other alternative is to just accept this race can happen and do some sanity
> > check and reset/cleanup on going to idle - which should be pretty cheap and
> > straightforward.
> 
> This seems the sanest option.

I think the below should do

-->8--

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d7c3b43158de..c6780358a797 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 76f3c84ca684..64ea91b743bb 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -502,6 +502,14 @@ 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.
+        */
+       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 7d2ec527b8a2..6d7be3e3b36b 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;
 }

+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: Uclamp races due to non-atomic static_branch_likely updates

[Steven Rostedt]

On Wed, 21 Oct 2026 13:38:06 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Oct 20, 2026 at 03:29:33PM -0400, Steven Rostedt wrote:
> > On Tue, 20 Oct 2026 11:53:47 -0700
> > John Stultz <jstultz@google.com> wrote:
> >   
> > > Maybe should the jump_label_update() logic be using something like
> > > stop_machine? Or should there still be a conditional on key->enabled
> > > in the unlikely case to make the behavior consistent? Or is there just
> > > something about the uclamp usage that is off?  
> > 
> > There's never been any guarantee that all call sites would update
> > atomically. The first user of jump labels was tracepoints and I really do
> > not want stop machine involved with them.
> > 
> > Perhaps we could add an api like static_key_enable_atomic() and
> > static_key_disable_atomic() that would go through a stop_machine() and
> > update everything that way for those locations that need an atomic view of
> > the updated jump labels.  
> 
> Argh, no, we do not want people to 'accidentally' add stop_machine()
> usage, which we then have to spend tons of time on removing again.

OK, sure. I was a bit skeptical of that approach too.

> 
> And yeah, its always been the case that jump_labels (and static_branch)
> updates are non-atomic. Most of them are init time, and don't care.
> 
> Now, I'm sure we've ran into this particular problem before, and it
> always requires care.
> 
> Also note that a simple global boolean has the exact same problem.
> Adding runtime conditionals isn't going to fix this.

Another idea for "atomic static branch" is to have two static branches at
each location. One that switches to a "check this variable and do a normal
test and branch" and "we are now in sync and can use the original static
branch".

Something like:

	if (static_branch_unlikely(&foo_use_variable.key)) {
		return static_key_enabled(&foo->key);
	} else {
		if (static_branch_unlikely(&foo->key))
			return true;
		else
			return false;
	}

And then in between updates of foo, the foo_use_variable gets set making
all the branches use the key, which will be updated "atomically" (maybe
some memory barriers needs to be added). After the updates of all the
static keys are complete, the foo_use_variable gets unset.

The non-atomic portion of "foo_use_variable" is OK because the foo->key
will match whatever the static_branch version it has.

The end result avoids the test and branch but will have two nop/jmp pairs.

-- Steve

Re: Uclamp races due to non-atomic static_branch_likely updates

[Peter Zijlstra]

On Wed, Oct 21, 2026 at 09:21:36AM -0400, Steven Rostedt wrote:

> > And yeah, its always been the case that jump_labels (and static_branch)
> > updates are non-atomic. Most of them are init time, and don't care.
> > 
> > Now, I'm sure we've ran into this particular problem before, and it
> > always requires care.
> > 


Read this part:

> > Also note that a simple global boolean has the exact same problem.
> > Adding runtime conditionals isn't going to fix this.

again, and then consider your response..

> 
> Another idea for "atomic static branch" is to have two static branches at
> each location. One that switches to a "check this variable and do a normal
> test and branch" and "we are now in sync and can use the original static
> branch".
> 
> Something like:
> 
> 	if (static_branch_unlikely(&foo_use_variable.key)) {
> 		return static_key_enabled(&foo->key);
> 	} else {
> 		if (static_branch_unlikely(&foo->key))
> 			return true;
> 		else
> 			return false;
> 	}
> 
> And then in between updates of foo, the foo_use_variable gets set making
> all the branches use the key, which will be updated "atomically" (maybe
> some memory barriers needs to be added). After the updates of all the
> static keys are complete, the foo_use_variable gets unset.
> 
> The non-atomic portion of "foo_use_variable" is OK because the foo->key
> will match whatever the static_branch version it has.

That simply doesn't work.

Even *IF* memory stores would propagate instantly (they don't) you can
still have the boolean swap while in the middle of this state.

---

B = true;

CPU0			CPU1

 enqueue
   if (B)
     something++;

     			B = false;

 dequeue
   if (B)
     something--;


---

Will still be left with something unbalanced, even if everything is
super duper magically atomic.

Re: Uclamp races due to non-atomic static_branch_likely updates

[Steven Rostedt]

On Wed, 21 Oct 2026 15:34:30 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> That simply doesn't work.
> 
> Even *IF* memory stores would propagate instantly (they don't) you can
> still have the boolean swap while in the middle of this state.
> 
> ---
> 
> B = true;
> 
> CPU0			CPU1
> 
>  enqueue
>    if (B)
>      something++;
> 
>      			B = false;
> 
>  dequeue
>    if (B)
>      something--;
> 
> 
> ---
> 
> Will still be left with something unbalanced, even if everything is
> super duper magically atomic.

Fair point.

-- Steve