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

[Waiman Long]

On 11/23/25 10:56 PM, Pingfan Liu wrote:
> On Mon, Nov 24, 2025 at 10:24 AM Waiman Long <llong@xxxxxxxxxx> wrote:
> [...]
> > > 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.
> I meant to put them outside the pi_lock, so it can reflect the
> original purpose of this section -- cpuset read access instead of
> write. But yes, I agree that at this point, there is no need to
> introduce a public API.

By putting it outside of the pi_lock, you may as well just call 
cpuset_cpus_allowed().

Cheers,
Longman

> > > > >    void dl_add_task_root_domain(struct task_struct *p)
> > > > >    {
> [...]
>
> > > > > +   /*
> > > > > +    * 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.
> @Juri, sorry - I need to send out a fix that should be simple and
> focused: just the fix itself, without removing the comments. So I have
> not removed them. Anyway, they can remind us this is an atomic cpuset
> read context.
>
>
> Best Regards,
>
> Pingfan