When a cfs_rq is to be throttled, its limbo list should be empty and
that's why there is a warn in tg_throttle_down() for non empty
cfs_rq->throttled_limbo_list.

When running a test with the following hierarchy:

          root
        /      \
        A*     ...
     /  |  \   ...
        B
       /  \
      C*

where both A and C have quota settings, that warn on non empty limbo list
is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
part of the cfs_rq for the sake of simpler representation).

Debugging showed it happened like this:
Task group C is created and quota is set, so in tg_set_cfs_bandwidth(),
cfs_rq_c is initialized with runtime_enabled set, runtime_remaining
equals to 0 and *unthrottled*. Before any tasks are enqueued to cfs_rq_c,
*multiple* throttled tasks can migrate to cfs_rq_c (e.g., due to task
group changes). When enqueue_task_fair(cfs_rq_c, throttled_task) is
called and cfs_rq_c is in a throttled hierarchy (e.g., A is throttled),
these throttled tasks are placed into cfs_rq_c's limbo list by
enqueue_throttled_task().

Later, when A is unthrottled, tg_unthrottle_up(cfs_rq_c) enqueues these
tasks. The first enqueue triggers check_enqueue_throttle(), and with zero
runtime_remaining, cfs_rq_c can be throttled in throttle_cfs_rq() if it
can't get more runtime and enters tg_throttle_down(), where the warning
is hit due to remaining tasks in the limbo list.

Fix this by calling throttle_cfs_rq() in tg_set_cfs_bandwidth()
immediately after enabling bandwidth and setting runtime_remaining = 0.
This ensures cfs_rq_c is throttled upfront and cannot enter the enqueue
path in an unthrottled state with no runtime.

Also, update outdated comments in tg_throttle_down() since
unthrottle_cfs_rq() is no longer called with zero runtime_remaining.

While at it, remove a redundant assignment to se in tg_throttle_down().

Fixes: e1fad12dcb66("sched/fair: Switch to task based throttle model")
Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
---
 kernel/sched/core.c  |  9 ++++++++-
 kernel/sched/fair.c  | 16 +++++++---------
 kernel/sched/sched.h |  1 +
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f1e5cb94c536..421166d431fa7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9608,7 +9608,14 @@ static int tg_set_cfs_bandwidth(struct task_group *tg,
 		cfs_rq->runtime_enabled = runtime_enabled;
 		cfs_rq->runtime_remaining = 0;
 
-		if (cfs_rq->throttled)
+		/*
+		 * Throttle cfs_rq now or it can be unthrottled with zero
+		 * runtime_remaining and gets throttled on its unthrottle path.
+		 */
+		if (cfs_rq->runtime_enabled && !cfs_rq->throttled)
+			throttle_cfs_rq(cfs_rq);
+
+		if (!cfs_rq->runtime_enabled && cfs_rq->throttled)
 			unthrottle_cfs_rq(cfs_rq);
 	}
 
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 22e6dd3af82fc..3ef11783369d7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5976,7 +5976,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
 	return 0;
 }
 
-static bool throttle_cfs_rq(struct cfs_rq *cfs_rq)
+bool throttle_cfs_rq(struct cfs_rq *cfs_rq)
 {
 	struct rq *rq = rq_of(cfs_rq);
 	struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
@@ -6025,19 +6025,17 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
 
 	/*
 	 * It's possible we are called with !runtime_remaining due to things
-	 * like user changed quota setting(see tg_set_cfs_bandwidth()) or async
-	 * unthrottled us with a positive runtime_remaining but other still
-	 * running entities consumed those runtime before we reached here.
+	 * like async unthrottled us with a positive runtime_remaining but
+	 * other still running entities consumed those runtime before we
+	 * reached here.
 	 *
-	 * Anyway, we can't unthrottle this cfs_rq without any runtime remaining
-	 * because any enqueue in tg_unthrottle_up() will immediately trigger a
-	 * throttle, which is not supposed to happen on unthrottle path.
+	 * We can't unthrottle this cfs_rq without any runtime remaining
+	 * because any enqueue in tg_unthrottle_up() will immediately trigger
+	 * a throttle, which is not supposed to happen on unthrottle path.
 	 */
 	if (cfs_rq->runtime_enabled && cfs_rq->runtime_remaining <= 0)
 		return;
 
-	se = cfs_rq->tg->se[cpu_of(rq)];
-
 	cfs_rq->throttled = 0;
 
 	update_rq_clock(rq);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b5367c514c143..359bb858cffd3 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -558,6 +558,7 @@ extern void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b, struct cfs_bandwidth
 
 extern void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b);
 extern void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b);
+extern bool throttle_cfs_rq(struct cfs_rq *cfs_rq);
 extern void unthrottle_cfs_rq(struct cfs_rq *cfs_rq);
 extern bool cfs_task_bw_constrained(struct task_struct *p);
 
-- 
2.39.5

Hello Aaron,

