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

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 <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Puranjay Mohan <[email protected]>
> > > ---
> > >  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);