Re: [PATCHv5] sched/deadline: Walk up cpuset hierarchy to decide root domain when hot-unplug

[Waiman Long <llong@redhat.com>]

On 11/10/25 6:14 AM, Juri Lelli wrote:
> Hi,
>
> Looks like this has two issues.
>
> On 10/11/25 09:47, Pingfan Liu wrote:
>
> ...
>
>> +/*
>> + * This function always returns a non-empty bitmap in @cpus. This is because
>> + * if a root domain has reserved bandwidth for DL tasks, the DL bandwidth
>> + * check will prevent CPU hotplug from deactivating all CPUs in that domain.
>> + */
>> +static void dl_get_task_effective_cpus(struct task_struct *p, struct cpumask *cpus)
>> +{
>> +	const struct cpumask *hk_msk;
>> +
>> +	hk_msk = housekeeping_cpumask(HK_TYPE_DOMAIN);
>> +	if (housekeeping_enabled(HK_TYPE_DOMAIN)) {
>> +		if (!cpumask_intersects(p->cpus_ptr, hk_msk)) {
>> +			/*
>> +			 * CPUs isolated by isolcpu="domain" always belong to
>> +			 * def_root_domain.
>> +			 */
>> +			cpumask_andnot(cpus, cpu_active_mask, hk_msk);
>> +			return;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * If a root domain holds a DL task, it must have active CPUs. So
>> +	 * active CPUs can always be found by walking up the task's cpuset
>> +	 * hierarchy up to the partition root.
>> +	 */
>> +	cpuset_cpus_allowed(p, cpus);
> Grabs callbak_lock spin_lock (sleepable on RT) under pi_lock
> raw_spin_lock.
I have been thinking about changing callback_lock to a raw_spinlock_t, 
but need to find a good use case for this change. So it is a solvable 
problem.
>> +}
>> +
>> +/* The caller should hold cpuset_mutex */
There is an upstream patch series that will add a helper function to 
check if cpuset_mutex has been held. So this comment should be replaced 
by a call to that helper function once it is available in the linux 
mainline.
>>   void dl_add_task_root_domain(struct task_struct *p)
>>   {
>>   	struct rq_flags rf;
>>   	struct rq *rq;
>>   	struct dl_bw *dl_b;
>> +	unsigned int cpu;
>> +	struct cpumask msk;
> Potentially huge mask allocated on the stack.

Yes, we should use cpumask_var_t and call alloc_cpumask_var() before 
acquiring lock.

Cheers,
Longman

>
>>   	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
>>   	if (!dl_task(p) || dl_entity_is_special(&p->dl)) {
>> @@ -2891,16 +2923,22 @@ void dl_add_task_root_domain(struct task_struct *p)
>>   		return;
>>   	}
>>   
>> -	rq = __task_rq_lock(p, &rf);
>> -
>> +	/*
>> +	 * Get an active rq, whose rq->rd traces the correct root
>> +	 * domain.
>> +	 * And the caller should hold cpuset_mutex, which gurantees
>> +	 * the cpu remaining in the cpuset until rq->rd is fetched.
>> +	 */
>> +	dl_get_task_effective_cpus(p, &msk);
>> +	cpu = cpumask_first_and(cpu_active_mask, &msk);
>> +	BUG_ON(cpu >= nr_cpu_ids);
>> +	rq = cpu_rq(cpu);
>>   	dl_b = &rq->rd->dl_bw;
>> -	raw_spin_lock(&dl_b->lock);
>>   
>> +	raw_spin_lock(&dl_b->lock);
>>   	__dl_add(dl_b, p->dl.dl_bw, cpumask_weight(rq->rd->span));
>> -
>>   	raw_spin_unlock(&dl_b->lock);
>> -
>> -	task_rq_unlock(rq, p, &rf);
>> +	raw_spin_unlock_irqrestore(&p->pi_lock, rf.flags);
> Thanks,
> Juri
>