Re: [PATCH RFC] rcu/nocb: Remove unnecessary WakeOvfIsDeferred wake path

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Frederic,

On Thu, Dec 25, 2025 at 11:20:23PM +0100, Frederic Weisbecker wrote:
> Le Thu, Dec 25, 2025 at 02:44:50AM -0500, Joel Fernandes a écrit :
> > The WakeOvfIsDeferred code path in __call_rcu_nocb_wake() attempts to
> > wake rcuog when the callback count exceeds qhimark and callbacks aren't
> > done with their GP (newly queued or awaiting GP). However, a lot of
> > testing proves this wake is always redundant or useless.
> > 
> > In the flooding case, rcuog is always waiting for a GP to finish. So
> > waking up the rcuog thread is pointless. The timer wakeup adds overhead,
> > rcuog simply wakes up and goes back to sleep achieving nothing.
> > 
> > This path also adds a full memory barrier, and additional timer expiry
> > modifications unnecessarily.
> > 
> > The root cause is that WakeOvfIsDeferred fires when
> > !rcu_segcblist_ready_cbs() (GP not complete), but waking rcuog cannot
> > accelerate GP completion.
> > 
> > This commit therefore removes this path, which also adding some rdp
> > counters to ensure we don't have lost wake ups.
> 
> There should be two patches: one that removes the useless path and the
> other that adds the debugging.

Sure, will split.

> > Tested with rcutorture scenarios: TREE01, TREE05, TREE08 (all NOCB
> > configurations) - all pass. Also stress tested using a kernel module
> > that floods call_rcu() to trigger the overload conditions and made the
> > observations confirming the findings.
> > 
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> 
> Cool! Just a few comments:
> 
> > @@ -549,24 +546,26 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> >  	lazy_len = READ_ONCE(rdp->lazy_len);
> >  	if (was_alldone) {
> >  		rdp->qlen_last_fqs_check = len;
> > +		rdp->nocb_gp_wake_attempt = true;
> > +		rcu_nocb_unlock(rdp);
> >  		// Only lazy CBs in bypass list
> >  		if (lazy_len && bypass_len == lazy_len) {
> > -			rcu_nocb_unlock(rdp);
> >  			wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_LAZY,
> >  					   TPS("WakeLazy"));
> >  		} else if (!irqs_disabled_flags(flags)) {
> >  			/* ... if queue was empty ... */
> > -			rcu_nocb_unlock(rdp);
> >  			wake_nocb_gp(rdp, false);
> >  			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
> >  					    TPS("WakeEmpty"));
> >  		} else {
> > -			rcu_nocb_unlock(rdp);
> >  			wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE,
> >  					   TPS("WakeEmptyIsDeferred"));
> >  		}
> > +
> > +		return;
> >  	} else if (len > rdp->qlen_last_fqs_check + qhimark) {
> > -		/* ... or if many callbacks queued. */
> > +		/* Callback overload condition. */
> > +		WARN_ON_ONCE(!rdp->nocb_gp_wake_attempt && !rdp->nocb_gp_serving);
> 
> With this test, the point of ->nocb_gp_serving is unclear given that both
> states are cleared in the same place but ->nocb_gp_serving is set later by
> the gp kthread. ->nocb_gp_serving implies ->nocb_gp_wake_attempt so the above
> test is the same as WARN_ON_ONCE(!rdp->nocb_gp_wake_attempt).
> 
> In fact ->nocb_gp_wake_attempt alone probably makes sense?

Ah true, I got a bit paranoid about false positive warnings hence I added the
extra variable, however on further analysis I realized the nocb lock takes
care of preventing potential false positive warnings. So yes, I will just use
the single variable. Thanks.

> 
> >  		rdp->qlen_last_fqs_check = len;
> >  		j = jiffies;
> >  		if (j != rdp->nocb_gp_adv_time &&
> > @@ -575,21 +574,10 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> >  			rcu_advance_cbs_nowake(rdp->mynode, rdp);
> >  			rdp->nocb_gp_adv_time = j;
> >  		}
> > -		smp_mb(); /* Enqueue before timer_pending(). */
> 
> You need to remove the pairing smp_mb__after_spin_lock() in
> do_nocb_deferred_wakeup_timer().

Ah, will do. Thanks!

 - Joel