Re: Uclamp races due to non-atomic static_branch_likely updates

[Christian Loehle]

On 10/21/26 15:02, Qais Yousef wrote:
> 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)

So I guess the clear-when-entering-idle taper is sufficient for UCLAMP_MIN.
UCLAMP_MAX (where on a sufficiently low setting we may just never have that
idle entry to clear) we expect to be fine because it's max-aggregated, i.e.
a second task would let us escape the low stale UCLAMP_MAX on the rq?
I guess we could reevaluate the static branch at every tick too if we really
care?