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
