Re: [PATCH -rcu dev 1/2] Revert b8c17e6664c4 ("rcu: Maintain special bits at bottom of ->dynticks counter")

[Paul E. McKenney]

On Wed, Sep 04, 2019 at 12:59:10AM -0400, Joel Fernandes wrote:
> On Tue, Sep 03, 2019 at 01:02:49PM -0700, Paul E. McKenney wrote:
> [snip] 
> > > ---
> > >  include/linux/rcutiny.h |  3 --
> > >  kernel/rcu/tree.c       | 82 ++++++++++-------------------------------
> > >  2 files changed, 19 insertions(+), 66 deletions(-)
> > > 
> > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
> > > index b7607e2667ae..b3f689711289 100644
> > > --- a/include/linux/rcutiny.h
> > > +++ b/include/linux/rcutiny.h
> > > @@ -14,9 +14,6 @@
> > >  
> > >  #include <asm/param.h> /* for HZ */
> > >  
> > > -/* Never flag non-existent other CPUs! */
> > > -static inline bool rcu_eqs_special_set(int cpu) { return false; }
> > > -
> > >  static inline unsigned long get_state_synchronize_rcu(void)
> > >  {
> > >  	return 0;
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 68ebf0eb64c8..417dd00b9e87 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -69,20 +69,10 @@
> > >  
> > >  /* Data structures. */
> > >  
> > > -/*
> > > - * Steal a bit from the bottom of ->dynticks for idle entry/exit
> > > - * control.  Initially this is for TLB flushing.
> > > - */
> > > -#define RCU_DYNTICK_CTRL_MASK 0x1
> > > -#define RCU_DYNTICK_CTRL_CTR  (RCU_DYNTICK_CTRL_MASK + 1)
> > > -#ifndef rcu_eqs_special_exit
> > > -#define rcu_eqs_special_exit() do { } while (0)
> > > -#endif
> > > -
> > >  static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > >  	.dynticks_nesting = 1,
> > >  	.dynticks_nmi_nesting = DYNTICK_IRQ_NONIDLE,
> > > -	.dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > +	.dynticks = ATOMIC_INIT(1),
> > >  };
> > >  struct rcu_state rcu_state = {
> > >  	.level = { &rcu_state.node[0] },
> > > @@ -229,20 +219,15 @@ void rcu_softirq_qs(void)
> > >  static void rcu_dynticks_eqs_enter(void)
> > >  {
> > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > -	int seq;
> > > +	int special;
> > 
> > Given that we really are now loading a pure sequence number, why
> > change the name to "special"?  This revert is after all removing
> > the ability of ->dynticks to be special.
> 
> I have no preference for this variable name, I can call it seq. I was going
> by staying close to 'git revert' to minimize any issues from reverting. I'll
> change it to seq then. But we are also going to rewrite this code anyway as
> we were discussing.
>  
> > >  	/*
> > > -	 * CPUs seeing atomic_add_return() must see prior idle sojourns,
> > > +	 * CPUs seeing atomic_inc_return() must see prior idle sojourns,
> > >  	 * and we also must force ordering with the next RCU read-side
> > >  	 * critical section.
> > >  	 */
> > > -	seq = atomic_add_return(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > -	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > > -		     !(seq & RCU_DYNTICK_CTRL_CTR));
> > > -	if (seq & RCU_DYNTICK_CTRL_MASK) {
> > > -		atomic_andnot(RCU_DYNTICK_CTRL_MASK, &rdp->dynticks);
> > > -		smp_mb__after_atomic(); /* _exit after clearing mask. */
> > > -		/* Prefer duplicate flushes to losing a flush. */
> > > -		rcu_eqs_special_exit();
> > > -	}
> > > +	special = atomic_inc_return(&rdp->dynticks);
> > > +	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !(special & 0x1));
> > >  }
> > >  
> > >  /*
> > > @@ -284,9 +262,9 @@ static void rcu_dynticks_eqs_online(void)
> > >  {
> > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >  
> > > -	if (atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR)
> > > +	if (atomic_read(&rdp->dynticks) & 0x1)
> > >  		return;
> > > -	atomic_add(RCU_DYNTICK_CTRL_CTR, &rdp->dynticks);
> > > +	atomic_add(0x1, &rdp->dynticks);
> > 
> > This could be atomic_inc(), right?
> 
> True, again I will blame 'git revert' ;-) Will change.
> 
> > > -/*
> > > - * Set the special (bottom) bit of the specified CPU so that it
> > > - * will take special action (such as flushing its TLB) on the
> > > - * next exit from an extended quiescent state.  Returns true if
> > > - * the bit was successfully set, or false if the CPU was not in
> > > - * an extended quiescent state.
> > > - */
> > > -bool rcu_eqs_special_set(int cpu)
> > > -{
> > > -	int old;
> > > -	int new;
> > > -	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> > > -
> > > -	do {
> > > -		old = atomic_read(&rdp->dynticks);
> > > -		if (old & RCU_DYNTICK_CTRL_CTR)
> > > -			return false;
> > > -		new = old | RCU_DYNTICK_CTRL_MASK;
> > > -	} while (atomic_cmpxchg(&rdp->dynticks, old, new) != old);
> > > -	return true;
> > > -}
> > > -
> > >  /*
> > >   * Let the RCU core know that this CPU has gone through the scheduler,
> > >   * which is a quiescent state.  This is called when the need for a
> > > @@ -366,13 +322,13 @@ bool rcu_eqs_special_set(int cpu)
> > >   */
> > >  void rcu_momentary_dyntick_idle(void)
> > >  {
> > > -	int special;
> > > +	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > +	int special = atomic_add_return(2, &rdp->dynticks);
> > >  
> > > -	raw_cpu_write(rcu_data.rcu_need_heavy_qs, false);
> > > -	special = atomic_add_return(2 * RCU_DYNTICK_CTRL_CTR,
> > > -				    &this_cpu_ptr(&rcu_data)->dynticks);
> > >  	/* It is illegal to call this from idle state. */
> > > -	WARN_ON_ONCE(!(special & RCU_DYNTICK_CTRL_CTR));
> > > +	WARN_ON_ONCE(!(special & 0x1));
> > > +
> > > +	raw_cpu_write(rcu_data.rcu_need_heavy_qs, false);
> > 
> > Is it really OK to clear .rcu_need_heavy_qs after the atomic_add_return()?
> 
> I think so. I reviewed other code paths and did not an issue with it. Did you
> see something wrong with it?

If this task gets delayed betweentimes, rcu_implicit_dynticks_qs() would
fail to set .rcu_need_heavy_qs because it saw it already being set,
even though the corresponding ->dynticks update had already happened.
(It might be a new grace period, given that the old grace period might
have ended courtesy of the atomic_add_return().)

							Thanx, Paul