[PATCH v5 0/4] Fix uclamp race against non atomic static key enable

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH v5 0/4] Fix uclamp race against non atomic static key enable
@ 2026-11-03  0:59 Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 1/4] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq Qais Yousef
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  0:59 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel, Qais Yousef

The latest version contains additional fixes to another related problem
exposed by this series. Fair and RT throttling were partially dequeuing
tasks when they are throttled without updating uclamp accounting. Patch
3 is marked as RFC as it partially reverts an old patch and rework it so
that RT usage of updating rq->nr_running is more consistent with the
other classes. Patch 4 then simply ensures that uclamp accounting is
updated as we update nr_running which is the source of truth when tasks
has moved in or out of the rq.

The series as a whole is no longer suitable for LTS though and I am not
sure it is worth more cycles given the low severity of the problem.
I kept the fixes tag on patch 1, but we can drop it and defer to folks
to pick it up if they are impacted by the race condition. Or keep it,
knowing that there will be a new (fairly harmless) warning appearing due
to the throttling path dequeueing without updating uclamp accounting.

I don't have a preference, but will err on dropping Fixes tag if no
clear voting comes up.

The series was tested with the test case below and I did check RT
nr_running accounting works correctly. But didn't check group RT or fair
throttling cases yet. I'd appreciate help in testing under these
scenarios.

In patch 3 I'd appreciate more eyes on inc/dec_rt_tasks() and removal of
rt_queued and its implication on some of the helper functions.

Changes since v4:
	- Fix problems in fair/RT throttling logic that were exposed by
	  the original fix.

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 (4):
  sched/uclamp: Fix race condition leading to imbalanced uclamp_rq
  sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE
  Revert "sched/rt: Substract number of tasks of throttled queues from
    rq->nr_running"
  sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c

 kernel/sched/core.c    | 49 +++++++++---------------
 kernel/sched/ext/ext.c |  6 +--
 kernel/sched/fair.c    |  6 +--
 kernel/sched/idle.c    | 19 ++++++++++
 kernel/sched/rt.c      | 86 +++++++++---------------------------------
 kernel/sched/sched.h   | 27 +++++++++----
 6 files changed, 76 insertions(+), 117 deletions(-)

-- 
2.53.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 1/4] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq
  2026-11-03  0:59 [PATCH v5 0/4] Fix uclamp race against non atomic static key enable Qais Yousef
