Re: [PATCH RFC v5 3/4] Revert "sched/rt: Substract number of tasks of throttled queues from rq->nr_running"

[Kayra Cizmeci <kayracizmeci@gmail.com>]

Hi Qais,


On Tue, Nov 03, 2026 at 12:59:52AM +0000, Qais Yousef wrote:

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

But AFAICT from here the commit was doing its job, no? It was just 'odd'.

> 
> 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
> 

So, as I was saying above I gave this a run. Did some tests:

In my base a RT task can only get a CPU's %95.5, with the series %99.9.
Even when rt_throttled is 1, task doesn't stop running. I opened
CONFIG_RT_GROUP_SCHED and set cpu.rt_runtime_us to 950000. cgroup v1.


And AFAICT from the code and my tests, nr_running is handled
differently/wrong with this patch.

When there is a RT group while throttling is enabled, with 2 tasks when
the group throttles nr_running should be 0. Since, there is nothing to
run. (If the group is alone in the rq tho :>) But with the patch series,
it stays 2. I think it's because dec_rt_tasks() does not call
sub_nr_running() in the group_rt_rq() case of se.


Or am I missing something :-)?


Thanks,

Kayra