Re: [PATCH v1 10/11] rcu: Advance callbacks for expedited GP completion in rcu_core()

[Puranjay Mohan]

On Wed, Jul 22, 2026 at 10:50 PM Frederic Weisbecker
<frederic@xxxxxxxxxx> wrote:
>
> Le Tue, Jul 21, 2026 at 04:06:24PM +0100, Puranjay Mohan a écrit :
> > On Tue, Jul 21, 2026 at 3:35 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> > >
> > > Le Wed, Jun 24, 2026 at 06:23:52AM -0700, Puranjay Mohan a écrit :
> > > > Even when rcu_pending() triggers rcu_core(), the normal callback
> > > > advancement path through note_gp_changes() -> __note_gp_changes() bails
> > > > out when rdp->gp_seq == rnp->gp_seq (no normal GP change). Since
> > > > expedited GPs do not update rnp->gp_seq, rcu_advance_cbs() is never
> > > > called and callbacks remain stuck in RCU_WAIT_TAIL.
> > > >
> > > > Add a direct callback advancement block in rcu_core() that checks for GP
> > > > completion via rcu_segcblist_nextgp() combined with
> > > > poll_state_synchronize_rcu_full(). When detected, trylock rnp and call
> > > > rcu_advance_cbs() to move completed callbacks to RCU_DONE_TAIL. Wake the
> > > > GP kthread if rcu_advance_cbs() requests a new grace period.
> > > >
> > > > Uses trylock to avoid adding contention on rnp->lock. If the lock is
> > > > contended, callbacks will be advanced on the next tick.
> > > >
> > > > Reviewed-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > > Signed-off-by: Puranjay Mohan <puranjay@xxxxxxxxxx>
> > > > ---
> > > >  kernel/rcu/tree.c | 17 +++++++++++++++++
> > > >  1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index b01d7bf6b57b1..f42e01ef479c4 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -2891,6 +2891,23 @@ static __latent_entropy void rcu_core(void)
> > > >       /* Update RCU state based on any recent quiescent states. */
> > > >       rcu_check_quiescent_state(rdp);
> > > >
> > > > +     /* Advance callbacks if an expedited GP has completed. */
> > > > +     if (!rcu_rdp_is_offloaded(rdp) && rcu_segcblist_is_enabled(&rdp->cblist)) {
> > > > +             struct rcu_gp_seq gp_state;
> > > > +
> > > > +             if (rcu_segcblist_nextgp(&rdp->cblist, &gp_state) &&
> > > > +                 poll_state_synchronize_rcu_full(&gp_state)) {
> > > > +                     guard(irqsave)();
> > > > +                     if (raw_spin_trylock_rcu_node(rnp)) {
> > > > +                             bool needwake = rcu_advance_cbs(rnp, rdp);
> > > > +
> > > > +                             raw_spin_unlock_rcu_node(rnp);
> > > > +                             if (needwake)
> > > > +                                     rcu_gp_kthread_wake();
> > > > +                     }
> > > > +             }
> > > > +     }
> > >
> > > Should that go as an improvement to note_gp_changes() instead?
> >
> > note_gp_changes() only reconciles rdp->gp_seq against rnp->gp_seq, and
> > the expedited path never advances rnp->gp_seq. So the gap this closes
> > is exactly rdp->gp_seq == rnp->gp_seq, where note_gp_changes() and
> > __note_gp_changes() both short-circuit, the expedited completion isn't
> > visible there at all. It's detected from the cblist's stored gp_seq
> > (rcu_segcblist_nextgp()) confirmed with
> > poll_state_synchronize_rcu_full(), so hosting it in note_gp_changes()
> > would mean running that in the lockless preamble for every caller,
> > including the off-tick call_rcu_core() path. In rcu_core() it's
> > already gated by rcu_pending(), which does the barrier-free detection.
>
> Let's take a step back. note_gp_changes() is for the CPU to ackowledge
> a grace period change, either start or completion, and react upon with:
>
> _ Making the callback progress through the state machine if a grace period
>   has changed.
>
> _ Starting to chase quiescent states.
>
> And now callback advancing/acceleration don't even refer anymore to the
> leaf node state but to the global one. So why not proceed with that
> logic?
>
> Also other callers of note_gp_changes() may want to benefit from expedited
> grace periods as well.
>
> Would the following (untested) work?
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index ff6601411a89..96bf7fe03be8 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1271,27 +1271,29 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
>  {
>         bool ret = false;
>         bool need_qs;
> +       struct rcu_gp_seq gp_state;
>         const bool offloaded = rcu_rdp_is_offloaded(rdp);
>
>         raw_lockdep_assert_held_rcu_node(rnp);
>
> -       if (rdp->gp_seq == rnp->gp_seq)
> -               return false; /* Nothing to do. */
> -
>         /* Handle the ends of any preceding grace periods first. */
> -       if (rcu_seq_completed_gp(rdp->gp_seq, rnp->gp_seq) ||
> +       if ((rcu_segcblist_nextgp(&rdp->cblist, &gp_state) &&
> +           poll_state_synchronize_rcu_full_unordered(&gp_state)) ||
>             unlikely(rdp->gpwrap)) {
>                 if (!offloaded)
>                         ret = rcu_advance_cbs(rnp, rdp); /* Advance CBs. */
>                 rdp->core_needs_qs = false;
>                 trace_rcu_grace_period(rcu_state.name, rdp->gp_seq, TPS("cpuend"));
> -       } else {
> +       } else if (rdp->gp_seq != rnp->gp_seq) {
>                 if (!offloaded)
>                         ret = rcu_accelerate_cbs(rnp, rdp); /* Recent CBs. */
>                 if (rdp->core_needs_qs)
>                         rdp->core_needs_qs = !!(rnp->qsmask & rdp->grpmask);
>         }
>
> +       if (rdp->gp_seq == rnp->gp_seq)
> +               return ret; /* Nothing else to do. */
> +
>         /* Now handle the beginnings of any new-to-this-CPU grace periods. */
>         if (rcu_seq_new_gp(rdp->gp_seq, rnp->gp_seq) ||
>             unlikely(rdp->gpwrap)) {
> @@ -1316,6 +1318,27 @@ static bool __note_gp_changes(struct rcu_node *rnp, struct rcu_data *rdp)
>         return ret;
>  }
>
> +static bool need_note_gp_changes(struct rcu_data *rdp)
> +{
> +       struct rcu_gp_seq gp_state;
> +       struct rcu_node *rnp = rdp->mynode;
> +
> +       /* Need to chase QS or accelerate? */
> +       if (rdp->gp_seq != rcu_seq_current(&rnp->gp_seq))
> +               return true;
> +
> +       /* Waited upon GP has ended, need to advance CBs ? */
> +       if (rcu_segcblist_nextgp(&rdp->cblist, &gp_state) &&
> +           poll_state_synchronize_rcu_full_unordered(&gp_state))
> +               return true;
> +
> +       /* Wrapped? */
> +       if (unlikely(READ_ONCE(rdp->gpwrap)))
> +               return true;
> +
> +       return false;
> +}
> +
>  static void note_gp_changes(struct rcu_data *rdp)
>  {
>         unsigned long flags;
> @@ -1324,8 +1347,7 @@ static void note_gp_changes(struct rcu_data *rdp)
>
>         local_irq_save(flags);
>         rnp = rdp->mynode;
> -       if ((rdp->gp_seq == rcu_seq_current(&rnp->gp_seq) &&
> -            !unlikely(READ_ONCE(rdp->gpwrap))) || /* w/out lock. */
> +       if (!need_note_gp_changes(rdp) || /* w/out lock. */
>             !raw_spin_trylock_rcu_node(rnp)) { /* irqs already off, so later. */
>                 local_irq_restore(flags);
>                 return;
> @@ -2888,23 +2910,6 @@ static __latent_entropy void rcu_core(void)
>         /* Update RCU state based on any recent quiescent states. */
>         rcu_check_quiescent_state(rdp);
>
> -       /* Advance callbacks if an expedited GP has completed. */
> -       if (!rcu_rdp_is_offloaded(rdp) && rcu_segcblist_is_enabled(&rdp->cblist)) {
> -               struct rcu_gp_seq gp_state;
> -
> -               if (rcu_segcblist_nextgp(&rdp->cblist, &gp_state) &&
> -                   poll_state_synchronize_rcu_full(&gp_state)) {
> -                       guard(irqsave)();
> -                       if (raw_spin_trylock_rcu_node(rnp)) {
> -                               bool needwake = rcu_advance_cbs(rnp, rdp);
> -
> -                               raw_spin_unlock_rcu_node(rnp);
> -                               if (needwake)
> -                                       rcu_gp_kthread_wake();
> -                       }
> -               }
> -       }
> -
>         /* No grace period and unregistered callbacks? */
>         if (!rcu_gp_in_progress() &&
>             rcu_segcblist_is_enabled(&rdp->cblist) && !rcu_rdp_is_offloaded(rdp)) {
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 01a1b2985abd..6b9b058d138e 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -517,6 +517,7 @@ static void rcu_nocb_unlock(struct rcu_data *rdp);
>  static void rcu_nocb_unlock_irqrestore(struct rcu_data *rdp,
>                                        unsigned long flags);
>  static void rcu_lockdep_assert_cblist_protected(struct rcu_data *rdp);
> +static bool poll_state_synchronize_rcu_full_unordered(struct rcu_gp_seq *gsp);
>  #ifdef CONFIG_RCU_NOCB_CPU
>  static void __init rcu_organize_nocb_kthreads(void);
>
>


Hi Frederic.

I took your approach and created this commit with minor changes to your diff:

A few points I'd like a second opinion on. I kept the unordered
poll_state_synchronize_rcu_full_unordered() for the advance check (the
old rcu_core() block used the ordered variant): this looks safe
because rcu_segcblist_advance() re-checks each segment with the
ordered poll_state_synchronize_rcu_full() before moving callbacks to
RCU_DONE_TAIL, so the check here is only a gate and the barriers still
apply where callbacks are actually advanced, please confirm that
reasoning. need_note_gp_changes() runs the callback-list poll on the
lockless preamble for both callers, including offloaded rdps where
__note_gp_changes() won't advance anything; I left it ungated since it
only leads to a trylock, but it could take a
!rcu_rdp_is_offloaded(rdp) guard. I also dropped the
rcu_segcblist_is_enabled() guard the rcu_core() block had, relying on
__note_gp_changes() already operating on the cblist unconditionally
for non-offloaded rdps.

-- >8 --