Re: [PATCH] sched/deadline: only set free_cpus for online runqueues

[Doug Berger]

On 8/12/2025 8:14 AM, Juri Lelli wrote:
> 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@xxxxxxxxx
> >
> > 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@xxxxxxxxx>
> > ---
>
> 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.
Thanks for the review. I appreciate any testing you are willing to do.

> >   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? :)
Sure! V2 on the way.>> +		__cpumask_clear_cpu(cpu, cp->free_cpus);
> > +	else
> > +		__cpumask_set_cpu(cpu, cp->free_cpus);
> > +
> >   	raw_spin_unlock_irqrestore(&cp->lock, flags);
> >   }
>
> Best,
> Juri

Thanks again,
    Doug