Re: [PATCH] srcutree: Add lockdep and early-boot checks to synchronize_srcu_atomic()

["Zqiang" <qiang.zhang@linux.dev>]

> 
> On Sun, Sep 13, 2026 at 2:21 PM Zqiang <qiang.zhang@linux.dev> wrote:
> 
> > 
> > On Sat, Sep 12, 2026 at 08:37:33PM +0800, KunWu Chan wrote:
> > 
> >  >
> >  > On Sat, Sep 12, 2026 at 12:47 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >  >
> >  > On Fri, Sep 11, 2026 at 05:35:15PM +0800, Kunwu Chan wrote:
> >  > > Add the lockdep annotation, same-type SRCU nesting warning, and
> >  > > early-boot check used by __synchronize_srcu().
> >  > >
> >  > > Suggested-by: Zqiang <qiang.zhang@linux.dev>
> >  > > Signed-off-by: Kunwu Chan <kunwu.chan@gmail.com>
> >  >
> >  > Queued for review and testing, thank you both!
> >  >
> >  > Interestingly enough, it is now the case that there is a grace-period
> >  > wait that can be placed in a normal RCU read-side critical section.
> >  > Does this mean that we should also adjust the --do-srcu-lockdep testing
> >  > in tools/testing/selftests/rcutorture/bin/torture.sh?
> >  >
> >  > Thanks, Paul. Good point.
> >  >
> >  > I’ll check the current `--do-srcu-lockdep` coverage, including the
> >  > case where `synchronize_srcu_atomic()` is called from a normal RCU
> >  > read-side critical section, and follow up with the necessary torture
> >  > testing changes.
> >  >
> >  Sounds good!
> > 
> >  Perhaps you and Zqiang can work together on this. Co-developed-by,
> >  for example.
> > 
> >  Hi, Paul and KunWu
> > 
> Hi Zqiang,
> 
> Thanks for pointing out these cases.
> 
> > 
> > Should we also consider the following situations ?
> > 
> >  idx = srcu_read_lock_atomic(srcu)
> > 
> >  by interrupt run hardirq context:
> >  synchronize_rcu_atomic(srcu)
> > 
> >  srcu_read_unlock_atomic(srcu, idx)
> > 
> For the same-CPU interrupt case, the existing check in
> synchronize_srcu_atomic() already catches it:
> 
>  synchronize_srcu_atomic(ssp)
>  RCU_LOCKDEP_WARN(lockdep_is_held(ssp),
>  "Illegal synchronize_srcu_atomic() in same-type SRCU ...");
> 
> lockdep_is_held() resolves to __lock_is_held() (lockdep.c:5612),
> which checks current->held_locks[]. The interrupt handler runs with
> the same current, so it sees the SRCU dep_map acquired by
> srcu_read_lock_atomic().
> 
> > 
> > or:
> > 
> >  CPU0: CPU1:
> > 
> >  idx = srcu_read_lock_atomic(srcu)
> > 
> >  smp_call_function_single(CPU1, som_func, NULL, 1)
> >  to send IPI to CPU1, and sync wait complete.
> >  hardirq context or ide task context:
> > 
> >  some_func()
> >  ->synchronize_rcu_atomic(srcu)
> > 
> >  srcu_read_unlock_atomic(srcu, idx)
> > 
> The cross-CPU case is different. current->held_locks[] is part of
> struct task_struct (sched.h:1302), so the existing
> __lock_is_held() check can only see the current task's held locks.
> It cannot see the SRCU read-side lock held by the task running on
> another CPU. The same limitation applies to the
> lock_is_held(&rcu_lock_map) check in synchronize_srcu() at
> srcutree.c:1665.
> 
> > 
> > Add WARN_ON(irqs_disabled()) to synchronize_rcu_atomic() ?
> > 
> >  Any thoughts?
> > 
> WARN_ON(irqs_disabled()) wouldn't help with the cross-CPU case:
> CPU1 could be running in process context with interrupts enabled, so
> the WARN would not trigger. It would also add a false positive for
> legitimate hardirq calls. synchronize_srcu_atomic() omits
> might_sleep() (compare __synchronize_srcu() at srcutree.c:1676)
> because it is designed to work in contexts where sleeping is not
> allowed, including hardirq context.

If runing in hardirq context, we should use raw_spin_lock_irqsave_rcu_node()
and raw_spin_unlock_irqrestore_rcu_node() to replace raw_spin_lock_irq_rcu_node()
and raw_spin_unlock_irq_rcu_node() in synchronize_srcu_atomic().

Thanks
Zqiang



> 
> Whether a general cross-CPU read-side-hold check is feasible is an
> open question. It would need to account for the read-side state across
> CPUs without adding too much overhead to the SRCU read-side fast path.
> 
> I'm happy to discuss and explore whether there is a reasonable way to
> handle this cross-CPU case.
> 
> Thanks,
> Kunwu
> 
> > 
> > Thanks
> >  Zqiang
> > 
> >  Thanx, Paul
> > 
> >  >
> >  > Thanks,
> >  > Kunwu
> >  >
> >  >
> >  > Thanx, Paul
> >  >
> >  > > ---
> >  > > kernel/rcu/srcutree.c | 8 ++++++++
> >  > > 1 file changed, 8 insertions(+)
> >  > >
> >  > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> >  > > index 6a9c432a3bd0..6c729e805fb3 100644
> >  > > --- a/kernel/rcu/srcutree.c
> >  > > +++ b/kernel/rcu/srcutree.c
> >  > > @@ -2123,6 +2123,14 @@ void synchronize_srcu_atomic(struct srcu_struct *ssp)
> >  > > unsigned long rdm0, rdm1;
> >  > > unsigned long unlocks0, unlocks1;
> >  > >
> >  > > + srcu_lock_sync(&ssp->dep_map);
> >  > > +
> >  > > + RCU_LOCKDEP_WARN(lockdep_is_held(ssp),
> >  > > + "Illegal synchronize_srcu_atomic() in same-type SRCU read-side critical section");
> >  > > +
> >  > > + if (rcu_scheduler_active == RCU_SCHEDULER_INACTIVE)
> >  > > + return;
> >  > > +
> >  > > // Initialize. Either init_srcu_struct() was invoked or
> >  > > // DEFINE_SRCU() or similar was used. Therefore, no allocation
> >  > > // will be done here.
> >  > > --
> >  > > 2.43.0
> >  > >
> >  >
> >
>