On 2025/9/29 15:46, Aaron Lu wrote:
> When a cfs_rq is to be throttled, its limbo list should be empty and
> that's why there is a warn in tg_throttle_down() for non empty
> cfs_rq->throttled_limbo_list.
> 
> When running a test with the following hierarchy:
> 
>            root
>          /      \
>          A*     ...
>       /  |  \   ...
>          B
>         /  \
>        C*
> 
> where both A and C have quota settings, that warn on non empty limbo list
> is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
> part of the cfs_rq for the sake of simpler representation).
> 

I encountered a similar warning a while ago and fixed it. I have a 
question I'd like to ask. tg_unthrottle_up(cfs_rq_C) calls 
enqueue_task_fair(p) to enqueue a task, which requires that the 
runtime_remaining of task p's entire task_group hierarchy be greater than 0.

In addition to the case you fixed above,
When bandwidth is running normally, Is it possible that there's a corner 
case where cfs_A->runtime_remaining > 0, but cfs_B->runtime_remaining < 
0  could trigger a similar warning?

So, I previously tried to fix this issue using the following code, 
adding the ENQUEUE_THROTTLE flag to ensure that tasks enqueued in 
tg_unthrottle_up() aren't throttled.

---
  kernel/sched/fair.c  | 6 ++++--
  kernel/sched/sched.h | 1 +
  2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index df8dc389af8e..128efa2eba57 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5290,7 +5290,9 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct 
sched_entity *se, int flags)
  	se->on_rq = 1;

  	if (cfs_rq->nr_queued == 1) {
-		check_enqueue_throttle(cfs_rq);
+		if (!(flags & ENQUEUE_THROTTLE))
+			check_enqueue_throttle(cfs_rq);
+
  		list_add_leaf_cfs_rq(cfs_rq);
  #ifdef CONFIG_CFS_BANDWIDTH
  		if (cfs_rq->pelt_clock_throttled) {
@@ -5905,7 +5907,7 @@ static int tg_unthrottle_up(struct task_group *tg, 
void *data)
  	list_for_each_entry_safe(p, tmp, &cfs_rq->throttled_limbo_list, 
throttle_node) {
  		list_del_init(&p->throttle_node);
  		p->throttled = false;
-		enqueue_task_fair(rq_of(cfs_rq), p, ENQUEUE_WAKEUP);
+		enqueue_task_fair(rq_of(cfs_rq), p, ENQUEUE_WAKEUP | ENQUEUE_THROTTLE);
  	}

  	/* Add cfs_rq with load or one or more already running entities to 
the list */
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b5367c514c14..871dfb761676 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2358,6 +2358,7 @@ extern const u32		sched_prio_to_wmult[40];
  #define ENQUEUE_MIGRATING	0x100
  #define ENQUEUE_DELAYED		0x200
  #define ENQUEUE_RQ_SELECTED	0x400
+#define ENQUEUE_THROTTLE	0x800

  #define RETRY_TASK		((void *)-1UL)
---

Unfortunately, I tried to build some tests locally and didn't reproduce 
this corner case.

Thanks,
Hao

Hi Hao,

On Tue, Oct 14, 2025 at 03:43:10PM +0800, Hao Jia wrote:
> 
> Hello Aaron,
> 
> On 2025/9/29 15:46, Aaron Lu wrote:
> > When a cfs_rq is to be throttled, its limbo list should be empty and
> > that's why there is a warn in tg_throttle_down() for non empty
> > cfs_rq->throttled_limbo_list.
> > 
> > When running a test with the following hierarchy:
> > 
> >            root
> >          /      \
> >          A*     ...
> >       /  |  \   ...
> >          B
> >         /  \
> >        C*
> > 
> > where both A and C have quota settings, that warn on non empty limbo list
> > is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
> > part of the cfs_rq for the sake of simpler representation).
> > 
> 
> I encountered a similar warning a while ago and fixed it. I have a question
> I'd like to ask. tg_unthrottle_up(cfs_rq_C) calls enqueue_task_fair(p) to
> enqueue a task, which requires that the runtime_remaining of task p's entire
> task_group hierarchy be greater than 0.
> 
> In addition to the case you fixed above,
> When bandwidth is running normally, Is it possible that there's a corner
> case where cfs_A->runtime_remaining > 0, but cfs_B->runtime_remaining < 0
> could trigger a similar warning?

Do you mean B also has quota set and cfs_B's runtime_remaining < 0?
In this case, B should be throttled and C is a descendent of B so should
also be throttled, i.e. C can't be unthrottled when B is in throttled
state. Do I understand you correctly?

> 
> So, I previously tried to fix this issue using the following code, adding
> the ENQUEUE_THROTTLE flag to ensure that tasks enqueued in
> tg_unthrottle_up() aren't throttled.
> 

Yeah I think this can also fix the warning.
I'm not sure if it is a good idea though, because on unthrottle, the
expectation is, this cfs_rq should have runtime_remaining > 0 and if
it's not the case, I think it is better to know why.

Thanks.

Hello Aaron,

Thank you for your reply.

On 2025/10/14 17:11, Aaron Lu wrote:
> Hi Hao,
> 
> On Tue, Oct 14, 2025 at 03:43:10PM +0800, Hao Jia wrote:
>>
>> Hello Aaron,
>>
>> On 2025/9/29 15:46, Aaron Lu wrote:
>>> When a cfs_rq is to be throttled, its limbo list should be empty and
>>> that's why there is a warn in tg_throttle_down() for non empty
>>> cfs_rq->throttled_limbo_list.
>>>
>>> When running a test with the following hierarchy:
>>>
>>>             root
>>>           /      \
>>>           A*     ...
>>>        /  |  \   ...
>>>           B
>>>          /  \
>>>         C*
>>>
>>> where both A and C have quota settings, that warn on non empty limbo list
>>> is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
>>> part of the cfs_rq for the sake of simpler representation).
>>>
>>
>> I encountered a similar warning a while ago and fixed it. I have a question
>> I'd like to ask. tg_unthrottle_up(cfs_rq_C) calls enqueue_task_fair(p) to
>> enqueue a task, which requires that the runtime_remaining of task p's entire
>> task_group hierarchy be greater than 0.
>>
>> In addition to the case you fixed above,
>> When bandwidth is running normally, Is it possible that there's a corner
>> case where cfs_A->runtime_remaining > 0, but cfs_B->runtime_remaining < 0
>> could trigger a similar warning?
> 
> Do you mean B also has quota set and cfs_B's runtime_remaining < 0?
> In this case, B should be throttled and C is a descendent of B so should
> also be throttled, i.e. C can't be unthrottled when B is in throttled
> state. Do I understand you correctly?
>
Yes, both A and B have quota set.

