Re: [PATCH] sched/topology: Add a cpus_read_lock to partition_sched_domains()

[Chen Yu]

On Wed, Aug 12, 2026 at 11:05:24AM -0700, Tim Chen wrote:
> > >   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> > >   			     struct sched_domain_attr *dattr_new)
> > >   {
> > > +	guard(cpus_read_lock)();
> > >   	sched_domains_mutex_lock();
> > >   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> > >   	sched_domains_mutex_unlock();
> > 
> > Thanks for taking a look at this issue. I found that there is a comment 
> > around
> > partition_sched_domains() says: "Call with hotplug lock held"
> > Not sure if the caller of partition_sched_domains() rather than 
> > partition_sched_domains()
> > should grab the cpuhotplug lock? I guess the issue is triggered when
> > CONFIG_CPUSETS=n, in this case rebuild_sched_domains() will not grab
> > cpuhotplug lock, should we add guard(cpus_read_lock) in 
> > rebuild_sched_domains()
> > instead?
> 
> When CONFIG_CPUSETS=n, rebuild_sched_domains() is stubbed to
> 
> static inline void rebuild_sched_domains(void)
> {
>         partition_sched_domains(1, NULL, NULL);
> }
> 
> without cpus_read_lock.  Are you suggesting adding the lock
> here?  We do acquire the lock for CONFIG_CPUSETS=y.
>

Yes, I think so.
 
> If we do follow this convention, cpuset_reset_sched_domains()
> and cpuset_reset_sched_domains() also stubbed to
> partition_sched_domains(1, NULL, NULL);
> Probably should add cpus_read_lock there too for consistency.
>

cpuset_reset_sched_domains() is invoked by cpuset_cpu_inactive() 
and via CPU-hotplug callback, and in cpuhp_thread_fun:
static void cpuhp_thread_fun(unsigned int cpu)
{
	lockdep_acquire_cpus_lock(); <--- lockdep
	call_back();
}
and according to the comments:
"
/*
 * The BP holds the hotplug lock, but we're now running on the AP,
 * ensure that anybody asserting the lock is held, will actually find
 * it so.
 */
"
it pretend that AP has grabed the lock on behalf of the BP in
_cpu_down(), which has grabed the cpu-hotplug write lock:
cpus_write_lock().

So it should be ok to not take hotplug lock in cpuset_reset_sched_domains().

thanks,
Chenyu
 
 
> Tim
> 
> > 
> > thanks,
> > Chenyu
> >