@ 2026-11-03  0:59 ` Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 2/4] sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE Qais Yousef
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  0:59 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel, Qais Yousef, Wei Wang

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 2/4] sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE
  2026-11-03  0:59 [PATCH v5 0/4] Fix uclamp race against non atomic static key enable Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 1/4] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq Qais Yousef
@ 2026-11-03  0:59 ` Qais Yousef
  2026-11-03  0:59 ` [PATCH RFC v5 3/4] Revert "sched/rt: Substract number of tasks of throttled queues from rq->nr_running" Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c Qais Yousef
  3 siblings, 0 replies; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  0:59 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel, Qais Yousef, Kayra Cizmeci

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.

Reviewed-by: Kayra Cizmeci <kayracizmeci@gmail.com>
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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH RFC v5 3/4] Revert "sched/rt: Substract number of tasks of throttled queues from rq->nr_running"
  2026-11-03  0:59 [PATCH v5 0/4] Fix uclamp race against non atomic static key enable Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 1/4] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 2/4] sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE Qais Yousef
@ 2026-11-03  0:59 ` Qais Yousef
  2026-11-03  0:59 ` [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c Qais Yousef
  3 siblings, 0 replies; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  0:59 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel, Qais Yousef

This partially reverts commit f4ebcbc0d7e009783256c9daf76bc4b90e645c14.

While trying to address uclamp static key race condition the throttling
logic in RT and fair has highlighted that both perform incomplete
dequeue since they don't go through core. RT logic seemed to diverge
from fair pattern where rq->nr_running was management oddly.

Digging into the history highlighted this partially reverted patch has
added this out-of-line management of rq->nr_running instead in the usual
place when tasks are actually added or removed from internal
structures. It also had a confusing call in sched_rt_rq_dequeue() to
dequeue_top_rt_rq() when it is essentially a dead code - in existing
code base at least.

Revert the logic, but don't break the functionality. Instead do {add,
sub}_nr_running() in {inc, dec}_rt_tasks() where the rest of the
housekeeping is done.

Drop rt_queued usage in rt_rq_is_runnable() since it was unnecessary
check. And make sched_rt_is_runnable() an alias to rt_rq_is_runnable()
since we no longer have rt_queued and both functions are essentially
identical.

Signed-off-by: Qais Yousef <qyousef@layalina.io>
---
 kernel/sched/rt.c    | 74 ++++++--------------------------------------
 kernel/sched/sched.h |  6 ++--
 2 files changed, 11 insertions(+), 69 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 1535046a23ffa..cefb0a8d37e81 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -82,8 +82,6 @@ void init_rt_rq(struct rt_rq *rt_rq)
 	rt_rq->highest_prio.next = MAX_RT_PRIO-1;
 	rt_rq->overloaded = 0;
 	plist_head_init(&rt_rq->pushable_tasks);
-	/* We start is dequeued state, because no RT tasks are queued */
-	rt_rq->rt_queued = 0;
 
 #ifdef CONFIG_RT_GROUP_SCHED
 	rt_rq->rt_time = 0;
@@ -429,8 +427,6 @@ static void dequeue_pushable_task(struct rq *rq, struct task_struct *p)
 	}
 }
 
-static void enqueue_top_rt_rq(struct rt_rq *rt_rq);
-static void dequeue_top_rt_rq(struct rt_rq *rt_rq, unsigned int count);
 
 static inline int on_rt_rq(struct sched_rt_entity *rt_se)
 {
@@ -535,9 +531,7 @@ static void sched_rt_rq_enqueue(struct rt_rq *rt_rq)
 	rt_se = rt_rq->tg->rt_se[cpu];
 
 	if (rt_rq->rt_nr_running) {
-		if (!rt_se)
-			enqueue_top_rt_rq(rt_rq);
-		else if (!on_rt_rq(rt_se))
+		if (rt_se && !on_rt_rq(rt_se))
 			enqueue_rt_entity(rt_se, 0);
 
 		if (rt_rq->highest_prio.curr < donor->prio)
@@ -553,7 +547,6 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
 	rt_se = rt_rq->tg->rt_se[cpu];
 
 	if (!rt_se) {
-		dequeue_top_rt_rq(rt_rq, rt_rq->rt_nr_running);
 		/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
 		cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
 	}
@@ -920,18 +913,12 @@ static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
 
 static inline void sched_rt_rq_enqueue(struct rt_rq *rt_rq)
 {
-	struct rq *rq = rq_of_rt_rq(rt_rq);
-
-	if (!rt_rq->rt_nr_running)
-		return;
-
-	enqueue_top_rt_rq(rt_rq);
-	resched_curr(rq);
+	if (rt_rq->rt_nr_running)
+		resched_curr(rq_of_rt_rq(rt_rq));
 }
 
 static inline void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
 {
-	dequeue_top_rt_rq(rt_rq, rt_rq->rt_nr_running);
 }
 
 static inline int rt_rq_throttled(struct rt_rq *rt_rq)
@@ -1007,44 +994,6 @@ static void update_curr_rt(struct rq *rq)
 #endif /* CONFIG_RT_GROUP_SCHED */
 }
 
-static void
-dequeue_top_rt_rq(struct rt_rq *rt_rq, unsigned int count)
-{
-	struct rq *rq = rq_of_rt_rq(rt_rq);
-
-	BUG_ON(&rq->rt != rt_rq);
-
-	if (!rt_rq->rt_queued)
-		return;
-
-	BUG_ON(!rq->nr_running);
-
-	sub_nr_running(rq, count);
-	rt_rq->rt_queued = 0;
-
-}
-
-static void
-enqueue_top_rt_rq(struct rt_rq *rt_rq)
-{
-	struct rq *rq = rq_of_rt_rq(rt_rq);
-
-	BUG_ON(&rq->rt != rt_rq);
-
-	if (rt_rq->rt_queued)
-		return;
-
-	if (rt_rq_throttled(rt_rq))
-		return;
-
-	if (rt_rq->rt_nr_running) {
-		add_nr_running(rq, rt_rq->rt_nr_running);
-		rt_rq->rt_queued = 1;
-	}
-
-	/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
-	cpufreq_update_util(rq, 0);
-}
 
 static void
 inc_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
@@ -1182,6 +1131,9 @@ void inc_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq *rt_rq)
 
 	inc_rt_prio(rt_rq, prio);
 	inc_rt_group(rt_se, rt_rq);
+
+	if (!group_rt_rq(rt_se))
+		add_nr_running(rq_of_rt_rq(rt_rq), 1);
 }
 
 static inline
@@ -1194,6 +1146,9 @@ void dec_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq *rt_rq)
 
 	dec_rt_prio(rt_rq, rt_se_prio(rt_se));
 	dec_rt_group(rt_se, rt_rq);
+
+	if (!group_rt_rq(rt_se))
+		sub_nr_running(rq_of_rt_rq(rt_rq), 1);
 }
 
 /*
@@ -1383,39 +1338,29 @@ static void __dequeue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flag
 static void dequeue_rt_stack(struct sched_rt_entity *rt_se, unsigned int flags)
 {
 	struct sched_rt_entity *back = NULL;
-	unsigned int rt_nr_running;
 
 	for_each_sched_rt_entity(rt_se) {
 		rt_se->back = back;
 		back = rt_se;
 	}
 
-	rt_nr_running = rt_rq_of_se(back)->rt_nr_running;
-
 	for (rt_se = back; rt_se; rt_se = rt_se->back) {
 		if (on_rt_rq(rt_se))
 			__dequeue_rt_entity(rt_se, flags);
 	}
-
-	dequeue_top_rt_rq(rt_rq_of_se(back), rt_nr_running);
 }
 
 static void enqueue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flags)
 {
-	struct rq *rq = rq_of_rt_se(rt_se);
-
 	update_stats_enqueue_rt(rt_rq_of_se(rt_se), rt_se, flags);
 
 	dequeue_rt_stack(rt_se, flags);
 	for_each_sched_rt_entity(rt_se)
 		__enqueue_rt_entity(rt_se, flags);
-	enqueue_top_rt_rq(&rq->rt);
 }
 
 static void dequeue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flags)
 {
-	struct rq *rq = rq_of_rt_se(rt_se);
-
 	update_stats_dequeue_rt(rt_rq_of_se(rt_se), rt_se, flags);
 
 	dequeue_rt_stack(rt_se, flags);
@@ -1426,7 +1371,6 @@ static void dequeue_rt_entity(struct sched_rt_entity *rt_se, unsigned int flags)
 		if (rt_rq && rt_rq->rt_nr_running)
 			__enqueue_rt_entity(rt_se, flags);
 	}
-	enqueue_top_rt_rq(&rq->rt);
 }
 
 /*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 4706f73838a86..0cab55963657c 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -872,8 +872,6 @@ struct rt_rq {
 	bool			overloaded;
 	struct plist_head	pushable_tasks;
 
-	int			rt_queued;
-
 #ifdef CONFIG_RT_GROUP_SCHED
 	int			rt_throttled;
 	u64			rt_time; /* consumed RT time, goes up in update_curr_rt */
@@ -892,7 +890,7 @@ struct rt_rq {
 
 static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
 {
-	return rt_rq->rt_queued && rt_rq->rt_nr_running;
+	return rt_rq->rt_nr_running;
 }
 
 /* Deadline class' related fields in a runqueue */
@@ -2918,7 +2916,7 @@ static inline bool sched_dl_runnable(struct rq *rq)
 
 static inline bool sched_rt_runnable(struct rq *rq)
 {
-	return rq->rt.rt_queued > 0;
+	return rt_rq_is_runnable(&rq->rt);
 }
 
 static inline bool sched_fair_runnable(struct rq *rq)
-- 
2.53.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c
  2026-11-03  0:59 [PATCH v5 0/4] Fix uclamp race against non atomic static key enable Qais Yousef
                   ` (2 preceding siblings ...)
  2026-11-03  0:59 ` [PATCH RFC v5 3/4] Revert "sched/rt: Substract number of tasks of throttled queues from rq->nr_running" Qais Yousef
@ 2026-11-03  0:59 ` Qais Yousef
  2026-11-03  1:35   ` Qais Yousef
  3 siblings, 1 reply; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  0:59 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel, Qais Yousef

fair and RT throttling can dequeue tasks without going through core
leading to partial dequeue and corrupt rq uclamp state. Fix the problem
by pairing uclamp_rq_{inc, dec}() with {add, sub}_nr_running() so that
each class that cares about uclamp updates the rq state along with the
nr_running.

Remove the sched_delayed guards from uclamp_rq_{inc, dec}() since we now
react at the appropriate time to do the operations and no longer need to
care about this detail. With this gone, the flags parameter can be
dropped too.

Signed-off-by: Qais Yousef <qyousef@layalina.io>
---
 kernel/sched/core.c    | 33 +++++++++------------------------
 kernel/sched/ext/ext.c |  6 ++----
 kernel/sched/fair.c    |  6 ++----
 kernel/sched/rt.c      | 20 ++++++++++++--------
 kernel/sched/sched.h   |  8 ++++----
 5 files changed, 29 insertions(+), 44 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a2035d66270b9..bf690d27faf6e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1866,7 +1866,7 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
 	}
 }
 
-static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags)
+void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
 {
 	enum uclamp_id clamp_id;
 
@@ -1879,13 +1879,6 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags
 	if (!uclamp_is_used())
 		return;
 
-	if (unlikely(!p->sched_class->uclamp_enabled))
-		return;
-
-	/* Only inc the delayed task which being woken up. */
-	if (p->se.sched_delayed && !(flags & ENQUEUE_DELAYED))
-		return;
-
 	for_each_clamp_id(clamp_id)
 		uclamp_rq_inc_id(rq, p, clamp_id);
 
@@ -1894,7 +1887,7 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags
 		uclamp_rq_clear_idle(rq);
 }
 
-static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
+void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
 {
 	enum uclamp_id clamp_id;
 
@@ -1907,12 +1900,6 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
 	if (!uclamp_is_used())
 		return;
 
-	if (unlikely(!p->sched_class->uclamp_enabled))
-		return;
-
-	if (p->se.sched_delayed)
-		return;
-
 	for_each_clamp_id(clamp_id)
 		uclamp_rq_dec_id(rq, p, clamp_id);
 }
@@ -2151,8 +2138,6 @@ static void __init init_uclamp(void)
 }
 
 #else /* !CONFIG_UCLAMP_TASK: */
-static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p, int flags) { }
-static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p) { }
 static inline void uclamp_fork(struct task_struct *p) { }
 static inline void uclamp_post_fork(struct task_struct *p) { }
 static inline void init_uclamp(void) { }
@@ -2188,11 +2173,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 		update_rq_clock(rq);
 
 	/*
-	 * Can be before ->enqueue_task() because uclamp considers the
-	 * ENQUEUE_DELAYED task before its ->sched_delayed gets cleared
-	 * in ->enqueue_task().
+	 * Need to be careful what operations are done here, some classes like
+	 * fair and RT can enqueue tasks without going through core due to some
+	 * features like throttling.
 	 */
-	uclamp_rq_inc(rq, p, flags);
 
 	p->sched_class->enqueue_task(rq, p, flags);
 
@@ -2222,10 +2206,11 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
 	psi_dequeue(p, flags);
 
 	/*
-	 * Must be before ->dequeue_task() because ->dequeue_task() can 'fail'
-	 * and mark the task ->sched_delayed.
+	 * Need to be careful what operations are done here, some classes like
+	 * fair and RT can dequeue tasks without going through core due to some
+	 * features like throttling.
 	 */
-	uclamp_rq_dec(rq, p);
+
 	return p->sched_class->dequeue_task(rq, p, flags);
 }
 
diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 0403c46342813..fe0971cdba691 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -2174,6 +2174,7 @@ static void enqueue_task_scx(struct rq *rq, struct task_struct *p, int core_enq_
 	p->scx.flags |= SCX_TASK_QUEUED;
 	rq->scx.nr_running++;
 	add_nr_running(rq, 1);
+	uclamp_rq_inc(rq, p);
 
 	if (SCX_HAS_OP(sch, runnable) && !task_on_rq_migrating(p))
 		SCX_CALL_OP_TASK(sch, runnable, rq, p, enq_flags);
@@ -2319,6 +2320,7 @@ static bool dequeue_task_scx(struct rq *rq, struct task_struct *p, int core_deq_
 	p->scx.flags &= ~SCX_TASK_QUEUED;
 	rq->scx.nr_running--;
 	sub_nr_running(rq, 1);
+	uclamp_rq_dec(rq, p);
 
 	scx_dispatch_dequeue(rq, p);
 
@@ -5068,10 +5070,6 @@ DEFINE_SCHED_CLASS(ext) = {
 	.prio_changed		= prio_changed_scx,
 
 	.update_curr		= update_curr_scx,
-
-#ifdef CONFIG_UCLAMP_TASK
-	.uclamp_enabled		= 1,
-#endif
 };
 
 s32 scx_init_dsq(struct scx_dispatch_q *dsq, u64 dsq_id, struct scx_sched *sch)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0feec172fd37e..a518e28538a7f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8210,6 +8210,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 
 	/* At this point se is NULL and we are at root level*/
 	add_nr_running(rq, 1);
+	uclamp_rq_inc(rq, p);
 
 	/*
 	 * Since new tasks are assigned an initial util_avg equal to
@@ -8328,6 +8329,7 @@ static bool __dequeue_task(struct rq *rq, struct task_struct *p, int flags)
 		__dequeue_entity(cfs_rq, se);
 
 	sub_nr_running(rq, 1);
+	uclamp_rq_dec(rq, p);
 
 	/* balance early to pull high priority tasks */
 	if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
@@ -15886,10 +15888,6 @@ DEFINE_SCHED_CLASS(fair) = {
 #ifdef CONFIG_SCHED_CORE
 	.task_is_throttled	= task_is_throttled_fair,
 #endif
-
-#ifdef CONFIG_UCLAMP_TASK
-	.uclamp_enabled		= 1,
-#endif
 };
 
 void print_cfs_stats(struct seq_file *m, int cpu)
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index cefb0a8d37e81..96d37a1ccc2a8 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1132,8 +1132,12 @@ void inc_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq *rt_rq)
 	inc_rt_prio(rt_rq, prio);
 	inc_rt_group(rt_se, rt_rq);
 
-	if (!group_rt_rq(rt_se))
-		add_nr_running(rq_of_rt_rq(rt_rq), 1);
+	if (!group_rt_rq(rt_se)) {
+		struct rq *rq = rq_of_rt_rq(rt_rq);
+
+		add_nr_running(rq, 1);
+		uclamp_rq_inc(rq, rt_task_of(rt_se));
+	}
 }
 
 static inline
@@ -1147,8 +1151,12 @@ void dec_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq *rt_rq)
 	dec_rt_prio(rt_rq, rt_se_prio(rt_se));
 	dec_rt_group(rt_se, rt_rq);
 
-	if (!group_rt_rq(rt_se))
-		sub_nr_running(rq_of_rt_rq(rt_rq), 1);
+	if (!group_rt_rq(rt_se)) {
+		struct rq *rq = rq_of_rt_rq(rt_rq);
+
+		sub_nr_running(rq, 1);
+		uclamp_rq_dec(rq, rt_task_of(rt_se));
+	}
 }
 
 /*
@@ -2578,10 +2586,6 @@ DEFINE_SCHED_CLASS(rt) = {
 #ifdef CONFIG_SCHED_CORE
 	.task_is_throttled	= task_is_throttled_rt,
 #endif
-
-#ifdef CONFIG_UCLAMP_TASK
-	.uclamp_enabled		= 1,
-#endif
 };
 
 #ifdef CONFIG_RT_GROUP_SCHED
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0cab55963657c..40876533defdd 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2650,10 +2650,6 @@ enum snt_e {
 
 struct sched_class {
 
-#ifdef CONFIG_UCLAMP_TASK
-	int uclamp_enabled;
-#endif
-
 	/*
 	 * move_queued_task/activate_task/enqueue_task: rq->lock
 	 * ttwu_do_activate/activate_task/enqueue_task: rq->lock
@@ -3703,6 +3699,8 @@ static inline unsigned long cpu_util_rt(struct rq *rq)
 
 unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
 void init_uclamp_rq(struct rq *rq);
+void uclamp_rq_inc(struct rq *rq, struct task_struct *p);
+void uclamp_rq_dec(struct rq *rq, struct task_struct *p);
 
 /*
  * When uclamp is compiled in, the aggregation at rq level is 'turned off'
@@ -3812,6 +3810,8 @@ uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id)
 }
 
 static inline void init_uclamp_rq(struct rq *rq) {}
+static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p) {}
+static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p) {}
 
 static inline bool uclamp_rq_is_capped(struct rq *rq) { return false; }
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c
  2026-11-03  0:59 ` [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c Qais Yousef
@ 2026-11-03  1:35   ` Qais Yousef
  0 siblings, 0 replies; 6+ messages in thread
From: Qais Yousef @ 2026-11-03  1:35 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Vincent Guittot
  Cc: Christian Loehle, Dietmar Eggemann, Steven Rostedt, John Stultz,
	linux-kernel

On 11/3/26 00:59, Qais Yousef wrote:

> @@ -2188,11 +2173,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>  		update_rq_clock(rq);
>  
>  	/*
> -	 * Can be before ->enqueue_task() because uclamp considers the
> -	 * ENQUEUE_DELAYED task before its ->sched_delayed gets cleared
> -	 * in ->enqueue_task().
> +	 * Need to be careful what operations are done here, some classes like
> +	 * fair and RT can enqueue tasks without going through core due to some
> +	 * features like throttling.
>  	 */
> -	uclamp_rq_inc(rq, p, flags);
>  
>  	p->sched_class->enqueue_task(rq, p, flags);
>  
> @@ -2222,10 +2206,11 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>  	psi_dequeue(p, flags);
>  
>  	/*
> -	 * Must be before ->dequeue_task() because ->dequeue_task() can 'fail'
> -	 * and mark the task ->sched_delayed.
> +	 * Need to be careful what operations are done here, some classes like
> +	 * fair and RT can dequeue tasks without going through core due to some
> +	 * features like throttling.
>  	 */
> -	uclamp_rq_dec(rq, p);
> +
>  	return p->sched_class->dequeue_task(rq, p, flags);
>  }
psi_enqueue/dequeue() are done at core, not sure how much an impact this has on
it due to throttling dequeueing/enqueueing without going via core and missing
these updates.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-11-03  1:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-03  0:59 [PATCH v5 0/4] Fix uclamp race against non atomic static key enable Qais Yousef
2026-11-03  0:59 ` [PATCH v5 1/4] sched/uclamp: Fix race condition leading to imbalanced uclamp_rq Qais Yousef
2026-11-03  0:59 ` [PATCH v5 2/4] sched/uclamp: Use wrappers instead of open coding UCLAMP_FLAG_IDLE Qais Yousef
2026-11-03  0:59 ` [PATCH RFC v5 3/4] Revert "sched/rt: Substract number of tasks of throttled queues from rq->nr_running" Qais Yousef
2026-11-03  0:59 ` [PATCH v5 4/4] sched/uclamp: Move uclamp_rq_{inc, dec}() from core.c Qais Yousef
2026-11-03  1:35   ` Qais Yousef

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®