Is there a possible corner case?
Asynchronous unthrottling causes other running entities to completely 
consume cfs_B->runtime_remaining (cfs_B->runtime_remaining < 0) but not 
completely consume cfs_A->runtime_remaining (cfs_A->runtime_remaining > 
0) when we call unthrottle_cfs_rq(cfs_rq_A) .

When we unthrottle_cfs_rq(cfs_rq_A), cfs_A->runtime_remaining > 0, but 
if cfs_B->runtime_remaining < 0 at this time,
therefore, when 
enqueue_task_fair(p)->check_enqueue_throttle(cfs_rq_B)->throttle_cfs_rq(cfs_rq_B), 
an warnning may be triggered.

My core question is:
When we call unthrottle_cfs_rq(cfs_rq_A), we only check 
cfs_rq_A->runtime_remaining. However, 
enqueue_task_fair(p)->enqueue_entity(C->B->A)->check_enqueue_throttle() 
does require that the runtime_remaining of each task_group level of task 
p is greater than 0.

Can we guarantee this?

Thanks,
Hao

>>
>> So, I previously tried to fix this issue using the following code, adding
>> the ENQUEUE_THROTTLE flag to ensure that tasks enqueued in
>> tg_unthrottle_up() aren't throttled.
>>
> 
> Yeah I think this can also fix the warning.
> I'm not sure if it is a good idea though, because on unthrottle, the
> expectation is, this cfs_rq should have runtime_remaining > 0 and if
> it's not the case, I think it is better to know why.
> 
> Thanks.

On Tue, Oct 14, 2025 at 07:01:15PM +0800, Hao Jia wrote:
> 
> Hello Aaron,
> 
> Thank you for your reply.
> 
> On 2025/10/14 17:11, Aaron Lu wrote:
> > Hi Hao,
> > 
> > On Tue, Oct 14, 2025 at 03:43:10PM +0800, Hao Jia wrote:
> > > 
> > > Hello Aaron,
> > > 
> > > On 2025/9/29 15:46, Aaron Lu wrote:
> > > > When a cfs_rq is to be throttled, its limbo list should be empty and
> > > > that's why there is a warn in tg_throttle_down() for non empty
> > > > cfs_rq->throttled_limbo_list.
> > > > 
> > > > When running a test with the following hierarchy:
> > > > 
> > > >             root
> > > >           /      \
> > > >           A*     ...
> > > >        /  |  \   ...
> > > >           B
> > > >          /  \
> > > >         C*
> > > > 
> > > > where both A and C have quota settings, that warn on non empty limbo list
> > > > is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
> > > > part of the cfs_rq for the sake of simpler representation).
> > > > 
> > > 
> > > I encountered a similar warning a while ago and fixed it. I have a question
> > > I'd like to ask. tg_unthrottle_up(cfs_rq_C) calls enqueue_task_fair(p) to
> > > enqueue a task, which requires that the runtime_remaining of task p's entire
> > > task_group hierarchy be greater than 0.
> > > 
> > > In addition to the case you fixed above,
> > > When bandwidth is running normally, Is it possible that there's a corner
> > > case where cfs_A->runtime_remaining > 0, but cfs_B->runtime_remaining < 0
> > > could trigger a similar warning?
> > 
> > Do you mean B also has quota set and cfs_B's runtime_remaining < 0?
> > In this case, B should be throttled and C is a descendent of B so should
> > also be throttled, i.e. C can't be unthrottled when B is in throttled
> > state. Do I understand you correctly?
> > 
> Yes, both A and B have quota set.
> 
> Is there a possible corner case?
> Asynchronous unthrottling causes other running entities to completely
> consume cfs_B->runtime_remaining (cfs_B->runtime_remaining < 0) but not
> completely consume cfs_A->runtime_remaining (cfs_A->runtime_remaining > 0)
> when we call unthrottle_cfs_rq(cfs_rq_A) .

