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

On Thu, Nov 05, 2026 at 01:31:37AM +0000, Qais Yousef wrote:
> On 11/3/26 18:51, Kayra Cizmeci wrote:
> 
> > 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 :-)?
> 
> This is an RFC as I was looking for potential historical context why it had to
> be done this way. I did call out in the cover letter that I am not sure about
> the implementation of inc/dec_rt_tasks() and was looking for advise too.
> 
> I might have just missed that we have rt_task_is_entity(), does things get
> better with the below? Otherwise I'll just need to find the time to figure out
> the code and test it properly. Thanks!
> 
> --->8---
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 96d37a1ccc2a8..520273086134d 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1132,7 +1132,7 @@ 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)) {
> +       if (rt_entity_is_task(rt_se)) {
>                 struct rq *rq = rq_of_rt_rq(rt_rq);
> 
>                 add_nr_running(rq, 1);
> @@ -1151,7 +1151,7 @@ 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)) {
> +       if (rt_entity_is_task(rt_se)) {
>                 struct rq *rq = rq_of_rt_rq(rt_rq);
> 
>                 sub_nr_running(rq, 1);
> 


Well I'm *thinking* it's the same thing :/, no?

  #define rt_entity_is_task(rt_se) (!(rt_se)->my_q)


  And look at this:

  static inline struct rt_rq *group_rt_rq(struct sched_rt_entity *rt_se)
  {
  	return rt_se->my_q;
  }




Also, my other test was for this case that you said:

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

It's not dead. My other test was just for that case. Normally it
should, but when we change root's cgroup cpu.rt_runtime_us to 950000, it runs.

Because of two things:


1. We remove dequeue_top_rt_rq() calls, and in root throttle path that is
the thing that preventing root from being selected with reseting
rt_queued.

2. We remove rt_queued itself AND remove the rt_queued check from
rt_rq_is_runnable().

So when we come to pick_task_rt() sched_rt_runnable() returns true,
and we don't return NULL.


Or, is my brain oopsed?

BTW, fixing both of these AFAICT requires bit of
doing the same thing as the commit we are trying to revert. So may be
dropping the revert would be better. Its up to you tho.


What ya think?



Cheers,

Kayra