linux-kernel - Re: [PATCH v2] sched/fair: Fix non-empty throttled_limbo_list warning in tg_throttle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251027120211.GB33@bytedance>
Date: Mon, 27 Oct 2025 20:02:11 +0800
From: Aaron Lu <ziqianlu@...edance.com>
To: Hao Jia <jiahao.kernel@...il.com>
Cc: mingo@...hat.com, peterz@...radead.org, mingo@...nel.org,
	juri.lelli@...hat.com, vincent.guittot@...aro.org,
	dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
	mgorman@...e.de, vschneid@...hat.com, kprateek.nayak@....com,
	linux-kernel@...r.kernel.org, Hao Jia <jiahao1@...iang.com>
Subject: Re: [PATCH v2] sched/fair: Fix non-empty throttled_limbo_list
 warning in tg_throttle_down()

On Mon, Oct 27, 2025 at 05:05:34PM +0800, Hao Jia wrote:
> @@ -6403,7 +6407,7 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
>   * expired/exceeded, otherwise it may be allowed to steal additional ticks of
>   * runtime as update_curr() throttling can not trigger until it's on-rq.
>   */
> -static void check_enqueue_throttle(struct cfs_rq *cfs_rq)
> +static void check_enqueue_throttle(struct cfs_rq *cfs_rq, int flags)
>  {
>  	if (!cfs_bandwidth_used())
>  		return;
> @@ -6418,6 +6422,13 @@ static void check_enqueue_throttle(struct cfs_rq *cfs_rq)
>  
>  	/* update runtime allocation */
>  	account_cfs_rq_runtime(cfs_rq, 0);
> +	/*
> +	 * Do not attempt to throttle on the cfs_rq unthrottle path.
> +	 * and it must be placed after account_cfs_rq_runtime() to
> +	 * prevent a possible missed start of the bandwidth timer.

Hi Prateek and Hao,

Does it matter to start the bw timer? If no cfs_rq gets throttled, the
timer doesn't look that useful. Also, account_cfs_rq_runtime() calls
assign_cfs_rq_runtime() and if assign failed, it will do resched_curr()
but since we do not throttle cfs_rq here, that resched would be useless.

> +	 */
> +	if (flags & ENQUEUE_THROTTLE)
> +		return;
>  	if (cfs_rq->runtime_remaining <= 0)
>  		throttle_cfs_rq(cfs_rq);
>  }
> @@ -6724,7 +6735,7 @@ static void sched_fair_update_stop_tick(struct rq *rq, struct task_struct *p)
>  
>  static void account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec) {}
>  static bool check_cfs_rq_runtime(struct cfs_rq *cfs_rq) { return false; }
> -static void check_enqueue_throttle(struct cfs_rq *cfs_rq) {}
> +static void check_enqueue_throttle(struct cfs_rq *cfs_rq, int flags) {}
>  static inline void sync_throttle(struct task_group *tg, int cpu) {}
>  static __always_inline void return_cfs_rq_runtime(struct cfs_rq *cfs_rq) {}
>  static void task_throttle_setup_work(struct task_struct *p) {}
> @@ -6926,6 +6937,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  	int h_nr_runnable = 1;
>  	int task_new = !(flags & ENQUEUE_WAKEUP);
>  	int rq_h_nr_queued = rq->cfs.h_nr_queued;
> +	int throttle_flag = flags & ENQUEUE_THROTTLE;
>  	u64 slice = 0;
>  
>  	if (task_is_throttled(p) && enqueue_throttled_task(p))
> @@ -6983,7 +6995,8 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  		if (cfs_rq_is_idle(cfs_rq))
>  			h_nr_idle = 1;
>  
> -		flags = ENQUEUE_WAKEUP;
> +		/* Ensure ENQUEUE_THROTTLE flag can be propagated through the hierarchy */
> +		flags = ENQUEUE_WAKEUP | throttle_flag;
>  	}
>  
>  	for_each_sched_entity(se) {
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e7718f12bc55..468013d860a6 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2364,6 +2364,10 @@ extern const u32		sched_prio_to_wmult[40];
>   * CLASS - going to update p->sched_class; makes sched_change call the
>   *         various switch methods.
>   *
> + * THROTTLE - invoke in throttle_cfs_rq_work() to ensure task dequeue
> + *            during throttling, and in tg_unthrottle_up() to ensure
> + *            task enqueue during unthrottling.

Thanks for adding the comment for dequeue_throttle. One nit is:
dequeue_throttle is used for time accounting purpose so it would be
better if you can make that clear, maybe something like this:

invoked in throttle_cfs_rq_work() during throttle for accounting
purpose, and in tg_unthrottle_up() to avoid throttling when enqueuing
tasks.

> + *
>   * ENQUEUE_HEAD      - place at front of runqueue (tail if not specified)
>   * ENQUEUE_REPLENISH - CBS (replenish runtime and postpone deadline)
>   * ENQUEUE_MIGRATED  - the task was migrated during wakeup
> @@ -2381,9 +2385,9 @@ extern const u32		sched_prio_to_wmult[40];
>  #define DEQUEUE_MIGRATING	0x0010 /* Matches ENQUEUE_MIGRATING */
>  #define DEQUEUE_DELAYED		0x0020 /* Matches ENQUEUE_DELAYED */
>  #define DEQUEUE_CLASS		0x0040 /* Matches ENQUEUE_CLASS */
> +#define DEQUEUE_THROTTLE	0x0080 /* Matches ENQUEUE_THROTTLE */
>  
>  #define DEQUEUE_SPECIAL		0x00010000
> -#define DEQUEUE_THROTTLE	0x00020000
>  
>  #define ENQUEUE_WAKEUP		0x0001
>  #define ENQUEUE_RESTORE		0x0002
> @@ -2393,6 +2397,7 @@ extern const u32		sched_prio_to_wmult[40];
>  #define ENQUEUE_MIGRATING	0x0010
>  #define ENQUEUE_DELAYED		0x0020
>  #define ENQUEUE_CLASS		0x0040
> +#define ENQUEUE_THROTTLE	0x0080
>  
>  #define ENQUEUE_HEAD		0x00010000
>  #define ENQUEUE_REPLENISH	0x00020000
> -- 
> 2.34.1
>