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

[Tim Chen <tim.c.chen@linux.intel.com>]

On Thu, 2026-08-13 at 10:24 +0800, Chen Yu wrote:
> 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().

Yes, likewise cpuset_update_active_cpus() that uses partition_sched_domains(1, NULL, NULL)
is called from cpu-hotplug path with proper lock, we are good there too.

Tim