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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Paul,

On Tue, Dec 15, 2020 at 7:24 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> On Tue, Dec 15, 2020 at 09:40:26AM +0100, Geert Uytterhoeven wrote:
> > On Fri, Nov 6, 2020 at 12:40 AM <paulmck@kernel.org> wrote:
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > >
> > > 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@intel.com>. ]
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >
> > 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").

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

> 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 ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds