Re: [PATCH v4 1/5] sched/debug: Protect lockless rq->rd access in print_dl_rq()

[Aaron Tomlin]

On Mon, Aug 10, 2026 at 12:20:27PM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> On 8/10/2026 7:28 AM, Aaron Tomlin wrote:
> > @@ -1089,9 +1090,14 @@ void print_dl_rq(struct seq_file *m, int cpu, struct dl_rq *dl_rq)
> >  	SEQ_printf(m, "  .%-30s: %lu\n", #x, (unsigned long)(dl_rq->x))
> >  
> >  	PU(dl_nr_running);
> > -	dl_bw = &cpu_rq(cpu)->rd->dl_bw;
> > -	SEQ_printf(m, "  .%-30s: %lld\n", "dl_bw->bw", dl_bw->bw);
> > -	SEQ_printf(m, "  .%-30s: %lld\n", "dl_bw->total_bw", dl_bw->total_bw);
> > +	rcu_read_lock();
> > +	rd = READ_ONCE(cpu_rq(cpu)->rd);
> 
> nit. Why not rcu_dereference_all() here to keep symmetry with the
> update side that now uses rcu_assign_pointer()?

Hi Prateek,

Thank you for your feedback.

The 'rd' field of struct rq does not have the __rcu compiler annotation.
So, technically it is not an RCU-pointer. Therfore, using
rcu_dereference(cpu_rq(cpu)->rd) directly would actually trigger a sparse
warning. However, now that I introduced rcu_assign_pointer(rq->rd, rd) in
rq_attach_root(), we should add __rcu to be compliant.

> > +	if (rd) {
> 
> I don't think rq->rd can ever be NULL after the scheduler has
> initialized.

Yes, you're right, after sched_init().

> 
> > +		dl_bw = &rd->dl_bw;
> > +		SEQ_printf(m, "  .%-30s: %lld\n", "dl_bw->bw", dl_bw->bw);
> > +		SEQ_printf(m, "  .%-30s: %lld\n", "dl_bw->total_bw", dl_bw->total_bw);
> > +	}
> > +	rcu_read_unlock();
> >  
> >  #undef PU
> >  }


Kind regards,
-- 
Aaron Tomlin