[PATCH tip/core/rcu 0/4] RCU-tasks updates for v5.11

[PATCH tip/core/rcu 0/4] RCU-tasks updates for v5.11

[Paul E. McKenney]

Hello!

This series contains updates to the various RCU-tasks flavors:

1.	Make preemptible TRACE02 enable lockdep.

2.	Convert rcu_tasks_wait_gp() for-loop to while-loop.

3.	Make grace-period kthread report match RCU flavor being tested.

4.	Make the units of ->init_fract be jiffies.

						Thanx, Paul

------------------------------------------------------------------------

 kernel/rcu/rcu.h                                       |   16 +++++
 kernel/rcu/rcutorture.c                                |   11 +++
 kernel/rcu/tasks.h                                     |   49 +++++++----------
 tools/testing/selftests/rcutorture/configs/rcu/TRACE01 |    6 +-
 tools/testing/selftests/rcutorture/configs/rcu/TRACE02 |    6 +-
 5 files changed, 52 insertions(+), 36 deletions(-)

[PATCH tip/core/rcu 1/4] rcutorture: Make preemptible TRACE02 enable lockdep

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

Currently, the CONFIG_PREEMPT_NONE=y rcutorture TRACE01 rcutorture
scenario enables lockdep.  This limits its ability to find bugs due to
non-preemptible sections of code being RCU readers, and pretty much all
code thus appearing to lockdep to be an RCU reader.  This commit therefore
moves lockdep testing to the CONFIG_PREEMPT=y rcutorture TRACE02 scenario.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 tools/testing/selftests/rcutorture/configs/rcu/TRACE01 | 6 +++---
 tools/testing/selftests/rcutorture/configs/rcu/TRACE02 | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TRACE01 b/tools/testing/selftests/rcutorture/configs/rcu/TRACE01
index 12e7661..34c8ff5 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TRACE01
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TRACE01
@@ -4,8 +4,8 @@ CONFIG_HOTPLUG_CPU=y
 CONFIG_PREEMPT_NONE=y
 CONFIG_PREEMPT_VOLUNTARY=n
 CONFIG_PREEMPT=n
-CONFIG_DEBUG_LOCK_ALLOC=y
-CONFIG_PROVE_LOCKING=y
-#CHECK#CONFIG_PROVE_RCU=y
+CONFIG_DEBUG_LOCK_ALLOC=n
+CONFIG_PROVE_LOCKING=n
+#CHECK#CONFIG_PROVE_RCU=n
 CONFIG_TASKS_TRACE_RCU_READ_MB=y
 CONFIG_RCU_EXPERT=y
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TRACE02 b/tools/testing/selftests/rcutorture/configs/rcu/TRACE02
index b69ed66..77541ee 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TRACE02
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TRACE02
@@ -4,8 +4,8 @@ CONFIG_HOTPLUG_CPU=y
 CONFIG_PREEMPT_NONE=n
 CONFIG_PREEMPT_VOLUNTARY=n
 CONFIG_PREEMPT=y
-CONFIG_DEBUG_LOCK_ALLOC=n
-CONFIG_PROVE_LOCKING=n
-#CHECK#CONFIG_PROVE_RCU=n
+CONFIG_DEBUG_LOCK_ALLOC=y
+CONFIG_PROVE_LOCKING=y
+#CHECK#CONFIG_PROVE_RCU=y
 CONFIG_TASKS_TRACE_RCU_READ_MB=n
 CONFIG_RCU_EXPERT=y
-- 
2.9.5

[PATCH tip/core/rcu 2/4] rcu-tasks: Convert rcu_tasks_wait_gp() for-loop to while-loop

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

The infinite for-loop in rcu_tasks_wait_gp() has its only exit at the
top of the loop, so this commit does the straightforward conversion to
a while-loop, thus saving a few lines.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tasks.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index d5d9f2d..a93271f 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -338,14 +338,11 @@ static void rcu_tasks_wait_gp(struct rcu_tasks *rtp)
 	if (fract > HZ)
 		fract = HZ;
 
