Date:   Wed, 4 Sep 2019 00:59:10 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Ingo Molnar <mingo@...hat.com>,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Petr Mladek <pmladek@...e.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        rcu@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        Yafang Shao <laoar.shao@...il.com>
Subject: Re: [PATCH -rcu dev 1/2] Revert b8c17e6664c4 ("rcu: Maintain special
 bits at bottom of ->dynticks counter")

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?

thanks,

 - Joel

Powered by blists - more mailing lists