Re: [tip:core/rcu] Revert "rcu: Decrease memory-barrier usage based on semi-formal proof"

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, May 24, 2011 at 02:23:45PM -0700, Yinghai Lu wrote:
> On 05/23/2011 06:35 PM, Paul E. McKenney wrote:
> > On Mon, May 23, 2011 at 06:26:23PM -0700, Yinghai Lu wrote:
> >> On 05/23/2011 06:18 PM, Paul E. McKenney wrote:
> >>
> >>> OK, so it looks like I need to get this out of the way in order to track
> >>> down the delays.  Or does reverting PeterZ's patch get you a stable
> >>> system, but with the longish delays in memory_dev_init()?  If the latter,
> >>> it might be more productive to handle the two problems separately.
> >>>
> >>> For whatever it is worth, I do see about 5% increase in grace-period
> >>> duration when switching to kthreads.  This is acceptable -- your
> >>> 30x increase clearly is completely unacceptable and must be fixed.
> >>> Other than that, the main thing that affects grace period duration is
> >>> the setting of CONFIG_HZ -- the smaller the HZ value, the longer the
> >>> grace-period duration.
> >>
> >> for my 1024g system when memory hotadd is enabled in kernel config:
> >> 1. current linus tree + tip tree:  memory_dev_init will take about 100s.
> >> 2. current linus tree + tip tree + your tree - Peterz patch: 
> >>    a. on fedora 14 gcc: will cost about 4s: like old times
> >>    b. on opensuse 11.3 gcc: will cost about 10s.
> > 
> > So some patch in my tree that is not yet in tip makes things better?
> > 
> > If so, could you please see which one?  Maybe that would give me a hint
> > that could make things better on opensuse 11.3 as well.
> 
> today's tip:
> 
> [   31.795597] cpu_dev_init done
> [   40.930202] memory_dev_init done

One other question...  What is memory_dev_init() doing to wait for so
many RCU grace periods?  (Yes, I do need to fix the slowdowns in any
case, but I am curious.)

> after
> 
> commit e219b351fc90c0f5304e16efbc603b3b78843ea1
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon May 16 02:44:06 2011 -0700
> 
>     rcu: Remove old memory barriers from rcu_process_callbacks()
>     
>     Second step of partitioning of commit e59fb3120b.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 3731141..011bf6f 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -1460,25 +1460,11 @@ __rcu_process_callbacks(struct rcu_state *rsp, struct rcu_data *rdp)
>   */
>  static void rcu_process_callbacks(void)
>  {
> -	/*
> -	 * Memory references from any prior RCU read-side critical sections
> -	 * executed by the interrupted code must be seen before any RCU
> -	 * grace-period manipulations below.
> -	 */
> -	smp_mb(); /* See above block comment. */
> -
>  	__rcu_process_callbacks(&rcu_sched_state,
>  				&__get_cpu_var(rcu_sched_data));
>  	__rcu_process_callbacks(&rcu_bh_state, &__get_cpu_var(rcu_bh_data));
>  	rcu_preempt_process_callbacks();
> 
> -	/*
> -	 * Memory references from any later RCU read-side critical sections
> -	 * executed by the interrupted code must be seen after any RCU
> -	 * grace-period manipulations above.
> -	 */
> -	smp_mb(); /* See above block comment. */
> -
>  	/* If we are last CPU on way to dyntick-idle mode, accelerate it. */
>  	rcu_needs_cpu_flush();
>  }
> 
> cause
> 
> [   32.235103] cpu_dev_init done
> [   74.897943] memory_dev_init done
> 
> then add
> 
> commit d0d642680d4cf5cc2ccf542b74a3c8b7e197306b
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon May 16 02:52:04 2011 -0700
> 
>     rcu: Don't do reschedule unless in irq
>     
>     Condition the set_need_resched() in rcu_irq_exit() on in_irq().  This
>     should be a no-op, because rcu_irq_exit() should only be called from irq.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 011bf6f..195b3a3 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -421,8 +421,9 @@ void rcu_irq_exit(void)
>  	WARN_ON_ONCE(rdtp->dynticks & 0x1);
> 
>  	/* If the interrupt queued a callback, get out of dyntick mode. */
> -	if (__this_cpu_read(rcu_sched_data.nxtlist) ||
> -	    __this_cpu_read(rcu_bh_data.nxtlist))
> +	if (in_irq() &&
> +	    (__this_cpu_read(rcu_sched_data.nxtlist) ||
> +	     __this_cpu_read(rcu_bh_data.nxtlist)))
>  		set_need_resched();
>  }
> 
> got:
> 
> [   34.384490] cpu_dev_init done
> [   86.656322] memory_dev_init done
> 
> 
> after
> 
> commit fcfc28801f5b3b9c70616fc57e3a2c6f52014e14
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Mon May 16 14:27:31 2011 -0700
> 
>     rcu: Make rcu_enter_nohz() pay attention to nesting
>     
>     The old version of rcu_enter_nohz() forced RCU into nohz mode even if
>     the nesting count was non-zero.  This change causes rcu_enter_nohz()
>     to hold off for non-zero nesting counts.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 195b3a3..99c6038 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -324,8 +324,8 @@ void rcu_enter_nohz(void)
>  	smp_mb(); /* CPUs seeing ++ must see prior RCU read-side crit sects */
>  	local_irq_save(flags);
>  	rdtp = &__get_cpu_var(rcu_dynticks);
> -	rdtp->dynticks++;
> -	rdtp->dynticks_nesting--;
> +	if (--rdtp->dynticks_nesting == 0)
> +		rdtp->dynticks++;
>  	WARN_ON_ONCE(rdtp->dynticks & 0x1);
>  	local_irq_restore(flags);
>  }
> 
> got: 
> 
> [   32.414049] cpu_dev_init done
> [   38.237979] memory_dev_init done

