Re: [PATCH tip/core/rcu 3/4] rcutorture: Make grace-period kthread report match RCU flavor being tested

[Paul E. McKenney]

On Wed, Dec 16, 2020 at 08:48:29AM -0800, Paul E. McKenney wrote:
> On Wed, Dec 16, 2020 at 10:31:16AM +0100, Geert Uytterhoeven wrote:
> > Hi Paul,
> > 
> > On Tue, Dec 15, 2020 at 7:24 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > > On Tue, Dec 15, 2020 at 09:40:26AM +0100, Geert Uytterhoeven wrote:
> > > > On Fri, Nov 6, 2020 at 12:40 AM <paulmck@xxxxxxxxxx> wrote:
> > > > > From: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> > > > >
> > > > > At the end of the test and after rcu_torture_writer() stalls, rcutorture
> > > > > invokes show_rcu_gp_kthreads() in order to dump out information on the
> > > > > RCU grace-period kthread.  This makes a lot of sense when testing vanilla
> > > > > RCU, but not so much for the other flavors.  This commit therefore allows
> > > > > per-flavor kthread-dump functions to be specified.
> > > > >
> > > > > [ paulmck: Apply feedback from kernel test robot <lkp@xxxxxxxxx>. ]
> > > > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > >
> > > > Thanks for your patch, which is now commit 27c0f1448389baf7
> > > > ("rcutorture: Make grace-period kthread report match RCU flavor being
> > > > tested").
> > > >
> > > > > --- a/kernel/rcu/rcu.h
> > > > > +++ b/kernel/rcu/rcu.h
> > > > > @@ -533,4 +533,20 @@ static inline bool rcu_is_nocb_cpu(int cpu) { return false; }
> > > > >  static inline void rcu_bind_current_to_nocb(void) { }
> > > > >  #endif
> > > > >
> > > > > +#if !defined(CONFIG_TINY_RCU) && defined(CONFIG_TASKS_RCU)
> > > > > +void show_rcu_tasks_classic_gp_kthread(void);
> > > > > +#else
> > > > > +static inline void show_rcu_tasks_classic_gp_kthread(void) {}
> > > > > +#endif
> > > > > +#if !defined(CONFIG_TINY_RCU) && defined(CONFIG_TASKS_RUDE_RCU)
> > > > > +void show_rcu_tasks_rude_gp_kthread(void);
> > > > > +#else
> > > > > +static inline void show_rcu_tasks_rude_gp_kthread(void) {}
> > > > > +#endif
> > > >
> > > > The #ifdef expression does not match the one for the implementation
> > > > below.
> > >
> > > That does sound like something I would do...
> > >
> > > The definition of show_rcu_tasks_rude_gp_kthread() must be provided
> > > elsewhere if !TINY_RCU && TASKS_RUDE_RCU, correct?
> > >
> > > > > --- a/kernel/rcu/rcutorture.c
> > > > > +++ b/kernel/rcu/rcutorture.c
> > > >
> > > > > @@ -762,6 +765,7 @@ static struct rcu_torture_ops tasks_rude_ops = {
> > > > >         .exp_sync       = synchronize_rcu_tasks_rude,
> > > > >         .call           = call_rcu_tasks_rude,
> > > > >         .cb_barrier     = rcu_barrier_tasks_rude,
> > > > > +       .gp_kthread_dbg = show_rcu_tasks_rude_gp_kthread,
> > > >
> > > > Perhaps you just want to have a NULL pointer for the dummy case, instead
> > > > of instantiating a dummy static inline function and taking its address?
> > >
> > > You mean something like this in kernel/rcu/rcu.h?
> > >
> > > #if !defined(CONFIG_TINY_RCU) && defined(CONFIG_TASKS_RUDE_RCU)
> > > void show_rcu_tasks_rude_gp_kthread(void);
> > > #else
> > > #define show_rcu_tasks_rude_gp_kthread NULL
> > > #endif
> > >
> > > This does looks better to me, and at first glance would work.
> > 
> > Exactly. This is similar to how unimplemented PM callbacks are handled
> > (git grep "#define\s*pm_.*NULL").
> 
> OK, as shown in the (untested) patch below?

I finally got around to running the full suite after completing a couple
of long-running bisections, and yes, you got me on this one!  ;-)

Both locktorture and scftorture build with CONFIG_TINY_RCU,
CONFIG_TASKS_RCU, CONFIG_TASKS_RUDE_RCU, and CONFIG_TASKS_TRACE_RCU
all disabled.  So when the compiler attempts to build this:

	void show_rcu_tasks_gp_kthreads(void)
	{
		show_rcu_tasks_classic_gp_kthread();
		show_rcu_tasks_rude_gp_kthread();
		show_rcu_tasks_trace_gp_kthread();
	}

It sees this:

	void show_rcu_tasks_gp_kthreads(void)
	{
		NULL();
		NULL();
		NULL();
	}

Needless to say, it doesn't like this much, and refuses to suffer
in silence.

I am reverting the commit that defined these three functions to NULL.
This means that rcutorture runs still instantiate static inline
definitions of these three functions, but that does work and only takes
effect for those who are actually running rcutorture.

							Thanx, Paul