Re: [RFC PATCH] sched/topology: clear freecpu bit on detach

[Doug Berger <opendmb@gmail.com>]

On 1/16/2025 2:13 AM, Juri Lelli wrote:
> Hi Doug,
> 
> On 14/01/25 15:04, Doug Berger wrote:
>> There is a hazard in the deadline scheduler where an offlined CPU
>> can have its free_cpus bit left set in the def_root_domain when
>> the schedutil cpufreq governor is used. This can allow a deadline
>> thread to be pushed to the runqueue of a powered down CPU which
>> breaks scheduling. The details can be found here:
>> https://lore.kernel.org/lkml/20250110233010.2339521-1-opendmb@gmail.com
>>
>> The free_cpus mask is expected to be cleared by set_rq_offline();
>> however, the hazard occurs before the root domain is made online
>> during CPU hotplug so that function is not invoked for the CPU
>> that is being made active.
>>
>> This commit works around the issue by ensuring the free_cpus bit
>> for a CPU is always cleared when the CPU is removed from a
>> root_domain. This likely makes the call of cpudl_clear_freecpu()
>> in rq_offline_dl() fully redundant, but I have not removed it
>> here because I am not certain of all flows.
> 
> Hummm, I am not sure we are covering all the cases this way.
> 
> Would you mind trying out the following maybe? It's based around your
> idea on the first patch you proposed.
Thanks! I'm happy to test any suggestions.

