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

[Christian Loehle <christian.loehle@arm.com>]

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)