Let me try to understand the situation here: in your described setup,
all three task groups(A, B, C) have quota set?

> 
> When we unthrottle_cfs_rq(cfs_rq_A), cfs_A->runtime_remaining > 0, but if
> cfs_B->runtime_remaining < 0 at this time,

Hmm... if cfs_B->runtime_remaining < 0, why it's not throttled?

> therefore, when enqueue_task_fair(p)->check_enqueue_throttle(cfs_rq_B)->throttle_cfs_rq(cfs_rq_B),

I assume p is a task of group B?
So when A is unthrottled, since p is a throttled task of group B and B
is still throttled, enqueue_task_fair(p) should not happen.

> an warnning may be triggered.
> 
> My core question is:
> When we call unthrottle_cfs_rq(cfs_rq_A), we only check
> cfs_rq_A->runtime_remaining. However,
> enqueue_task_fair(p)->enqueue_entity(C->B->A)->check_enqueue_throttle() does

According to this info, I assume p is a task of group C here. If
unthrottle A would cause enqueuing p, that means: either group C and B
do not have quota set or group C and B are in unthrottled state. 

> require that the runtime_remaining of each task_group level of task p is
> greater than 0.

If group C and B are in unthrottled state, their runtime_remaining
should be > 0.

> 
> Can we guarantee this?

To guarantee this, a warn like below could be used. Can you try in your
setup if you can hit it? Thanks.

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 3ef11783369d7..c347aa28c411a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5908,6 +5908,8 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
 		cfs_rq->throttled_clock_self_time += delta;
 	}
 
