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@xxxxxxxxxx> 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.