Re: [patch 59/66] rcu: Convert rcutree to hotplug state machine

[Paul E. McKenney]

On Tue, Jul 12, 2016 at 12:57:35PM +0200, Anna-Maria Gleixner wrote:
> (edit cc: add tglx)
> 
> On Mon, 11 Jul 2016, Paul E. McKenney wrote:
> 
> > On Mon, Jul 11, 2016 at 12:29:04PM -0000, Anna-Maria Gleixner wrote:
> > > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > 
> > > Straight forward conversion to the state machine. Though the question arises
> > > whether this needs really all these state transitions to work.
> > > 
> > > Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Anna-Maria Gleixner <anna-maria@xxxxxxxxxxxxx>
> > 
> > I believe that this patch breaks !SMP builds, as it has the side effect
> > of pulling a Tree RCU include file into Tiny RCU builds.
> >
> > Some questions below, and a related patch at the end.  The related patch
> > provides exact detection of CPUs coming online, and passes light rcutorture
> > testing.
> 
> We will take it into account before this change.

Very good, thank you!

My current plan is to submit this patch to the v4.9 merge window, that
is, not the one in a few weeks, but the one after that.  Please let me
know if you need me to take a different approach.

> > The dying-idle state is still covered by direct function call, correct?
> > (The call to rcu_report_dead() from cpuhp_report_idle_dead().)
> 
> Yes.

Whew!  ;-)

> > > --- a/include/linux/rcutree.h
> > > +++ b/include/linux/rcutree.h
> > > @@ -111,4 +111,19 @@ bool rcu_is_watching(void);
> > > 
> > >  void rcu_all_qs(void);
> > > 
> > > +/* RCUtree hotplug events */
> > > +#if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
> > > +int rcutree_prepare_cpu(unsigned int cpu);
> > > +int rcutree_online_cpu(unsigned int cpu);
> > > +int rcutree_offline_cpu(unsigned int cpu);
> > > +int rcutree_dead_cpu(unsigned int cpu);
> > > +int rcutree_dying_cpu(unsigned int cpu);
> > > +#else
> > > +#define rcutree_prepare_cpu	NULL
> > > +#define rcutree_online_cpu	NULL
> > > +#define rcutree_offline_cpu	NULL
> > > +#define rcutree_dead_cpu	NULL
> > > +#define rcutree_dying_cpu	NULL
> > > +#endif
> > 
> > This file is included only in CONFIG_TREE_RCU or CONFIG_PREEMPT_RCU
> > builds, so you should not need this ifdef.
> > 
> > The only other option is CONFIG_TINY_RCU, for which CONFIG_HOTPLUG_CPU
> > cannot possibly be set.
> > 
> > > +
> > >  #endif /* __LINUX_RCUTREE_H */
> > > --- a/kernel/cpu.c
> > > +++ b/kernel/cpu.c
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/tick.h>
> > >  #include <linux/irq.h>
> > >  #include <linux/smpboot.h>
> > > +#include <linux/rcutree.h>
> > 
> > Ah, I see...  ;-)
> > 
> > I am going to guess that this code was never built for CONFIG_SMP=n...
> > I would expect a few build errors.
> > 
> > I suggest moving the #ifdef from include/linux/rcutree.h to
> > include/linux/cpu.h.  That way, you avoid including code intended
> > only for Tree RCU into Tiny RCU builds.
> >
> 
> Is it ok, to leave the defines without ifdef in
> include/linux/rcutree.h and remove the include rcutree.h in
> kernel/cpu.c ?  Because only if CONFIG_TREE_RCU or CONFIG_PREEMPT_RCU
> is defined, rcupdate.h includes rcutree.h . See delta patch below.

Yes, that would be much better!

Also, you need to put the other leg of the #ifdef (the #defines with
all the NULLs) into include/linux/rcutiny.  That way, kernel/cpu.c
will get the correct set of #defines automatically, given that
include/linux/rcupdate.h #includes one or the other of rcutree.h and
rcutiny.h.

							Thanx, Paul

> 8<----------------
> 
> --- a/include/linux/rcutree.h
> +++ b/include/linux/rcutree.h
> @@ -112,18 +112,10 @@ bool rcu_is_watching(void);
>  void rcu_all_qs(void);
> 
>  /* RCUtree hotplug events */
> -#if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
>  int rcutree_prepare_cpu(unsigned int cpu);
>  int rcutree_online_cpu(unsigned int cpu);
>  int rcutree_offline_cpu(unsigned int cpu);
>  int rcutree_dead_cpu(unsigned int cpu);
>  int rcutree_dying_cpu(unsigned int cpu);
> -#else
> -#define rcutree_prepare_cpu	NULL
> -#define rcutree_online_cpu	NULL
> -#define rcutree_offline_cpu	NULL
> -#define rcutree_dead_cpu	NULL
> -#define rcutree_dying_cpu	NULL
> -#endif
> 
>  #endif /* __LINUX_RCUTREE_H */
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -23,7 +23,6 @@
>  #include <linux/tick.h>
>  #include <linux/irq.h>
>  #include <linux/smpboot.h>
> -#include <linux/rcutree.h>
> 
>  #include <trace/events/power.h>
>  #define CREATE_TRACE_POINTS
>