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

[Paul E. McKenney]

On Tue, Dec 15, 2020 at 09:40:26AM +0100, Geert Uytterhoeven wrote:
> Hi Paul,

Hello, Geert, and thank you for looking this over!

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

This patch is already in mainline, but if the second approach above
works, I would welcome a patch making that change.

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

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

Yes, this is a bit unconventional, but it has been this way for more
than a decade, at least for tree_plugin.h.

Again, please let me know if I am missing something.

							Thanx, Paul

> > +#endif // !defined(CONFIG_TINY_RCU)
> > +#endif /* #ifdef CONFIG_TASKS_RUDE_RCU */
> >
> >  ////////////////////////////////////////////////////////////////////////
> >  //
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> 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