So this is best for you -- where we have done all but the last commit
of restoring "Decrease memory-barrier usage based on semi-formal proof".
It makes sense that this one would help, as it is eliminating delays
due to misnesting.  These delays are not hangs, as force_quiescent_state()
will eventually force the right thing to happen, but getting rid of these
delays should indeed speed things up.

> after:
> commit bcd6e68330f893a81b3519ab3c5fc2bebbc9988c
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Tue Sep 7 10:38:22 2010 -0700
> 
>     rcu: Decrease memory-barrier usage based on semi-formal proof
> ...
> 
> got:
> 
> [   32.447936] cpu_dev_init done
> [  111.027066] memory_dev_init done

So there is something nasty in this patch.

Not seeing it immediately, but it does give me some focus for both
code inspection and possible diagnostic patches.

> after 
> 
> commit fbb753fb9dd62318d27fa070c686423ced139817
> Author: Paul E. McKenney <paul.mckenney@linaro.org>
> Date:   Wed May 11 05:33:33 2011 -0700
> 
>     atomic: Add atomic_or()
>     
>     An atomic_or() function is needed by TREE_RCU to avoid deadlock, so
>     add a generic version.
>     
>     Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index 96c038e..ee456c7 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -34,4 +34,17 @@ static inline int atomic_inc_not_zero_hint(atomic_t *v, int hint)
>  }
>  #endif
> 
> +#ifndef CONFIG_ARCH_HAS_ATOMIC_OR
> +static inline void atomic_or(int i, atomic_t *v)
> +{
> +	int old;
> +	int new;
> +
> +	do {
> +		old = atomic_read(v);
> +		new = old | i;
> +	} while (atomic_cmpxchg(v, old, new) != old);
> +}
> +#endif /* #ifndef CONFIG_ARCH_HAS_ATOMIC_OR */
> +
>  #endif /* _LINUX_ATOMIC_H */
> 
> got:
> 
> [   32.803704] cpu_dev_init done
> [   99.171292] memory_dev_init done

So the difference between these two is noise, I hope.  Adding a static
inline function that is not used should not have an effect on performance.
Still, the difference between 6 seconds and 60 seconds rises far above
this noise level, so the big differences are likely quite real.

						Thanx, Paul