+	WARN_ON_ONCE(cfs_rq->runtime_enabled && cfs_rq->runtime_remaining <= 0);
+
 	/* Re-enqueue the tasks that have been throttled at this level. */
 	list_for_each_entry_safe(p, tmp, &cfs_rq->throttled_limbo_list, throttle_node) {
 		list_del_init(&p->throttle_node);

Hello Aaron,

On 9/29/2025 1:16 PM, Aaron Lu wrote:
> When a cfs_rq is to be throttled, its limbo list should be empty and
> that's why there is a warn in tg_throttle_down() for non empty
> cfs_rq->throttled_limbo_list.
> 
> When running a test with the following hierarchy:
> 
>           root
>         /      \
>         A*     ...
>      /  |  \   ...
>         B
>        /  \
>       C*
> 
> where both A and C have quota settings, that warn on non empty limbo list
> is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
> part of the cfs_rq for the sake of simpler representation).
> 
> Debugging showed it happened like this:
> Task group C is created and quota is set, so in tg_set_cfs_bandwidth(),
> cfs_rq_c is initialized with runtime_enabled set, runtime_remaining
> equals to 0 and *unthrottled*. Before any tasks are enqueued to cfs_rq_c,
> *multiple* throttled tasks can migrate to cfs_rq_c (e.g., due to task
> group changes). When enqueue_task_fair(cfs_rq_c, throttled_task) is
> called and cfs_rq_c is in a throttled hierarchy (e.g., A is throttled),
> these throttled tasks are placed into cfs_rq_c's limbo list by
> enqueue_throttled_task().
> 
> Later, when A is unthrottled, tg_unthrottle_up(cfs_rq_c) enqueues these
> tasks. The first enqueue triggers check_enqueue_throttle(), and with zero
> runtime_remaining, cfs_rq_c can be throttled in throttle_cfs_rq() if it
> can't get more runtime and enters tg_throttle_down(), where the warning
> is hit due to remaining tasks in the limbo list.
> 
> Fix this by calling throttle_cfs_rq() in tg_set_cfs_bandwidth()
> immediately after enabling bandwidth and setting runtime_remaining = 0.
> This ensures cfs_rq_c is throttled upfront and cannot enter the enqueue
> path in an unthrottled state with no runtime.
> 
> Also, update outdated comments in tg_throttle_down() since
> unthrottle_cfs_rq() is no longer called with zero runtime_remaining.
> 
> While at it, remove a redundant assignment to se in tg_throttle_down().
> 
> Fixes: e1fad12dcb66("sched/fair: Switch to task based throttle model")
> Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
> ---
>  kernel/sched/core.c  |  9 ++++++++-
>  kernel/sched/fair.c  | 16 +++++++---------
>  kernel/sched/sched.h |  1 +
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 7f1e5cb94c536..421166d431fa7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -9608,7 +9608,14 @@ static int tg_set_cfs_bandwidth(struct task_group *tg,
>  		cfs_rq->runtime_enabled = runtime_enabled;
>  		cfs_rq->runtime_remaining = 0;
>  
> -		if (cfs_rq->throttled)
> +		/*
> +		 * Throttle cfs_rq now or it can be unthrottled with zero
> +		 * runtime_remaining and gets throttled on its unthrottle path.
> +		 */
> +		if (cfs_rq->runtime_enabled && !cfs_rq->throttled)
> +			throttle_cfs_rq(cfs_rq);

So one downside of this is throttle_cfs_rq() here can assign bandwidth
to an empty cfs_rq and a genuine enqueue later on another CPU might not
find bandwidth thus delaying its execution.

Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
if we find cfs_rq->throttled_limbo_list to be empty?

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 18a30ae35441..fd2d4dad9c27 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
 	 */
 	if (throttled_hierarchy(cfs_rq) &&
 	    !task_current_donor(rq_of(cfs_rq), p)) {
+		if (list_empty(&cfs_rq->throttled_limbo_list))
+			check_enqueue_throttle(cfs_rq);
 		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
 		return true;
 	}
---

> +
> +		if (!cfs_rq->runtime_enabled && cfs_rq->throttled)
>  			unthrottle_cfs_rq(cfs_rq);
>  	}
>  
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 22e6dd3af82fc..3ef11783369d7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5976,7 +5976,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
>  	return 0;
>  }
>  
> -static bool throttle_cfs_rq(struct cfs_rq *cfs_rq)
> +bool throttle_cfs_rq(struct cfs_rq *cfs_rq)
>  {
>  	struct rq *rq = rq_of(cfs_rq);
>  	struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
> @@ -6025,19 +6025,17 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
>  
>  	/*
>  	 * It's possible we are called with !runtime_remaining due to things
> -	 * like user changed quota setting(see tg_set_cfs_bandwidth()) or async
> -	 * unthrottled us with a positive runtime_remaining but other still
> -	 * running entities consumed those runtime before we reached here.
> +	 * like async unthrottled us with a positive runtime_remaining but
> +	 * other still running entities consumed those runtime before we
> +	 * reached here.
>  	 *
> -	 * Anyway, we can't unthrottle this cfs_rq without any runtime remaining
> -	 * because any enqueue in tg_unthrottle_up() will immediately trigger a
> -	 * throttle, which is not supposed to happen on unthrottle path.
> +	 * We can't unthrottle this cfs_rq without any runtime remaining
> +	 * because any enqueue in tg_unthrottle_up() will immediately trigger
> +	 * a throttle, which is not supposed to happen on unthrottle path.
>  	 */
>  	if (cfs_rq->runtime_enabled && cfs_rq->runtime_remaining <= 0)
>  		return;
>  
> -	se = cfs_rq->tg->se[cpu_of(rq)];
> -

Ack on these bits!

>  	cfs_rq->throttled = 0;
>  
>  	update_rq_clock(rq);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b5367c514c143..359bb858cffd3 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -558,6 +558,7 @@ extern void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b, struct cfs_bandwidth
>  
>  extern void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b);
>  extern void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b);
> +extern bool throttle_cfs_rq(struct cfs_rq *cfs_rq);
>  extern void unthrottle_cfs_rq(struct cfs_rq *cfs_rq);
>  extern bool cfs_task_bw_constrained(struct task_struct *p);
>  

-- 
Thanks and Regards,
Prateek

On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
... ...
> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
> if we find cfs_rq->throttled_limbo_list to be empty?
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 18a30ae35441..fd2d4dad9c27 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
>  	 */
>  	if (throttled_hierarchy(cfs_rq) &&
>  	    !task_current_donor(rq_of(cfs_rq), p)) {
                /*
                 * Make sure to throttle this cfs_rq or it can be unthrottled
                 * with no runtime_remaining and gets throttled again on its
                 * unthrottle path.
                 */
> +		if (list_empty(&cfs_rq->throttled_limbo_list))
> +			check_enqueue_throttle(cfs_rq);

BTW, do you think a comment is needed? Something like the above, not
sure if it's too redundant though, feel free to let me know your
thoughts, thanks.

>  		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
>  		return true;
>  	}
> ---

Hello Aaron,

