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

[Doug Berger <opendmb@gmail.com>]

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@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@gmail.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.
> 
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