-	for (;;) {
+	while (!list_empty(&holdouts)) {
 		bool firstreport;
 		bool needreport;
 		int rtst;
 
-		if (list_empty(&holdouts))
-			break;
-
 		/* Slowly back off waiting for holdouts */
 		set_tasks_gp_state(rtp, RTGS_WAIT_SCAN_HOLDOUTS);
 		schedule_timeout_idle(HZ/fract);
-- 
2.9.5

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

[paulmck]

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>
---
 kernel/rcu/rcu.h        | 16 ++++++++++++++++
 kernel/rcu/rcutorture.c | 11 +++++++++--
 kernel/rcu/tasks.h      | 30 ++++++++++++++----------------
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index e01cba5..59ef1ae 100644
--- 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
+#if !defined(CONFIG_TINY_RCU) && defined(CONFIG_TASKS_TRACE_RCU)
+void show_rcu_tasks_trace_gp_kthread(void);
+#else
+static inline void show_rcu_tasks_trace_gp_kthread(void) {}
+#endif
+
 #endif /* __LINUX_RCU_H */
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 916ea4f..c811f23 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -317,6 +317,7 @@ struct rcu_torture_ops {
 	void (*cb_barrier)(void);
 	void (*fqs)(void);
 	void (*stats)(void);
+	void (*gp_kthread_dbg)(void);
 	int (*stall_dur)(void);
 	int irq_capable;
 	int can_boost;
@@ -466,6 +467,7 @@ static struct rcu_torture_ops rcu_ops = {
 	.cb_barrier	= rcu_barrier,
 	.fqs		= rcu_force_quiescent_state,
 	.stats		= NULL,
+	.gp_kthread_dbg	= show_rcu_gp_kthreads,
 	.stall_dur	= rcu_jiffies_till_stall_check,
 	.irq_capable	= 1,
 	.can_boost	= rcu_can_boost(),
@@ -693,6 +695,7 @@ static struct rcu_torture_ops tasks_ops = {
 	.exp_sync	= synchronize_rcu_mult_test,
 	.call		= call_rcu_tasks,
 	.cb_barrier	= rcu_barrier_tasks,
+	.gp_kthread_dbg	= show_rcu_tasks_classic_gp_kthread,
 	.fqs		= NULL,
 	.stats		= NULL,
 	.irq_capable	= 1,
@@ -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,
 	.fqs		= NULL,
 	.stats		= NULL,
 	.irq_capable	= 1,
@@ -800,6 +804,7 @@ static struct rcu_torture_ops tasks_tracing_ops = {
 	.exp_sync	= synchronize_rcu_tasks_trace,
 	.call		= call_rcu_tasks_trace,
 	.cb_barrier	= rcu_barrier_tasks_trace,
+	.gp_kthread_dbg	= show_rcu_tasks_trace_gp_kthread,
 	.fqs		= NULL,
 	.stats		= NULL,
 	.irq_capable	= 1,
@@ -1594,7 +1599,8 @@ rcu_torture_stats_print(void)
 			sched_show_task(wtp);
 			splatted = true;
 		}
-		show_rcu_gp_kthreads();
+		if (cur_ops->gp_kthread_dbg)
+			cur_ops->gp_kthread_dbg();
 		rcu_ftrace_dump(DUMP_ALL);
 	}
 	rtcv_snap = rcu_torture_current_version;
@@ -2472,7 +2478,8 @@ rcu_torture_cleanup(void)
 		return;
 	}
 
-	show_rcu_gp_kthreads();
+	if (cur_ops->gp_kthread_dbg)
+		cur_ops->gp_kthread_dbg();
 	rcu_torture_read_exit_cleanup();
 	rcu_torture_barrier_cleanup();
 	rcu_torture_fwd_prog_cleanup();
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index a93271f..0b45989 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -290,7 +290,7 @@ static void show_rcu_tasks_generic_gp_kthread(struct rcu_tasks *rtp, char *s)
 		".C"[!!data_race(rtp->cbs_head)],
 		s);
 }
-#endif /* #ifndef CONFIG_TINY_RCU */
+#endif // #ifndef CONFIG_TINY_RCU
 
 static void exit_tasks_rcu_finish_trace(struct task_struct *t);
 
@@ -568,12 +568,13 @@ static int __init rcu_spawn_tasks_kthread(void)
 }
 core_initcall(rcu_spawn_tasks_kthread);
 
-#ifndef CONFIG_TINY_RCU
-static void show_rcu_tasks_classic_gp_kthread(void)
+#if !defined(CONFIG_TINY_RCU)
+void show_rcu_tasks_classic_gp_kthread(void)
 {
 	show_rcu_tasks_generic_gp_kthread(&rcu_tasks, "");
 }
-#endif /* #ifndef CONFIG_TINY_RCU */
+EXPORT_SYMBOL_GPL(show_rcu_tasks_classic_gp_kthread);
+#endif // !defined(CONFIG_TINY_RCU)
 
 /* Do the srcu_read_lock() for the above synchronize_srcu().  */
 void exit_tasks_rcu_start(void) __acquires(&tasks_rcu_exit_srcu)
@@ -595,7 +596,6 @@ void exit_tasks_rcu_finish(void) __releases(&tasks_rcu_exit_srcu)
 }
 
 #else /* #ifdef CONFIG_TASKS_RCU */
-static inline void show_rcu_tasks_classic_gp_kthread(void) { }
 void exit_tasks_rcu_start(void) { }
 void exit_tasks_rcu_finish(void) { exit_tasks_rcu_finish_trace(current); }
 #endif /* #else #ifdef CONFIG_TASKS_RCU */
@@ -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)
+void show_rcu_tasks_rude_gp_kthread(void)
 {
 	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);