On 9/30/2025 1:26 PM, Aaron Lu wrote:
> On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
> ... ...
>> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
>> if we find cfs_rq->throttled_limbo_list to be empty?
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 18a30ae35441..fd2d4dad9c27 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
>>  	 */
>>  	if (throttled_hierarchy(cfs_rq) &&
>>  	    !task_current_donor(rq_of(cfs_rq), p)) {
>                 /*
>                  * Make sure to throttle this cfs_rq or it can be unthrottled
>                  * with no runtime_remaining and gets throttled again on its
>                  * unthrottle path.
>                  */
>> +		if (list_empty(&cfs_rq->throttled_limbo_list))
>> +			check_enqueue_throttle(cfs_rq);
> 
> BTW, do you think a comment is needed? Something like the above, not
> sure if it's too redundant though, feel free to let me know your
> thoughts, thanks.

Now that I'm looking at it again, I think we should actually do a:

    for_each_entity(se)
        check_enqueue_throttle(cfs_rq_of(se));

The reason being, we can have:

    root -> A (throttled) -> B -> C

Consider B has runtime_remaining = 0, and subsequently a throttled task
is queued onto C. Ideally, we should start the B/W timer for B at that
point but we bail out after queuing it on C. Thoughts?

Since we only catch up to the 0 runtime_remaining point, it should be
fine.

The comment can perhaps be something like:

	/*
	 * If this is the first enqueue on throttled hierarchy,
	 * ensure bandwidth is available when the hierarchy is
	 * unthrottled. check_enqueue_throttle() will ensure
	 * either some bandwidth is available, or will throttle
	 * the cfs_rq and queue the bandwidth timer.
	 */

> 
>>  		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
>>  		return true;
>>  	}
>> ---

-- 
Thanks and Regards,
Prateek

