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

[Anna-Maria Gleixner]

(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.

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

Yes.

> > --- 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.


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