linux-kernel - Re: [PATCH] sched/deadline: only set free

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] [day] [month] [year] [list]

Message-ID: <aJtaSgXf9fkRo-Rp@jlelli-thinkpadt14gen4.remote.csb>
Date: Tue, 12 Aug 2025 17:14:18 +0200
From: Juri Lelli <juri.lelli@...hat.com>
To: Doug Berger <opendmb@...il.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	linux-kernel@...r.kernel.org,
	Florian Fainelli <florian.fainelli@...adcom.com>
Subject: Re: [PATCH] sched/deadline: only set free_cpus for online runqueues

Hi!

On 11/08/25 12:05, Doug Berger wrote:
> Commit 16b269436b72 ("sched/deadline: Modify cpudl::free_cpus
> to reflect rd->online") introduced the cpudl_set/clear_freecpu
> functions to allow the cpu_dl::free_cpus mask to be manipulated
> by the deadline scheduler class rq_on/offline callbacks so the
> mask would also reflect this state.
> 
> Commit 9659e1eeee28 ("sched/deadline: Remove cpu_active_mask
> from cpudl_find()") removed the check of the cpu_active_mask to
> save some processing on the premise that the cpudl::free_cpus
> mask already reflected the runqueue online state.
> 
> Unfortunately, there are cases where it is possible for the
> cpudl_clear function to set the free_cpus bit for a CPU when the
> deadline runqueue is offline. When this occurs while a CPU is
> connected to the default root domain the flag may retain the bad
> state after the CPU has been unplugged. Later, a different CPU
> that is transitioning through the default root domain may push a
> deadline task to the powered down CPU when cpudl_find sees its
> free_cpus bit is set. If this happens the task will not have the
> opportunity to run.
> 
> One example is outlined here:
> https://lore.kernel.org/lkml/20250110233010.2339521-1-opendmb@gmail.com
> 
> Another occurs when the last deadline task is migrated from a
> CPU that has an offlined runqueue. The dequeue_task member of
> the deadline scheduler class will eventually call cpudl_clear
> and set the free_cpus bit for the CPU.
> 
> This commit modifies the cpudl_clear function to be aware of the
> online state of the deadline runqueue so that the free_cpus mask
> can be updated appropriately.
> 
> It is no longer necessary to manage the mask outside of the
> cpudl_set/clear functions so the cpudl_set/clear_freecpu
> functions are removed. In addition, since the free_cpus mask is
> now only updated under the cpudl lock the code was changed to
> use the non-atomic __cpumask functions.
> 
> Signed-off-by: Doug Berger <opendmb@...il.com>
> ---

Thanks for this new approach, it looks good to me.

I would like to stress test it a little more, and have a comment below.

>  kernel/sched/cpudeadline.c | 34 +++++++++-------------------------
>  kernel/sched/cpudeadline.h |  4 +---
>  kernel/sched/deadline.c    |  8 ++++----
>  3 files changed, 14 insertions(+), 32 deletions(-)
> 
> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c
> index cdd740b3f774..d612d5c6c61a 100644
> --- a/kernel/sched/cpudeadline.c
> +++ b/kernel/sched/cpudeadline.c
> @@ -166,12 +166,13 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p,
>   * cpudl_clear - remove a CPU from the cpudl max-heap
>   * @cp: the cpudl max-heap context
>   * @cpu: the target CPU
> + * @online: the online state of the deadline runqueue
>   *
>   * Notes: assumes cpu_rq(cpu)->lock is locked
>   *
>   * Returns: (void)
>   */
> -void cpudl_clear(struct cpudl *cp, int cpu)
> +void cpudl_clear(struct cpudl *cp, int cpu, bool online)
>  {
>  	int old_idx, new_cpu;
>  	unsigned long flags;
> @@ -184,7 +185,7 @@ void cpudl_clear(struct cpudl *cp, int cpu)
>  	if (old_idx == IDX_INVALID) {
>  		/*
>  		 * Nothing to remove if old_idx was invalid.
> -		 * This could happen if a rq_offline_dl is
> +		 * This could happen if rq_online_dl or rq_offline_dl is
>  		 * called for a CPU without -dl tasks running.
>  		 */
>  	} else {
> @@ -195,9 +196,12 @@ void cpudl_clear(struct cpudl *cp, int cpu)
>  		cp->elements[new_cpu].idx = old_idx;
>  		cp->elements[cpu].idx = IDX_INVALID;
>  		cpudl_heapify(cp, old_idx);
> -
> -		cpumask_set_cpu(cpu, cp->free_cpus);
>  	}
> +	if (unlikely(!online))

Isn't using likely(online) more direct and cleaner? :)

> +		__cpumask_clear_cpu(cpu, cp->free_cpus);
> +	else
> +		__cpumask_set_cpu(cpu, cp->free_cpus);
> +
>  	raw_spin_unlock_irqrestore(&cp->lock, flags);
>  }

Best,
Juri