Re: [PATCH v10 tip/sched/core 1/6] sched: Annotate rq->rd with __rcu and update lockless readers

[Aaron Tomlin]

On Fri, Oct 30, 2026 at 10:11:12AM +0530, K Prateek Nayak wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index d7c3b43158de..4ead07fd0f6d 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8542,8 +8542,10 @@ void set_rq_online(struct rq *rq)
> >  {
> >  	if (!rq->online) {
> >  		const struct sched_class *class;
> > +		struct root_domain *rd;
> >  
> > -		cpumask_set_cpu(rq->cpu, rq->rd->online);
> > +		rd = rcu_dereference_root_domain(rq->rd);

Hi Prateek,

> 
> nit. Using rcu_dereference_root_domain() everywhere is a bit mouthful.
> How about a couple helpers like like:
> 
> #define rq_rd(rq)	rcu_dereference_root_domain(rq->rd)
> #define cpu_rd(cpu)	rcu_dereference_root_domain(cpu_rq(cpu)->rd)
> 
> ... and those makes the usage much shorter. Thoughts?
> 
> I've attached a coccinelle script at the end that I used to do the
> conversion.

The explicit name was chosen deliberately to mirror
rcu_dereference_sched_domain() and make the RCU read-side critical section
requirement unmistakable to anyone reading the code. I believe shorthand
accessors tend to obscure those synchronization requirements.

> 
> > +		cpumask_set_cpu(rq->cpu, rd->online);
> >  		rq->online = 1;
> >  
> >  		for_each_class(class) {
> > @@ -8557,6 +8559,7 @@ void set_rq_offline(struct rq *rq)
> >  {
> >  	if (rq->online) {
> >  		const struct sched_class *class;
> > +		struct root_domain *rd;
> >  
> >  		update_rq_clock(rq);
> >  		for_each_class(class) {
> > @@ -8564,7 +8567,8 @@ void set_rq_offline(struct rq *rq)
> >  				class->rq_offline(rq);
> >  		}
> >  
> > -		cpumask_clear_cpu(rq->cpu, rq->rd->online);
> > +		rd = rcu_dereference_root_domain(rq->rd);
> 
> nit. If there is no rq_lock() / rcu dependnecy, maybe you can
> initialize rd up top where you declare it for.

Acknowledged.

> 
> > +		cpumask_clear_cpu(rq->cpu, rd->online);
> >  		rq->online = 0;
> >  	}
> >  }
> > @@ -8572,10 +8576,12 @@ void set_rq_offline(struct rq *rq)
> >  static inline void sched_set_rq_online(struct rq *rq, int cpu)
> >  {
> >  	struct rq_flags rf;
> > +	struct root_domain *rd;
> 
> nit. reverse x-mas declaration style would be better IMO.

Acknowledged.

> 
> >  
> >  	rq_lock_irqsave(rq, &rf);
> 
> ... and since you are touching these bits, maybe you can also
> covert these to guard(rq_lock_irqsave)(rq) and save on needing to
> have explicit rq_flags.

I would prefer to leave this out of this series and keep the scope strictly
focused on the __rcu annotations and lockless reader correctness.
Converting rq_lock call sites to guard(rq_lock_irqsave) would be better
suited for a dedicated, tree-wide cleanup pass in core.c.

> 
> > -	if (rq->rd) {
> > -		BUG_ON(!cpumask_test_cpu(cpu, rq->rd->span));
> > +	rd = rcu_dereference_root_domain(rq->rd);
> > +	if (rd) {
> > +		BUG_ON(!cpumask_test_cpu(cpu, rd->span));
> >  		set_rq_online(rq);
> >  	}
> >  	rq_unlock_irqrestore(rq, &rf);
> > @@ -8584,10 +8590,12 @@ static inline void sched_set_rq_online(struct rq *rq, int cpu)
> >  static inline void sched_set_rq_offline(struct rq *rq, int cpu)
> >  {
> >  	struct rq_flags rf;
> > +	struct root_domain *rd;
> >  
> >  	rq_lock_irqsave(rq, &rf);
> > -	if (rq->rd) {
> > -		BUG_ON(!cpumask_test_cpu(cpu, rq->rd->span));
> > +	rd = rcu_dereference_root_domain(rq->rd);
> > +	if (rd) {
> > +		BUG_ON(!cpumask_test_cpu(cpu, rd->span));
> >  		set_rq_offline(rq);
> >  	}
> >  	rq_unlock_irqrestore(rq, &rf);
> > @@ -9009,8 +9017,8 @@ void __init sched_init(void)
> >  #endif
> >  		rq->next_class = &idle_sched_class;
> >  
> > -		rq->sd = NULL;
> > -		rq->rd = NULL;
> > +		RCU_INIT_POINTER(rq->sd, NULL);
> > +		RCU_INIT_POINTER(rq->rd, NULL);
> >  		rq->cpu_capacity = SCHED_CAPACITY_SCALE;
> >  		rq->balance_callback = &balance_push_callback;
> >  		rq->active_balance = 0;
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index c0ebdcde5fe5..2dcb3722c317 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -122,12 +122,12 @@ static inline struct dl_bw *dl_bw_of(int i)
> >  {
> >  	RCU_LOCKDEP_WARN(!rcu_read_lock_sched_held(),
> >  			 "sched RCU must be held");
> 
> Do we need this warbing given we are using rcu_dereference_all_check()
> which checks for RCU too?
> 
> > -	return &cpu_rq(i)->rd->dl_bw;
> > +	return &rcu_dereference_root_domain(cpu_rq(i)->rd)->dl_bw;
> >  }
> >  
> >  static inline int dl_bw_cpus(int i)
> >  {
> > -	struct root_domain *rd = cpu_rq(i)->rd;
> > +	struct root_domain *rd = rcu_dereference_root_domain(cpu_rq(i)->rd);
> >  
> >  	RCU_LOCKDEP_WARN(!rcu_read_lock_sched_held(),
> >  			 "sched RCU must be held");
> 
> this warning may not be required either.

Agreed. Previously, rq->rd was accessed directly, so that manual
RCU_LOCKDEP_WARN() was the sole assertion in place. With
rcu_dereference_root_domain(), rcu_dereference_all_check() already
validates that either RCU is held or sched_domains_mutex is locked under
CONFIG_PROVE_RCU. I will drop the redundant warnings in the next iteration.

> 
> [..snip..]
> 
> > @@ -2167,6 +2167,9 @@ queue_balance_callback(struct rq *rq,
> >  	rq->balance_callback = head;
> >  }
> >  
> > +#define rcu_dereference_root_domain(p) \
> 
>                                        ^
> This argument here can be 'rd' for better clarity
>                                    v
> 
> > +	rcu_dereference_all_check((p), lockdep_is_held(&sched_domains_mutex))
> > +
> >  #define rcu_dereference_sched_domain(p) \
> >  	rcu_dereference_all_check((p), lockdep_is_held(&sched_domains_mutex))

The parameter name 'p' was kept intentionally to maintain 1:1 parity with
rcu_dereference_sched_domain(p) immediately below it, as well as the
general convention used across <linux/rcupdate.h>. Keeping 'p' preserves
that consistency across sched.h.


Kind regards,
-- 
Aaron Tomlin