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

> > > >         .fqs            = NULL,
> > > >         .stats          = NULL,
> > > >         .irq_capable    = 1,
> > >
> > >
> > > > --- a/kernel/rcu/tasks.h
> > > > +++ b/kernel/rcu/tasks.h
> > >
> > > > @@ -696,16 +696,14 @@ static int __init rcu_spawn_tasks_rude_kthread(void)
> > > >  }
> > > >  core_initcall(rcu_spawn_tasks_rude_kthread);
> > > >
> > > > -#ifndef CONFIG_TINY_RCU
> > > > -static void show_rcu_tasks_rude_gp_kthread(void)
> > > > +#if !defined(CONFIG_TINY_RCU)
> > >
> > > Different #ifdef expression.
> >
> > I don't believe that it is.  The above supplies the !TINY_RCU, and a
> > prior #ifdef supplies the TASKS_RUDE_RCU.  So what am I missing here?
> 
> Sorry, you're right. I missed the outer #ifdef.
> 
> > > > +void show_rcu_tasks_rude_gp_kthread(void)
> > >
> > > Do you really want to define a non-static function...
> >
> > Yes, because its user is in kernel/rcu/rcutorture.c, which is in
> > a separate translation unit, so it must be non-static.  The earlier
> > version instead only called it from this file, but that turned out to
> > produce confusing output containing information for flavors of RCU that
> > were not under test.  So this commit exported it to allow rcutorture to
> > complain about only that RCU flavor being tested.
> >
> > > >  {
> > > >         show_rcu_tasks_generic_gp_kthread(&rcu_tasks_rude, "");
> > > >  }
> > > > -#endif /* #ifndef CONFIG_TINY_RCU */
> > > > -
> > > > -#else /* #ifdef CONFIG_TASKS_RUDE_RCU */
> > > > -static void show_rcu_tasks_rude_gp_kthread(void) {}
> > > > -#endif /* #else #ifdef CONFIG_TASKS_RUDE_RCU */
> > > > +EXPORT_SYMBOL_GPL(show_rcu_tasks_rude_gp_kthread);
> > >
> > > ... and export its symbol, from a header file?
> > > I know the file is included only once.
> >
> > Because kernel/rcu/rcutorture.c can be built as a module, it must be
> > exported.  I agree that it is unusual to export from a .h file, but the
> > single inclusion is intentional.  There are several other .h files in
> > kernel/rcu that are also split out to group similar functionality while
> > still allowing the compiler to inline to its heart's content.
> 
> My main gripe is having non-static functions in a header file, which
> causes havoc if someone ever start including it from a second source
> file.
> 
> Why not move the contents of the header to the (single) source file that
> includes the header _unconditionally_, to make it nicely self-contained?
> For conditional includes, things are obviously different.

Because I used to do it that way and continually got tripped up by
the outer #if ranges that caused you trouble above.  Only with more
#if ranges spread through a larger file, it was even more painful.
With each .h file confined to a particular topic, its #if structure is
relatively simple, though perhaps not all that helpful to people such
as yourself who are new to the file.

But still way easier to pick up than having all of the #if ranges
contained within a single file.

> > Yes, this is a bit unconventional, but it has been this way for more
> > than a decade, at least for tree_plugin.h.
> 
> Oh right, there are even more of these ;-)

Indeed there are.

							Thanx, Paul