+#endif // !defined(CONFIG_TINY_RCU)
+#endif /* #ifdef CONFIG_TASKS_RUDE_RCU */
 
 ////////////////////////////////////////////////////////////////////////
 //
@@ -1199,8 +1197,8 @@ static int __init rcu_spawn_tasks_trace_kthread(void)
 }
 core_initcall(rcu_spawn_tasks_trace_kthread);
 
-#ifndef CONFIG_TINY_RCU
-static void show_rcu_tasks_trace_gp_kthread(void)
+#if !defined(CONFIG_TINY_RCU)
+void show_rcu_tasks_trace_gp_kthread(void)
 {
 	char buf[64];
 
@@ -1210,11 +1208,11 @@ static void show_rcu_tasks_trace_gp_kthread(void)
 		data_race(n_heavy_reader_attempts));
 	show_rcu_tasks_generic_gp_kthread(&rcu_tasks_trace, buf);
 }
-#endif /* #ifndef CONFIG_TINY_RCU */
+EXPORT_SYMBOL_GPL(show_rcu_tasks_trace_gp_kthread);
+#endif // !defined(CONFIG_TINY_RCU)
 
 #else /* #ifdef CONFIG_TASKS_TRACE_RCU */
 static void exit_tasks_rcu_finish_trace(struct task_struct *t) { }
-static inline void show_rcu_tasks_trace_gp_kthread(void) {}
 #endif /* #else #ifdef CONFIG_TASKS_TRACE_RCU */
 
 #ifndef CONFIG_TINY_RCU
-- 
2.9.5

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

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

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

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

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

[Geert Uytterhoeven]

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

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@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").

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

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@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").
> 
> 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

[PATCH tip/core/rcu 4/4] rcu-tasks: Make the units of ->init_fract be jiffies

[paulmck]

From: "Paul E. McKenney" <paulmck@kernel.org>

Currently, the units of ->init_fract are milliseconds while those of
->gp_sleep are jiffies.  For consistency with each other and with the
argument of schedule_timeout_idle(), this commit changes the units of
->init_fract to jiffies.

This change does affect the backoff algorithm, but only on systems where
HZ is not 1000, and even there the change makes more sense, given that the
current setup would "back off" to the same number of jiffies repeatedly.
In contrast, with this change, the number of jiffies waited increases
on each pass throught the loop in the rcu_tasks_wait_gp() function.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tasks.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 0b45989..35bdcfd 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -335,8 +335,6 @@ static void rcu_tasks_wait_gp(struct rcu_tasks *rtp)
 
 	// Start off with initial wait and slowly back off to 1 HZ wait.
 	fract = rtp->init_fract;
-	if (fract > HZ)
-		fract = HZ;
 
 	while (!list_empty(&holdouts)) {
 		bool firstreport;
@@ -345,10 +343,10 @@ static void rcu_tasks_wait_gp(struct rcu_tasks *rtp)
 
 		/* Slowly back off waiting for holdouts */
 		set_tasks_gp_state(rtp, RTGS_WAIT_SCAN_HOLDOUTS);
-		schedule_timeout_idle(HZ/fract);
+		schedule_timeout_idle(fract);
 
-		if (fract > 1)
-			fract--;
+		if (fract < HZ)
+			fract++;
 
 		rtst = READ_ONCE(rcu_task_stall_timeout);
 		needreport = rtst > 0 && time_after(jiffies, lastreport + rtst);
@@ -557,7 +555,7 @@ EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 static int __init rcu_spawn_tasks_kthread(void)
 {
 	rcu_tasks.gp_sleep = HZ / 10;
-	rcu_tasks.init_fract = 10;
+	rcu_tasks.init_fract = HZ / 10;
 	rcu_tasks.pregp_func = rcu_tasks_pregp_step;
 	rcu_tasks.pertask_func = rcu_tasks_pertask;
 	rcu_tasks.postscan_func = rcu_tasks_postscan;
@@ -1178,12 +1176,12 @@ static int __init rcu_spawn_tasks_trace_kthread(void)
 {
 	if (IS_ENABLED(CONFIG_TASKS_TRACE_RCU_READ_MB)) {
 		rcu_tasks_trace.gp_sleep = HZ / 10;
-		rcu_tasks_trace.init_fract = 10;
+		rcu_tasks_trace.init_fract = HZ / 10;
 	} else {
 		rcu_tasks_trace.gp_sleep = HZ / 200;
 		if (rcu_tasks_trace.gp_sleep <= 0)
 			rcu_tasks_trace.gp_sleep = 1;
-		rcu_tasks_trace.init_fract = HZ / 5;
+		rcu_tasks_trace.init_fract = HZ / 200;
 		if (rcu_tasks_trace.init_fract <= 0)
 			rcu_tasks_trace.init_fract = 1;
 	}
-- 
2.9.5