Re: [PATCH 1/3] rcu/nocb: Use switch/case on NOCB timer state machine

[Frederic Weisbecker]

Le Thu, Oct 10, 2024 at 01:16:41AM -0700, Boqun Feng a écrit :
> On Wed, Oct 02, 2024 at 04:57:36PM +0200, Frederic Weisbecker wrote:
> > It's more convenient to benefit from the fallthrough feature of
> > switch / case to handle the timer state machine. Also a new state is
> > about to be added that will take advantage of it.
> > 
> > No intended functional change.
> > 
> > Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
> > ---
> >  kernel/rcu/tree_nocb.h | 33 +++++++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > index 97b99cd06923..2fb803f863da 100644
> > --- a/kernel/rcu/tree_nocb.h
> > +++ b/kernel/rcu/tree_nocb.h
> > @@ -271,22 +271,35 @@ static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
> >  
> >  	raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
> >  
> > -	/*
> > -	 * Bypass wakeup overrides previous deferments. In case of
> > -	 * callback storms, no need to wake up too early.
> > -	 */
> > -	if (waketype == RCU_NOCB_WAKE_LAZY &&
> > -	    rdp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT) {
> 
> In the old code, if this "if" branch is not taken,
> 
> > -		mod_timer(&rdp_gp->nocb_timer, jiffies + rcu_get_jiffies_lazy_flush());
> > -		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> > -	} else if (waketype == RCU_NOCB_WAKE_BYPASS) {
> > +	switch (waketype) {
> > +	case RCU_NOCB_WAKE_BYPASS:
> > +		/*
> > +		 * Bypass wakeup overrides previous deferments. In case of
> > +		 * callback storms, no need to wake up too early.
> > +		 */
> >  		mod_timer(&rdp_gp->nocb_timer, jiffies + 2);
> >  		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> > -	} else {
> 
> ... it will end up in this else branch, however,
> 
> > +		break;
> > +	case RCU_NOCB_WAKE_LAZY:
> > +		if (rdp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT) {
> > +			mod_timer(&rdp_gp->nocb_timer, jiffies + rcu_get_jiffies_lazy_flush());
> > +			WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> > +		}
> > +		/*
> > +		 * If the timer is already armed, a non-lazy enqueue may have happened
> > +		 * in-between. Don't delay it and fall-through.
> > +		 */
> > +		break;
> 
> ... here we break instead of fallthrough when waketype ==
> RCU_NOCB_WAKE_LAZY and rdp->nocb_defer_wakeup != RCU_NOCB_WAKE_NOT, this
> seems to me a functional change, is this intented?

You unveiled my secret plans!

I think it was intended at the last minute, because it "fixes" some rare case
where RCU_NOCB_WAKE_LAZY can be spuriously set over a RCU_NOCB_WAKE. The effect
shouldn't be too bad because the timer still fires in one jiffy but still
this is confusing.

So saying in the changelog "No functional change intended" was not intended.

I'll refactor the changelogs and comments.

Thanks.

> 
> Regards,
> Boqun
> 
> > +	case RCU_NOCB_WAKE:
> > +		fallthrough;
> > +	case RCU_NOCB_WAKE_FORCE:
> >  		if (rdp_gp->nocb_defer_wakeup < RCU_NOCB_WAKE)
> >  			mod_timer(&rdp_gp->nocb_timer, jiffies + 1);
> >  		if (rdp_gp->nocb_defer_wakeup < waketype)
> >  			WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> > +		break;
> > +	default:
> > +		WARN_ON_ONCE(1);
> >  	}
> >  
> >  	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
> > -- 
> > 2.46.0
> >