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

[Waiman Long <llong@redhat.com>]

On 11/23/25 8:45 PM, Pingfan Liu wrote:
> On Fri, Nov 21, 2025 at 02:05:31PM +0100, Juri Lelli wrote:
>> Hi!
>>
>> On 19/11/25 17:55, Pingfan Liu wrote:
>>
>> ...
>>
>>> +/* Access rule: must be called on local CPU with preemption disabled */
>>>   static DEFINE_PER_CPU(cpumask_var_t, local_cpu_mask_dl);
>> ...
>>
>>> +/* The caller should hold cpuset_mutex */
>> Maybe we can add a lockdep explicit check?
>>
> Currently, all cpuset locks are encapsulated in
> kernel/cgroup/cpuset-internal.h. I'm not sure if it's appropriate to
> expose them. If exposing them is acceptable,
> cpuset_callback_lock_irq()/cpuset_callback_unlock_irq() would be
> preferable to cpuset_mutex assertion.
>
> @Waiman, @Ridong, could you kindly share your opinion?

The cpuset_cpus_allowed_locked() already has a 
"lockdep_assert_held(&cpuset_mutex)"  call to make sure that 
cpuset_mutex is held, or a warning will be printed by the debug kernel. 
So a check is there, it is just not in the deadline.c code. The 
dl_add_task_root_domain() is called indirectly from 
dl_rebuild_rd_accounting() in cpuset.c which does have an assertion on 
cpuset_mutex.

There is an external visible cpuset_lock/unlock() to acquire and release 
the cpuset_mutex. However, there is no public API to assert that 
cpuset_mutex is held. There is another set of patch series that is going 
to add that in the near future. At this point, I don't think we need to 
have such an API yet. I will suggest adding comment 
to cpuset_cpus_allowed_locked() that it will warn if cpuset_mutex isn't 
held.

Providing a cpuset_callback_{lock|unlock}_irq() helpers may not be 
helpful because we are back to the problem that callback_lock isn't a 
raw_spinlock_t.

Cheers,
Longman
>
>>>   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 = this_cpu_cpumask_var_ptr(local_cpu_mask_dl);
>> Can this corrupt local_cpu_mask_dl?
>>
>> Without preemption being disabled, the following race can occur:
>>
>> 1. Thread calls dl_add_task_root_domain() on CPU 0
>> 2. Gets pointer to CPU 0's local_cpu_mask_dl
>> 3. Thread is preempted and migrated to CPU 1
>> 4. Thread continues using CPU 0's local_cpu_mask_dl
>> 5. Meanwhile, the scheduler on CPU 0 calls find_later_rq() which also
>>     uses local_cpu_mask_dl (with preemption properly disabled)
>> 6. Both contexts now corrupt the same per-CPU buffer concurrently
>>
> Oh, that is definitely an issue. Thanks for pointing it out.
>
>>>   
>>>   	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
>> It's safe to get the pointer after this point.
>>
> Yes.
>>>   	if (!dl_task(p) || dl_entity_is_special(&p->dl)) {
>>> @@ -2919,16 +2952,25 @@ 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.
>>> +	 * Ideally this would be under cpuset reader lock until rq->rd is
>>> +	 * fetched.  However, sleepable locks cannot nest inside pi_lock, so we
>>> +	 * rely on the caller of dl_add_task_root_domain() holds 'cpuset_mutex'
>>> +	 * to guarantee the CPU stays in the cpuset.
>>> +	 */
>>> +	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);
>>> +	/* End of fetching rd */
>> Not sure we need this comment above. :)
>>
> OK, I can remove them to keep the code neat.
>
>
> Thanks,
>
> Pingfan
>