> 
> Thanks!
> Juri
> 
> ---
>   kernel/sched/cpudeadline.c | 23 ++++++-----------------
>   kernel/sched/cpudeadline.h |  3 +--
>   kernel/sched/deadline.c    | 11 ++++++-----
>   3 files changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c
> index 95baa12a1029..30419fa92c1e 100644
> --- a/kernel/sched/cpudeadline.c
> +++ b/kernel/sched/cpudeadline.c
> @@ -170,7 +170,7 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p,
>    *
>    * Returns: (void)
>    */
> -void cpudl_clear(struct cpudl *cp, int cpu)
> +void cpudl_clear(struct cpudl *cp, int cpu, bool offline)
>   {
>   	int old_idx, new_cpu;
>   	unsigned long flags;
> @@ -181,11 +181,8 @@ void cpudl_clear(struct cpudl *cp, int cpu)
>   
>   	old_idx = cp->elements[cpu].idx;
>   	if (old_idx == IDX_INVALID) {
> -		/*
> -		 * Nothing to remove if old_idx was invalid.
> -		 * This could happen if a rq_offline_dl is
> -		 * called for a CPU without -dl tasks running.
> -		 */
> +		if (offline)
> +			cpumask_clear_cpu(cpu, cp->free_cpus);
>   	} else {
>   		new_cpu = cp->elements[cp->size - 1].cpu;
>   		cp->elements[old_idx].dl = cp->elements[cp->size - 1].dl;
> @@ -195,7 +192,8 @@ void cpudl_clear(struct cpudl *cp, int cpu)
>   		cp->elements[cpu].idx = IDX_INVALID;
>   		cpudl_heapify(cp, old_idx);
>   
> -		cpumask_set_cpu(cpu, cp->free_cpus);
> +		if (!offline)
> +			cpumask_set_cpu(cpu, cp->free_cpus);
>   	}
>   	raw_spin_unlock_irqrestore(&cp->lock, flags);
>   }
> @@ -243,19 +241,10 @@ void cpudl_set(struct cpudl *cp, int cpu, u64 dl)
>    */
>   void cpudl_set_freecpu(struct cpudl *cp, int cpu)
>   {
> +	guard(raw_spinlock)(&cp->lock);
>   	cpumask_set_cpu(cpu, cp->free_cpus);
>   }
>   
> -/*
> - * cpudl_clear_freecpu - Clear the cpudl.free_cpus
> - * @cp: the cpudl max-heap context
> - * @cpu: rd attached CPU
> - */
> -void cpudl_clear_freecpu(struct cpudl *cp, int cpu)
> -{
> -	cpumask_clear_cpu(cpu, cp->free_cpus);
> -}
> -
>   /*
>    * cpudl_init - initialize the cpudl structure
>    * @cp: the cpudl max-heap context
> diff --git a/kernel/sched/cpudeadline.h b/kernel/sched/cpudeadline.h
> index 0adeda93b5fb..cb2be7a205dd 100644
> --- a/kernel/sched/cpudeadline.h
> +++ b/kernel/sched/cpudeadline.h
> @@ -18,9 +18,8 @@ struct cpudl {
>   #ifdef CONFIG_SMP
>   int  cpudl_find(struct cpudl *cp, struct task_struct *p, struct cpumask *later_mask);
>   void cpudl_set(struct cpudl *cp, int cpu, u64 dl);
> -void cpudl_clear(struct cpudl *cp, int cpu);
> +void cpudl_clear(struct cpudl *cp, int cpu, bool offline);
>   int  cpudl_init(struct cpudl *cp);
>   void cpudl_set_freecpu(struct cpudl *cp, int cpu);
> -void cpudl_clear_freecpu(struct cpudl *cp, int cpu);
>   void cpudl_cleanup(struct cpudl *cp);
>   #endif /* CONFIG_SMP */
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 643d101cb96a..b52234ebb991 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1870,7 +1870,7 @@ static void dec_dl_deadline(struct dl_rq *dl_rq, u64 deadline)
>   	if (!dl_rq->dl_nr_running) {
>   		dl_rq->earliest_dl.curr = 0;
>   		dl_rq->earliest_dl.next = 0;
> -		cpudl_clear(&rq->rd->cpudl, rq->cpu);
> +		cpudl_clear(&rq->rd->cpudl, rq->cpu, false);
>   		cpupri_set(&rq->rd->cpupri, rq->cpu, rq->rt.highest_prio.curr);
>   	} else {
>   		struct rb_node *leftmost = rb_first_cached(&dl_rq->root);
> @@ -2921,9 +2921,11 @@ static void rq_online_dl(struct rq *rq)
>   	if (rq->dl.overloaded)
>   		dl_set_overload(rq);
>   
> -	cpudl_set_freecpu(&rq->rd->cpudl, rq->cpu);
> -	if (rq->dl.dl_nr_running > 0)
> +	if (rq->dl.dl_nr_running > 0) {
>   		cpudl_set(&rq->rd->cpudl, rq->cpu, rq->dl.earliest_dl.curr);
> +	} else {
> +		cpudl_set_freecpu(&rq->rd->cpudl, rq->cpu);
> +	}
>   }
>   
>   /* Assumes rq->lock is held */
> @@ -2932,8 +2934,7 @@ static void rq_offline_dl(struct rq *rq)
>   	if (rq->dl.overloaded)
>   		dl_clear_overload(rq);
>   
> -	cpudl_clear(&rq->rd->cpudl, rq->cpu);
> -	cpudl_clear_freecpu(&rq->rd->cpudl, rq->cpu);
> +	cpudl_clear(&rq->rd->cpudl, rq->cpu, true);
>   }
>   
>   void __init init_sched_dl_class(void)

Unfortunately, this patch doesn't prevent the issue in my testing.

I believe when the "sugov:n" thread is early scheduled while hotplugging 
CPUn it takes the dec_dl_deadline() path upon completion which ends up 
setting the free_cpus bit in the def_root_domain before the dynamic 
scheduling domain is onlined (even with your patch). This sets the trap 
that gets tripped later.

I attempted to confirm that this was in fact introduced by:
Fixes: 120455c514f7 ("sched: Fix hotplug vs CPU bandwidth control")
However, I discovered that it doesn't appear to have been introduced by 
that commit since the set_rq_offline() call from sched_cpu_dying() was 
already meaningless since the runqueue was already offline by that point 
in the unplug sequence.

Thanks for your feedback,
     Doug