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

[Geert Uytterhoeven]

Hi Paul,

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.

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

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

> +void show_rcu_tasks_rude_gp_kthread(void)

Do you really want to define a non-static function...

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

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