Re: [PATCH v2 07/21] srcutree: Disable preemption across synchronize_srcu_atomic()

[Paul E. McKenney]

On Thu, Oct 22, 2026 at 08:19:55PM -0400, Joel Fernandes wrote:
> Hi Paul,
> 
> On Wed, Oct 07, 2026 at 01:59:10PM -0700, Paul E. McKenney wrote:
> > Because synchronize_srcu_atomic() cannot sleep, if a pair of them run
> > concurrently pinned to the same CPU, it is possible that one will spin
> > uselessly waiting for the other while at the same time preventing that
> > other from running.  This commit therefore disables preemption to prevent
> > this failure mode.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > ---
> >  kernel/rcu/srcutree.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 7ff45e992c011..672fb41345c91 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -2123,11 +2123,14 @@ void synchronize_srcu_atomic(struct srcu_struct *ssp)
> >  
> >  	// Perhaps others will do our work for us.
> >  	srcu_state = get_state_synchronize_srcu(ssp);
> > +	preempt_disable();
> 
> Should this be upgraded to local_irq_disable()?
> 
> 
> >  	while (atomic_read(&sup->srcu_atomic_gp_flag) ||
> >  	       atomic_xchg(&sup->srcu_atomic_gp_flag, 1)) {
> > +		preempt_enable();
> >  		if (poll_state_synchronize_srcu(ssp, srcu_state))
> >  			return;
> 
> Otherwise, it is possible that an interrupt handler comes in here while the
> srcu_atomic_gp_flag is already acquired and then spins forever?

If there is a call to synchronize_srcu_atomic() for a given srcu_struct
in an interrupt handler, then all calls to synchronize_srcu_atomic()
for that same srcu_struct must have interrupts disabled.  In addition,
all SRCU read-side critical sections for that same srcu_struct must also
have interrupts disabled.  The rule is that the SRCU readers must run
in at least as restrictive a context as the updaters.  So promoting
that preempt_disable() to local_irq_disable() might instead actually
make user usage errors harder to debug.

I believe that Kunwu's lockdep commits enforce some of these constraints,
but maybe not all of them.

> If we disabled IRQs as well, along with preempt, that might add other
> problems, though, such as a reader calling smp_call_function waiting for the
> IPI handler to run on this CPU which is spinning with interrupts disabled.

Agreed, that could be another problem.

And of course guest OSes have even more fun!

							Thanx, Paul

> thanks,
> 
> Joel Fernandes
> 
> 
> 
> 
> 
> >  		cpu_relax();
> > +		preempt_disable();
> >  	}
> >  
> >  	// One last check for others doing our work for us under the lock.
> > @@ -2135,6 +2138,7 @@ void synchronize_srcu_atomic(struct srcu_struct *ssp)
> >  	if (poll_state_synchronize_srcu(ssp, srcu_state)) {
> >  		raw_spin_unlock_irq_rcu_node(sup);
> >  		atomic_set(&sup->srcu_atomic_gp_flag, 0);
> > +		preempt_enable();
> >  		return;
> >  	}
> >  
> > @@ -2152,6 +2156,7 @@ void synchronize_srcu_atomic(struct srcu_struct *ssp)
> >  	}
> >  	ASSERT_EXCLUSIVE_WRITER(sup->srcu_atomic_gp_flag);
> >  	atomic_set_release(&sup->srcu_atomic_gp_flag, 0);
> > +	preempt_enable();
> >  	non_block_end();
> >  }
> >  EXPORT_SYMBOL_GPL(synchronize_srcu_atomic);
> > -- 
> > 2.40.1
> >