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

[Waiman Long]

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