On Tue, Sep 30, 2025 at 02:28:16PM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> On 9/30/2025 1:26 PM, Aaron Lu wrote:
> > On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
> > ... ...
> >> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
> >> if we find cfs_rq->throttled_limbo_list to be empty?
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 18a30ae35441..fd2d4dad9c27 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
> >>  	 */
> >>  	if (throttled_hierarchy(cfs_rq) &&
> >>  	    !task_current_donor(rq_of(cfs_rq), p)) {
> >                 /*
> >                  * Make sure to throttle this cfs_rq or it can be unthrottled
> >                  * with no runtime_remaining and gets throttled again on its
> >                  * unthrottle path.
> >                  */
> >> +		if (list_empty(&cfs_rq->throttled_limbo_list))
> >> +			check_enqueue_throttle(cfs_rq);
> > 
> > BTW, do you think a comment is needed? Something like the above, not
> > sure if it's too redundant though, feel free to let me know your
> > thoughts, thanks.
> 
> Now that I'm looking at it again, I think we should actually do a:
> 
>     for_each_entity(se)
>         check_enqueue_throttle(cfs_rq_of(se));
> 
> The reason being, we can have:
> 
>     root -> A (throttled) -> B -> C
> 
> Consider B has runtime_remaining = 0, and subsequently a throttled task
> is queued onto C. Ideally, we should start the B/W timer for B at that
> point but we bail out after queuing it on C. Thoughts?

Yes agree the B/W timer should also be considered.

So in my original patch, cfs_rqs will (most likely) start with
runtime_remaining == 1 and unthrottled after calling throttle_cfs_rq(),
which will also start the B/W timer. The timer is not needed in this
case when no cfs_rqs are actually throttled but it doesn't hurt. Looks
like everything is OK, we do not need to do any special handling in
enqueue_throttled_task(). Thoughts?

Hello Aaron,

I'll merge the two replies in one.

On 9/30/2025 4:37 PM, Aaron Lu wrote:
> So in my original patch, cfs_rqs will (most likely) start with
> runtime_remaining == 1 and unthrottled after calling throttle_cfs_rq(),
> which will also start the B/W timer. The timer is not needed in this
> case when no cfs_rqs are actually throttled but it doesn't hurt. Looks
> like everything is OK, we do not need to do any special handling in
> enqueue_throttled_task(). Thoughts?

Now that I look at throttle_cfs_rq() properly, we'll only move the
runtime_remaining from 0 to 1 so few usecs worth of bandwidth
distributed at max should be okay. Sorry for the being overly cautious!

So your current approach should be good. Please feel free to include:

Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>

As for the other thread:

On 9/30/2025 6:09 PM, Aaron Lu wrote:
>>>
>>>     root -> A (throttled) -> B -> C
>>>
>>> Consider B has runtime_remaining = 0, and subsequently a throttled task
>>> is queued onto C. Ideally, we should start the B/W timer for B at that
>>> point but we bail out after queuing it on C. Thoughts?
>>
>> Yes agree the B/W timer should also be considered.
> 
> On another thought, do we really need care about B/W timer for B?
> 
> I mean, when C is unthrottled and gets enqueued on B,
> check_enqueue_throttle() will do the right thing for B so I don't
> think we need to do this hierarchy check_enqueue_throttle() here.

So what I though would happen here is that when A is unthrottled,
you'll enqueue the task and only then realize B doesn't have any
bandwidth and start the timer then but had you identified it
earlier, distribution could have already added some bandwidth to
B and then you could run the task without adding any further
latency.

-- 
Thanks and Regards,
Prateek

On Tue, Sep 30, 2025 at 07:08:20PM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> I'll merge the two replies in one.
> 
> On 9/30/2025 4:37 PM, Aaron Lu wrote:
> > So in my original patch, cfs_rqs will (most likely) start with
> > runtime_remaining == 1 and unthrottled after calling throttle_cfs_rq(),
> > which will also start the B/W timer. The timer is not needed in this
> > case when no cfs_rqs are actually throttled but it doesn't hurt. Looks
> > like everything is OK, we do not need to do any special handling in
> > enqueue_throttled_task(). Thoughts?
> 
> Now that I look at throttle_cfs_rq() properly, we'll only move the
> runtime_remaining from 0 to 1 so few usecs worth of bandwidth
> distributed at max should be okay. Sorry for the being overly cautious!

Never mind.

> 
> So your current approach should be good. Please feel free to include:
> 
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>

Thanks!

On Tue, Sep 30, 2025 at 07:07:17PM +0800, Aaron Lu wrote:
> On Tue, Sep 30, 2025 at 02:28:16PM +0530, K Prateek Nayak wrote:
> > Hello Aaron,
> > 
> > On 9/30/2025 1:26 PM, Aaron Lu wrote:
> > > On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
> > > ... ...
> > >> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
> > >> if we find cfs_rq->throttled_limbo_list to be empty?
> > >>
> > >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > >> index 18a30ae35441..fd2d4dad9c27 100644
> > >> --- a/kernel/sched/fair.c
> > >> +++ b/kernel/sched/fair.c
> > >> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
> > >>  	 */
> > >>  	if (throttled_hierarchy(cfs_rq) &&
> > >>  	    !task_current_donor(rq_of(cfs_rq), p)) {
> > >                 /*
> > >                  * Make sure to throttle this cfs_rq or it can be unthrottled
> > >                  * with no runtime_remaining and gets throttled again on its
> > >                  * unthrottle path.
> > >                  */
> > >> +		if (list_empty(&cfs_rq->throttled_limbo_list))
> > >> +			check_enqueue_throttle(cfs_rq);
> > > 
> > > BTW, do you think a comment is needed? Something like the above, not
> > > sure if it's too redundant though, feel free to let me know your
> > > thoughts, thanks.
> > 
> > Now that I'm looking at it again, I think we should actually do a:
> > 
> >     for_each_entity(se)
> >         check_enqueue_throttle(cfs_rq_of(se));
> > 
> > The reason being, we can have:
> > 
> >     root -> A (throttled) -> B -> C
> > 
> > Consider B has runtime_remaining = 0, and subsequently a throttled task
> > is queued onto C. Ideally, we should start the B/W timer for B at that
> > point but we bail out after queuing it on C. Thoughts?
> 
> Yes agree the B/W timer should also be considered.

On another thought, do we really need care about B/W timer for B?

I mean, when C is unthrottled and gets enqueued on B,
check_enqueue_throttle() will do the right thing for B so I don't
think we need to do this hierarchy check_enqueue_throttle() here.

I think the only difference with your suggestion and my patch is, with
your suggestion, it's possible for a runtime_enabled cfs_rq to reach
tg_unthrottle_up() with runtime_remaining equals to 0 but since it
doesn't have any tasks in its limbo list, it will not do any enqueue so
won't possibly trigger throttle there, so it's still fine. i.e. I think
your original suggestion works.

Hi Prateek,

On Tue, Sep 30, 2025 at 02:28:16PM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> On 9/30/2025 1:26 PM, Aaron Lu wrote:
> > On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
> > ... ...
> >> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
> >> if we find cfs_rq->throttled_limbo_list to be empty?
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 18a30ae35441..fd2d4dad9c27 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
> >>  	 */
> >>  	if (throttled_hierarchy(cfs_rq) &&
> >>  	    !task_current_donor(rq_of(cfs_rq), p)) {
> >                 /*
> >                  * Make sure to throttle this cfs_rq or it can be unthrottled
> >                  * with no runtime_remaining and gets throttled again on its
> >                  * unthrottle path.
> >                  */
> >> +		if (list_empty(&cfs_rq->throttled_limbo_list))
> >> +			check_enqueue_throttle(cfs_rq);
> > 
> > BTW, do you think a comment is needed? Something like the above, not
> > sure if it's too redundant though, feel free to let me know your
> > thoughts, thanks.
> 
> Now that I'm looking at it again, I think we should actually do a:
> 
>     for_each_entity(se)
>         check_enqueue_throttle(cfs_rq_of(se));

Nice catch and sigh.

> 
> The reason being, we can have:
> 
>     root -> A (throttled) -> B -> C
> 
> Consider B has runtime_remaining = 0, and subsequently a throttled task
> is queued onto C. Ideally, we should start the B/W timer for B at that
> point but we bail out after queuing it on C. Thoughts?
>

If we want to make sure no cfs_rqs with runtime_enabled gets unthrottled
with zero runtime_remaining, agree we will have to do that in a hierarchy
way.

I don't feel good about that for_each_entity(se) check_enqueue_throttle()
though, it made me feel we are duplicating enqueue_task_fair() somehow...

With this said, if we have to do that hierarchical check, I would prefer
to throttle it upfront in tg_set_cfs_bandwidth() :) The useless assign
of runtime is just 1ns, and it should only affect the first period, so
shouldn't matter much?

Hi Prateek,

Thanks for taking a look and the suggestion.

On Mon, Sep 29, 2025 at 03:04:03PM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> On 9/29/2025 1:16 PM, Aaron Lu wrote:
> > When a cfs_rq is to be throttled, its limbo list should be empty and
> > that's why there is a warn in tg_throttle_down() for non empty
> > cfs_rq->throttled_limbo_list.
> > 
> > When running a test with the following hierarchy:
> > 
> >           root
> >         /      \
> >         A*     ...
> >      /  |  \   ...
> >         B
> >        /  \
> >       C*
> > 
> > where both A and C have quota settings, that warn on non empty limbo list
> > is triggered for a cfs_rq of C, let's call it cfs_rq_c(and ignore the cpu
> > part of the cfs_rq for the sake of simpler representation).
> > 
> > Debugging showed it happened like this:
> > Task group C is created and quota is set, so in tg_set_cfs_bandwidth(),
> > cfs_rq_c is initialized with runtime_enabled set, runtime_remaining
> > equals to 0 and *unthrottled*. Before any tasks are enqueued to cfs_rq_c,
> > *multiple* throttled tasks can migrate to cfs_rq_c (e.g., due to task
> > group changes). When enqueue_task_fair(cfs_rq_c, throttled_task) is
> > called and cfs_rq_c is in a throttled hierarchy (e.g., A is throttled),
> > these throttled tasks are placed into cfs_rq_c's limbo list by
> > enqueue_throttled_task().
> > 
> > Later, when A is unthrottled, tg_unthrottle_up(cfs_rq_c) enqueues these
> > tasks. The first enqueue triggers check_enqueue_throttle(), and with zero
> > runtime_remaining, cfs_rq_c can be throttled in throttle_cfs_rq() if it
> > can't get more runtime and enters tg_throttle_down(), where the warning
> > is hit due to remaining tasks in the limbo list.
> > 
> > Fix this by calling throttle_cfs_rq() in tg_set_cfs_bandwidth()
> > immediately after enabling bandwidth and setting runtime_remaining = 0.
> > This ensures cfs_rq_c is throttled upfront and cannot enter the enqueue
> > path in an unthrottled state with no runtime.
> > 
> > Also, update outdated comments in tg_throttle_down() since
> > unthrottle_cfs_rq() is no longer called with zero runtime_remaining.
> > 
> > While at it, remove a redundant assignment to se in tg_throttle_down().
> > 
> > Fixes: e1fad12dcb66("sched/fair: Switch to task based throttle model")
> > Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
> > ---
> >  kernel/sched/core.c  |  9 ++++++++-
> >  kernel/sched/fair.c  | 16 +++++++---------
> >  kernel/sched/sched.h |  1 +
> >  3 files changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 7f1e5cb94c536..421166d431fa7 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -9608,7 +9608,14 @@ static int tg_set_cfs_bandwidth(struct task_group *tg,
> >  		cfs_rq->runtime_enabled = runtime_enabled;
> >  		cfs_rq->runtime_remaining = 0;
> >  
> > -		if (cfs_rq->throttled)
> > +		/*
> > +		 * Throttle cfs_rq now or it can be unthrottled with zero
> > +		 * runtime_remaining and gets throttled on its unthrottle path.
> > +		 */
> > +		if (cfs_rq->runtime_enabled && !cfs_rq->throttled)
> > +			throttle_cfs_rq(cfs_rq);
> 
> So one downside of this is throttle_cfs_rq() here can assign bandwidth
> to an empty cfs_rq and a genuine enqueue later on another CPU might not
> find bandwidth thus delaying its execution.

Agree that assign doesn't make sense here.

> 
> Can we instead do a check_enqueue_throttle() in enqueue_throttled_task()
> if we find cfs_rq->throttled_limbo_list to be empty?
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 18a30ae35441..fd2d4dad9c27 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5872,6 +5872,8 @@ static bool enqueue_throttled_task(struct task_struct *p)
>  	 */
>  	if (throttled_hierarchy(cfs_rq) &&
>  	    !task_current_donor(rq_of(cfs_rq), p)) {
> +		if (list_empty(&cfs_rq->throttled_limbo_list))
> +			check_enqueue_throttle(cfs_rq);
>  		list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list);
>  		return true;
>  	}
> ---
> 

Works for me, will follow your suggestion if no other comments, thanks!