All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
@ 2020-12-09 20:27 Uladzislau Rezki (Sony)
  2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
                   ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Uladzislau Rezki (Sony) @ 2020-12-09 20:27 UTC (permalink / raw)
  To: LKML, RCU, Paul E . McKenney, Michael Ellerman
  Cc: Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o,
	Sebastian Andrzej Siewior, Uladzislau Rezki, Oleksiy Avramchenko

Initialize the RCU-tasks earlier, before *_initcall() callbacks are
invoked. Do it after the workqueue subsytem is up and running. That
gives us a possibility to make use of synchronize_rcu_tasks*() wait
API in early_initcall() callbacks.

Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 include/linux/rcupdate.h |  6 ++++++
 init/main.c              |  1 +
 kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 3c3efa4d6ab5..340c7d5344a4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -88,6 +88,12 @@ void rcu_sched_clock_irq(int user);
 void rcu_report_dead(unsigned int cpu);
 void rcutree_migrate_callbacks(int cpu);
 
+#ifdef CONFIG_TASKS_RCU_GENERIC
+void rcu_init_tasks_generic(void);
+#else
+static inline void rcu_init_tasks_generic(void) { }
+#endif
+
 #ifdef CONFIG_RCU_STALL_COMMON
 void rcu_sysrq_start(void);
 void rcu_sysrq_end(void);
diff --git a/init/main.c b/init/main.c
index 130376ec10ba..e253e87bdf58 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1502,6 +1502,7 @@ static noinline void __init kernel_init_freeable(void)
 
 	init_mm_internals();
 
+	rcu_init_tasks_generic();
 	do_pre_smp_initcalls();
 	lockup_detector_init();
 
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 35bdcfd84d42..67a162949763 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -241,7 +241,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 	}
 }
 
-/* Spawn RCU-tasks grace-period kthread, e.g., at core_initcall() time. */
+/* Spawn RCU-tasks grace-period kthread. */
 static void __init rcu_spawn_tasks_kthread_generic(struct rcu_tasks *rtp)
 {
 	struct task_struct *t;
@@ -564,7 +564,6 @@ static int __init rcu_spawn_tasks_kthread(void)
 	rcu_spawn_tasks_kthread_generic(&rcu_tasks);
 	return 0;
 }
-core_initcall(rcu_spawn_tasks_kthread);
 
 #if !defined(CONFIG_TINY_RCU)
 void show_rcu_tasks_classic_gp_kthread(void)
@@ -692,7 +691,6 @@ static int __init rcu_spawn_tasks_rude_kthread(void)
 	rcu_spawn_tasks_kthread_generic(&rcu_tasks_rude);
 	return 0;
 }
-core_initcall(rcu_spawn_tasks_rude_kthread);
 
 #if !defined(CONFIG_TINY_RCU)
 void show_rcu_tasks_rude_gp_kthread(void)
@@ -968,6 +966,12 @@ static void rcu_tasks_trace_pregp_step(void)
 static void rcu_tasks_trace_pertask(struct task_struct *t,
 				    struct list_head *hop)
 {
+	// During early boot when there is only one boot-CPU,
+	// an idle_task is not set for other CPUs. In this case
+	// just revert.
+	if (unlikely(t == NULL))
+		return;
+
 	WRITE_ONCE(t->trc_reader_special.b.need_qs, false);
 	WRITE_ONCE(t->trc_reader_checked, false);
 	t->trc_ipi_to_cpu = -1;
@@ -1193,7 +1197,6 @@ static int __init rcu_spawn_tasks_trace_kthread(void)
 	rcu_spawn_tasks_kthread_generic(&rcu_tasks_trace);
 	return 0;
 }
-core_initcall(rcu_spawn_tasks_trace_kthread);
 
 #if !defined(CONFIG_TINY_RCU)
 void show_rcu_tasks_trace_gp_kthread(void)
@@ -1222,6 +1225,21 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+void __init rcu_init_tasks_generic(void)
+{
+#ifdef CONFIG_TASKS_RCU
+	rcu_spawn_tasks_kthread();
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+	rcu_spawn_tasks_rude_kthread();
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+	rcu_spawn_tasks_trace_kthread();
+#endif
+}
+
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 static inline void rcu_tasks_bootup_oddness(void) {}
 void show_rcu_tasks_gp_kthreads(void) {}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-09 20:27 [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki (Sony)
@ 2020-12-09 20:27 ` Uladzislau Rezki (Sony)
  2020-12-16 15:49   ` Uladzislau Rezki
  2021-02-12 19:20   ` Sebastian Andrzej Siewior
  2020-12-09 20:37 ` [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki
  2020-12-10  3:26 ` Paul E. McKenney
  2 siblings, 2 replies; 32+ messages in thread
From: Uladzislau Rezki (Sony) @ 2020-12-09 20:27 UTC (permalink / raw)
  To: LKML, RCU, Paul E . McKenney, Michael Ellerman
  Cc: Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o,
	Sebastian Andrzej Siewior, Uladzislau Rezki, Oleksiy Avramchenko

Add self tests for checking of RCU-tasks API functionality.
It covers:
    - wait API functions;
    - invoking/completion call_rcu_tasks*().

Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tasks.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 67a162949763..9407772780c1 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1225,6 +1225,16 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+static struct rcu_head rhp;
+static int rcu_execurted_test_counter;
+static int rcu_run_test_counter;
+
+static void test_rcu_tasks_callback(struct rcu_head *r)
+{
+	pr_info("RCU-tasks test callback executed %d\n",
+		++rcu_execurted_test_counter);
+}
+
 void __init rcu_init_tasks_generic(void)
 {
 #ifdef CONFIG_TASKS_RCU
@@ -1238,7 +1248,41 @@ void __init rcu_init_tasks_generic(void)
 #ifdef CONFIG_TASKS_TRACE_RCU
 	rcu_spawn_tasks_trace_kthread();
 #endif
+
+	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
+		pr_info("Running RCU-tasks wait API self tests\n");
+#ifdef CONFIG_TASKS_RCU
+		rcu_run_test_counter++;
+		call_rcu_tasks(&rhp, test_rcu_tasks_callback);
+		synchronize_rcu_tasks();
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+		rcu_run_test_counter++;
+		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
+		synchronize_rcu_tasks_rude();
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+		rcu_run_test_counter++;
+		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
+		synchronize_rcu_tasks_trace();
+#endif
+	}
+}
+
+static int rcu_tasks_verify_self_tests(void)
+{
+	int ret = 0;
+
+	if (rcu_run_test_counter != rcu_execurted_test_counter) {
+		WARN_ON(1);
+		ret = -1;
+	}
+
+	return ret;
 }
+late_initcall(rcu_tasks_verify_self_tests);
 
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 static inline void rcu_tasks_bootup_oddness(void) {}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-09 20:27 [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki (Sony)
  2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
@ 2020-12-09 20:37 ` Uladzislau Rezki
  2020-12-10 13:39   ` Daniel Axtens
  2020-12-10  3:26 ` Paul E. McKenney
  2 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-09 20:37 UTC (permalink / raw)
  To: Michael Ellerman, Daniel Axtens
  Cc: LKML, RCU, Paul E . McKenney, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

> Initialize the RCU-tasks earlier, before *_initcall() callbacks are
> invoked. Do it after the workqueue subsytem is up and running. That
> gives us a possibility to make use of synchronize_rcu_tasks*() wait
> API in early_initcall() callbacks.
> 
> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  include/linux/rcupdate.h |  6 ++++++
>  init/main.c              |  1 +
>  kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
I still don't have a powerPC hw so far, even though i have sent a request
to the osuosl.org. It would be appreciated if Michael or Daniel could run
and verify it.

Thank you in advance!

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-09 20:27 [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki (Sony)
  2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
  2020-12-09 20:37 ` [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki
@ 2020-12-10  3:26 ` Paul E. McKenney
  2020-12-10 13:04   ` Uladzislau Rezki
  2 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-10  3:26 UTC (permalink / raw)
  To: Uladzislau Rezki (Sony)
  Cc: LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Wed, Dec 09, 2020 at 09:27:31PM +0100, Uladzislau Rezki (Sony) wrote:
> Initialize the RCU-tasks earlier, before *_initcall() callbacks are
> invoked. Do it after the workqueue subsytem is up and running. That
> gives us a possibility to make use of synchronize_rcu_tasks*() wait
> API in early_initcall() callbacks.
> 
> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Thank you!  I have queued both with the usual editing, so please
check and let me know if I messed something up.

							Thanx, Paul

> ---
>  include/linux/rcupdate.h |  6 ++++++
>  init/main.c              |  1 +
>  kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 3c3efa4d6ab5..340c7d5344a4 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -88,6 +88,12 @@ void rcu_sched_clock_irq(int user);
>  void rcu_report_dead(unsigned int cpu);
>  void rcutree_migrate_callbacks(int cpu);
>  
> +#ifdef CONFIG_TASKS_RCU_GENERIC
> +void rcu_init_tasks_generic(void);
> +#else
> +static inline void rcu_init_tasks_generic(void) { }
> +#endif
> +
>  #ifdef CONFIG_RCU_STALL_COMMON
>  void rcu_sysrq_start(void);
>  void rcu_sysrq_end(void);
> diff --git a/init/main.c b/init/main.c
> index 130376ec10ba..e253e87bdf58 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1502,6 +1502,7 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	init_mm_internals();
>  
> +	rcu_init_tasks_generic();
>  	do_pre_smp_initcalls();
>  	lockup_detector_init();
>  
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 35bdcfd84d42..67a162949763 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -241,7 +241,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
>  	}
>  }
>  
> -/* Spawn RCU-tasks grace-period kthread, e.g., at core_initcall() time. */
> +/* Spawn RCU-tasks grace-period kthread. */
>  static void __init rcu_spawn_tasks_kthread_generic(struct rcu_tasks *rtp)
>  {
>  	struct task_struct *t;
> @@ -564,7 +564,6 @@ static int __init rcu_spawn_tasks_kthread(void)
>  	rcu_spawn_tasks_kthread_generic(&rcu_tasks);
>  	return 0;
>  }
> -core_initcall(rcu_spawn_tasks_kthread);
>  
>  #if !defined(CONFIG_TINY_RCU)
>  void show_rcu_tasks_classic_gp_kthread(void)
> @@ -692,7 +691,6 @@ static int __init rcu_spawn_tasks_rude_kthread(void)
>  	rcu_spawn_tasks_kthread_generic(&rcu_tasks_rude);
>  	return 0;
>  }
> -core_initcall(rcu_spawn_tasks_rude_kthread);
>  
>  #if !defined(CONFIG_TINY_RCU)
>  void show_rcu_tasks_rude_gp_kthread(void)
> @@ -968,6 +966,12 @@ static void rcu_tasks_trace_pregp_step(void)
>  static void rcu_tasks_trace_pertask(struct task_struct *t,
>  				    struct list_head *hop)
>  {
> +	// During early boot when there is only one boot-CPU,
> +	// an idle_task is not set for other CPUs. In this case
> +	// just revert.
> +	if (unlikely(t == NULL))
> +		return;
> +
>  	WRITE_ONCE(t->trc_reader_special.b.need_qs, false);
>  	WRITE_ONCE(t->trc_reader_checked, false);
>  	t->trc_ipi_to_cpu = -1;
> @@ -1193,7 +1197,6 @@ static int __init rcu_spawn_tasks_trace_kthread(void)
>  	rcu_spawn_tasks_kthread_generic(&rcu_tasks_trace);
>  	return 0;
>  }
> -core_initcall(rcu_spawn_tasks_trace_kthread);
>  
>  #if !defined(CONFIG_TINY_RCU)
>  void show_rcu_tasks_trace_gp_kthread(void)
> @@ -1222,6 +1225,21 @@ void show_rcu_tasks_gp_kthreads(void)
>  }
>  #endif /* #ifndef CONFIG_TINY_RCU */
>  
> +void __init rcu_init_tasks_generic(void)
> +{
> +#ifdef CONFIG_TASKS_RCU
> +	rcu_spawn_tasks_kthread();
> +#endif
> +
> +#ifdef CONFIG_TASKS_RUDE_RCU
> +	rcu_spawn_tasks_rude_kthread();
> +#endif
> +
> +#ifdef CONFIG_TASKS_TRACE_RCU
> +	rcu_spawn_tasks_trace_kthread();
> +#endif
> +}
> +
>  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
>  static inline void rcu_tasks_bootup_oddness(void) {}
>  void show_rcu_tasks_gp_kthreads(void) {}
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-10  3:26 ` Paul E. McKenney
@ 2020-12-10 13:04   ` Uladzislau Rezki
  0 siblings, 0 replies; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-10 13:04 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki (Sony),
	LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Wed, Dec 09, 2020 at 07:26:13PM -0800, Paul E. McKenney wrote:
> On Wed, Dec 09, 2020 at 09:27:31PM +0100, Uladzislau Rezki (Sony) wrote:
> > Initialize the RCU-tasks earlier, before *_initcall() callbacks are
> > invoked. Do it after the workqueue subsytem is up and running. That
> > gives us a possibility to make use of synchronize_rcu_tasks*() wait
> > API in early_initcall() callbacks.
> > 
> > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Thank you!  I have queued both with the usual editing, so please
> check and let me know if I messed something up.
> 
Thanks! 

Will have a look at editing.

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-09 20:37 ` [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki
@ 2020-12-10 13:39   ` Daniel Axtens
  2020-12-10 17:32     ` Paul E. McKenney
  2020-12-10 18:17     ` Uladzislau Rezki
  0 siblings, 2 replies; 32+ messages in thread
From: Daniel Axtens @ 2020-12-10 13:39 UTC (permalink / raw)
  To: Uladzislau Rezki, Michael Ellerman
  Cc: LKML, RCU, Paul E . McKenney, Michael Ellerman, Andrew Morton,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

Hi Vlad,

>> Initialize the RCU-tasks earlier, before *_initcall() callbacks are
>> invoked. Do it after the workqueue subsytem is up and running. That
>> gives us a possibility to make use of synchronize_rcu_tasks*() wait
>> API in early_initcall() callbacks.
>> 
>> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Tested-by: Daniel Axtens <dja@axtens.net>

>> ---
>>  include/linux/rcupdate.h |  6 ++++++
>>  init/main.c              |  1 +
>>  kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
>>  3 files changed, 29 insertions(+), 4 deletions(-)
>> 
> I still don't have a powerPC hw so far, even though i have sent a request
> to the osuosl.org. It would be appreciated if Michael or Daniel could run
> and verify it.

Sorry it's taken me so long to get to this. Your patch fixes things for
me. Thanks!

BTW, I'm happy to see you taking on the challenge of RCU after your good
work on vmalloc - all the best with it!

Kind regards,
Daniel

>
> Thank you in advance!
>
> --
> Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-10 13:39   ` Daniel Axtens
@ 2020-12-10 17:32     ` Paul E. McKenney
  2020-12-10 18:17     ` Uladzislau Rezki
  1 sibling, 0 replies; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-10 17:32 UTC (permalink / raw)
  To: Daniel Axtens
  Cc: Uladzislau Rezki, Michael Ellerman, LKML, RCU, Andrew Morton,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Fri, Dec 11, 2020 at 12:39:21AM +1100, Daniel Axtens wrote:
> Hi Vlad,
> 
> >> Initialize the RCU-tasks earlier, before *_initcall() callbacks are
> >> invoked. Do it after the workqueue subsytem is up and running. That
> >> gives us a possibility to make use of synchronize_rcu_tasks*() wait
> >> API in early_initcall() callbacks.
> >> 
> >> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> >> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Tested-by: Daniel Axtens <dja@axtens.net>

I will apply on the next rebase, thank you!

							Thanx, Paul

> >> ---
> >>  include/linux/rcupdate.h |  6 ++++++
> >>  init/main.c              |  1 +
> >>  kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
> >>  3 files changed, 29 insertions(+), 4 deletions(-)
> >> 
> > I still don't have a powerPC hw so far, even though i have sent a request
> > to the osuosl.org. It would be appreciated if Michael or Daniel could run
> > and verify it.
> 
> Sorry it's taken me so long to get to this. Your patch fixes things for
> me. Thanks!
> 
> BTW, I'm happy to see you taking on the challenge of RCU after your good
> work on vmalloc - all the best with it!
> 
> Kind regards,
> Daniel
> 
> >
> > Thank you in advance!
> >
> > --
> > Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall()
  2020-12-10 13:39   ` Daniel Axtens
  2020-12-10 17:32     ` Paul E. McKenney
@ 2020-12-10 18:17     ` Uladzislau Rezki
  1 sibling, 0 replies; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-10 18:17 UTC (permalink / raw)
  To: Daniel Axtens
  Cc: Uladzislau Rezki, Michael Ellerman, LKML, RCU, Paul E . McKenney,
	Andrew Morton, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

Hello, Daniel.

> Hi Vlad,
> 
> >> Initialize the RCU-tasks earlier, before *_initcall() callbacks are
> >> invoked. Do it after the workqueue subsytem is up and running. That
> >> gives us a possibility to make use of synchronize_rcu_tasks*() wait
> >> API in early_initcall() callbacks.
> >> 
> >> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> >> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Tested-by: Daniel Axtens <dja@axtens.net>
> 
Thank you for checking it!

> >> ---
> >>  include/linux/rcupdate.h |  6 ++++++
> >>  init/main.c              |  1 +
> >>  kernel/rcu/tasks.h       | 26 ++++++++++++++++++++++----
> >>  3 files changed, 29 insertions(+), 4 deletions(-)
> >> 
> > I still don't have a powerPC hw so far, even though i have sent a request
> > to the osuosl.org. It would be appreciated if Michael or Daniel could run
> > and verify it.
> 
> Sorry it's taken me so long to get to this. Your patch fixes things for
> me. Thanks!
> 
No problem. I fully understand that sometimes we are limited in spare time.

>
> BTW, I'm happy to see you taking on the challenge of RCU after your good
> work on vmalloc - all the best with it!
> 
Yep, that work was quite interesting and probably not so easy, at least
in the beginning. Thanks for your supportive words and same to you
regarding your excellent KASAN work and future challenge :)

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
@ 2020-12-16 15:49   ` Uladzislau Rezki
  2020-12-16 23:29     ` Paul E. McKenney
  2021-02-12 19:20   ` Sebastian Andrzej Siewior
  1 sibling, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-16 15:49 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: LKML, RCU, Paul E . McKenney, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

> Add self tests for checking of RCU-tasks API functionality.
> It covers:
>     - wait API functions;
>     - invoking/completion call_rcu_tasks*().
> 
> Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  kernel/rcu/tasks.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 67a162949763..9407772780c1 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1225,6 +1225,16 @@ void show_rcu_tasks_gp_kthreads(void)
>  }
>  #endif /* #ifndef CONFIG_TINY_RCU */
>  
> +static struct rcu_head rhp;
> +static int rcu_execurted_test_counter;
> +static int rcu_run_test_counter;
> +
> +static void test_rcu_tasks_callback(struct rcu_head *r)
> +{
> +	pr_info("RCU-tasks test callback executed %d\n",
> +		++rcu_execurted_test_counter);
> +}
> +
>  void __init rcu_init_tasks_generic(void)
>  {
>  #ifdef CONFIG_TASKS_RCU
> @@ -1238,7 +1248,41 @@ void __init rcu_init_tasks_generic(void)
>  #ifdef CONFIG_TASKS_TRACE_RCU
>  	rcu_spawn_tasks_trace_kthread();
>  #endif
> +
> +	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
> +		pr_info("Running RCU-tasks wait API self tests\n");
> +#ifdef CONFIG_TASKS_RCU
> +		rcu_run_test_counter++;
> +		call_rcu_tasks(&rhp, test_rcu_tasks_callback);
> +		synchronize_rcu_tasks();
> +#endif
> +
> +#ifdef CONFIG_TASKS_RUDE_RCU
> +		rcu_run_test_counter++;
> +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> +		synchronize_rcu_tasks_rude();
> +#endif
> +
> +#ifdef CONFIG_TASKS_TRACE_RCU
> +		rcu_run_test_counter++;
> +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> +		synchronize_rcu_tasks_trace();
> +#endif
> +	}
> +}
> +
> +static int rcu_tasks_verify_self_tests(void)
> +{
> +	int ret = 0;
> +
> +	if (rcu_run_test_counter != rcu_execurted_test_counter) {
> +		WARN_ON(1);
> +		ret = -1;
> +	}
> +
> +	return ret;
>  }
> +late_initcall(rcu_tasks_verify_self_tests);
>  
>  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
>  static inline void rcu_tasks_bootup_oddness(void) {}
Please find a v2 of the patch that is in question. First version
uses the same rhp for all RCU flavors what is wrong. Initially
i had three different one per one flavor. But for some reason
end up with only one.


From e7c6096af5a7916f29c0b4b05e1644b3b3a6c589 Mon Sep 17 00:00:00 2001
From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Date: Wed, 9 Dec 2020 21:27:32 +0100
Subject: [PATCH v2 1/1] rcu-tasks: Add RCU-tasks self tests

This commit adds self tests for early-boot use of RCU-tasks grace periods.
It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
call_rcu_tasks()) grace-period APIs.

Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tasks.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 36607551f966..7478d912734a 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1224,6 +1224,35 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */

+struct test_desc {
+       struct rcu_head rh;
+       const char *name;
+       bool run;
+};
+
+static struct test_desc tests[] = {
+       { .name = "call_rcu_tasks()" },
+       { .name = "call_rcu_rude()"  },
+       { .name = "call_rcu_trace()" },
+};
+
+static int rcu_executed_test_counter;
+
+static void test_rcu_tasks_callback(struct rcu_head *rhp)
+{
+       int i;
+
+       pr_info("RCU-tasks test callback executed %d\n",
+               ++rcu_executed_test_counter);
+
+       for (i = 0; i < ARRAY_SIZE(tests); i++) {
+               if (rhp == &tests[i].rh) {
+                       tests[i].run = false;
+                       break;
+               }
+       }
+}
+
 void __init rcu_init_tasks_generic(void)
 {
 #ifdef CONFIG_TASKS_RCU
@@ -1237,7 +1266,47 @@ void __init rcu_init_tasks_generic(void)
 #ifdef CONFIG_TASKS_TRACE_RCU
        rcu_spawn_tasks_trace_kthread();
 #endif
+
+       // Run the self-tests.
+       if (IS_ENABLED(CONFIG_PROVE_RCU)) {
+               pr_info("Running RCU-tasks wait API self tests\n");
+#ifdef CONFIG_TASKS_RCU
+               tests[0].run = true;
+               call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
+               synchronize_rcu_tasks();
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+               tests[1].run = true;
+               call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
+               synchronize_rcu_tasks_rude();
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+               tests[2].run = true;
+               call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
+               synchronize_rcu_tasks_trace();
+#endif
+       }
+}
+
+static int rcu_tasks_verify_self_tests(void)
+{
+       int ret, i;
+
+       for (i = 0, ret = 0; i < ARRAY_SIZE(tests); i++) {
+               if (tests[i].run) {             // still hanging.
+                       pr_err("%s has been failed.\n", tests[i].name);
+                       ret = -1;
+               }
+       }
+
+       if (ret)
+               WARN_ON(1);
+
+       return ret;
 }
+late_initcall(rcu_tasks_verify_self_tests);

 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 static inline void rcu_tasks_bootup_oddness(void) {}
-- 
2.20.1

--
Vlad Rezki

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-16 15:49   ` Uladzislau Rezki
@ 2020-12-16 23:29     ` Paul E. McKenney
  2020-12-21 15:38       ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-16 23:29 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > Add self tests for checking of RCU-tasks API functionality.
> > It covers:
> >     - wait API functions;
> >     - invoking/completion call_rcu_tasks*().
> > 
> > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  kernel/rcu/tasks.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 67a162949763..9407772780c1 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1225,6 +1225,16 @@ void show_rcu_tasks_gp_kthreads(void)
> >  }
> >  #endif /* #ifndef CONFIG_TINY_RCU */
> >  
> > +static struct rcu_head rhp;
> > +static int rcu_execurted_test_counter;
> > +static int rcu_run_test_counter;
> > +
> > +static void test_rcu_tasks_callback(struct rcu_head *r)
> > +{
> > +	pr_info("RCU-tasks test callback executed %d\n",
> > +		++rcu_execurted_test_counter);
> > +}
> > +
> >  void __init rcu_init_tasks_generic(void)
> >  {
> >  #ifdef CONFIG_TASKS_RCU
> > @@ -1238,7 +1248,41 @@ void __init rcu_init_tasks_generic(void)
> >  #ifdef CONFIG_TASKS_TRACE_RCU
> >  	rcu_spawn_tasks_trace_kthread();
> >  #endif
> > +
> > +	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
> > +		pr_info("Running RCU-tasks wait API self tests\n");
> > +#ifdef CONFIG_TASKS_RCU
> > +		rcu_run_test_counter++;
> > +		call_rcu_tasks(&rhp, test_rcu_tasks_callback);
> > +		synchronize_rcu_tasks();
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_RUDE_RCU
> > +		rcu_run_test_counter++;
> > +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> > +		synchronize_rcu_tasks_rude();
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_TRACE_RCU
> > +		rcu_run_test_counter++;
> > +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> > +		synchronize_rcu_tasks_trace();
> > +#endif
> > +	}
> > +}
> > +
> > +static int rcu_tasks_verify_self_tests(void)
> > +{
> > +	int ret = 0;
> > +
> > +	if (rcu_run_test_counter != rcu_execurted_test_counter) {
> > +		WARN_ON(1);
> > +		ret = -1;
> > +	}
> > +
> > +	return ret;
> >  }
> > +late_initcall(rcu_tasks_verify_self_tests);
> >  
> >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> >  static inline void rcu_tasks_bootup_oddness(void) {}
> Please find a v2 of the patch that is in question. First version
> uses the same rhp for all RCU flavors what is wrong. Initially
> i had three different one per one flavor. But for some reason
> end up with only one.
> 
> 
> >From e7c6096af5a7916f29c0b4b05e1644b3b3a6c589 Mon Sep 17 00:00:00 2001
> From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> Date: Wed, 9 Dec 2020 21:27:32 +0100
> Subject: [PATCH v2 1/1] rcu-tasks: Add RCU-tasks self tests
> 
> This commit adds self tests for early-boot use of RCU-tasks grace periods.
> It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> call_rcu_tasks()) grace-period APIs.
> 
> Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Much improved, thank you!  A few more comments below.

> ---
>  kernel/rcu/tasks.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 36607551f966..7478d912734a 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1224,6 +1224,35 @@ void show_rcu_tasks_gp_kthreads(void)
>  }
>  #endif /* #ifndef CONFIG_TINY_RCU */
> 
> +struct test_desc {

Please use something like "struct rcu_tasks_test_desc" to help the poor
people who might need to grep for this.  Feel free to shorten it, but
please make it descriptive and thus more likely to stay unique.

> +       struct rcu_head rh;
> +       const char *name;
> +       bool run;

If you make this "bool notrun" you don't need to initialize.

> +};
> +
> +static struct test_desc tests[] = {
> +       { .name = "call_rcu_tasks()" },
> +       { .name = "call_rcu_rude()"  },
> +       { .name = "call_rcu_trace()" },
> +};
> +
> +static int rcu_executed_test_counter;
> +
> +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> +{

	struct rcu_tasks_test_desc *rttdp;

> +       int i;
> +
> +       pr_info("RCU-tasks test callback executed %d\n",
> +               ++rcu_executed_test_counter);

	rttdp = container_of(rhp, rh, struct rcu_tasks_test_desc);
	rttdp->notrun = true;

Or I suppose:

	container_of(rhp, rh, struct rcu_tasks_test_desc)->notrun = true;

Then the loop below can go away.

> +
> +       for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +               if (rhp == &tests[i].rh) {
> +                       tests[i].run = false;
> +                       break;
> +               }
> +       }
> +}
> +
>  void __init rcu_init_tasks_generic(void)
>  {
>  #ifdef CONFIG_TASKS_RCU
> @@ -1237,7 +1266,47 @@ void __init rcu_init_tasks_generic(void)
>  #ifdef CONFIG_TASKS_TRACE_RCU
>         rcu_spawn_tasks_trace_kthread();
>  #endif
> +
> +       // Run the self-tests.
> +       if (IS_ENABLED(CONFIG_PROVE_RCU)) {
> +               pr_info("Running RCU-tasks wait API self tests\n");
> +#ifdef CONFIG_TASKS_RCU
> +               tests[0].run = true;

The s/run/notrun/ allows the three initializations of .run to go away.

> +               call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
> +               synchronize_rcu_tasks();

Why not reverse the order of these two statements?  That would test
call_rcu_tasks*()'s ability to do a grace period on their own, without
help from the corresponding synchronize_rcu_tasks*().

> +#endif
> +
> +#ifdef CONFIG_TASKS_RUDE_RCU
> +               tests[1].run = true;
> +               call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
> +               synchronize_rcu_tasks_rude();
> +#endif
> +
> +#ifdef CONFIG_TASKS_TRACE_RCU
> +               tests[2].run = true;
> +               call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
> +               synchronize_rcu_tasks_trace();
> +#endif
> +       }
> +}
> +
> +static int rcu_tasks_verify_self_tests(void)
> +{
> +       int ret, i;

Why not initialize "ret" in the declaration?

> +
> +       for (i = 0, ret = 0; i < ARRAY_SIZE(tests); i++) {
> +               if (tests[i].run) {             // still hanging.
> +                       pr_err("%s has been failed.\n", tests[i].name);
> +                       ret = -1;
> +               }
> +       }
> +
> +       if (ret)
> +               WARN_ON(1);
> +
> +       return ret;
>  }
> +late_initcall(rcu_tasks_verify_self_tests);
> 
>  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
>  static inline void rcu_tasks_bootup_oddness(void) {}
> -- 
> 2.20.1

Again, much improved!

							Thanx, Paul

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-16 23:29     ` Paul E. McKenney
@ 2020-12-21 15:38       ` Uladzislau Rezki
  2020-12-21 17:18         ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-21 15:38 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > > Add self tests for checking of RCU-tasks API functionality.
> > > It covers:
> > >     - wait API functions;
> > >     - invoking/completion call_rcu_tasks*().
> > > 
> > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  kernel/rcu/tasks.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > > 
> > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > index 67a162949763..9407772780c1 100644
> > > --- a/kernel/rcu/tasks.h
> > > +++ b/kernel/rcu/tasks.h
> > > @@ -1225,6 +1225,16 @@ void show_rcu_tasks_gp_kthreads(void)
> > >  }
> > >  #endif /* #ifndef CONFIG_TINY_RCU */
> > >  
> > > +static struct rcu_head rhp;
> > > +static int rcu_execurted_test_counter;
> > > +static int rcu_run_test_counter;
> > > +
> > > +static void test_rcu_tasks_callback(struct rcu_head *r)
> > > +{
> > > +	pr_info("RCU-tasks test callback executed %d\n",
> > > +		++rcu_execurted_test_counter);
> > > +}
> > > +
> > >  void __init rcu_init_tasks_generic(void)
> > >  {
> > >  #ifdef CONFIG_TASKS_RCU
> > > @@ -1238,7 +1248,41 @@ void __init rcu_init_tasks_generic(void)
> > >  #ifdef CONFIG_TASKS_TRACE_RCU
> > >  	rcu_spawn_tasks_trace_kthread();
> > >  #endif
> > > +
> > > +	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
> > > +		pr_info("Running RCU-tasks wait API self tests\n");
> > > +#ifdef CONFIG_TASKS_RCU
> > > +		rcu_run_test_counter++;
> > > +		call_rcu_tasks(&rhp, test_rcu_tasks_callback);
> > > +		synchronize_rcu_tasks();
> > > +#endif
> > > +
> > > +#ifdef CONFIG_TASKS_RUDE_RCU
> > > +		rcu_run_test_counter++;
> > > +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> > > +		synchronize_rcu_tasks_rude();
> > > +#endif
> > > +
> > > +#ifdef CONFIG_TASKS_TRACE_RCU
> > > +		rcu_run_test_counter++;
> > > +		call_rcu_tasks_trace(&rhp, test_rcu_tasks_callback);
> > > +		synchronize_rcu_tasks_trace();
> > > +#endif
> > > +	}
> > > +}
> > > +
> > > +static int rcu_tasks_verify_self_tests(void)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (rcu_run_test_counter != rcu_execurted_test_counter) {
> > > +		WARN_ON(1);
> > > +		ret = -1;
> > > +	}
> > > +
> > > +	return ret;
> > >  }
> > > +late_initcall(rcu_tasks_verify_self_tests);
> > >  
> > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > >  static inline void rcu_tasks_bootup_oddness(void) {}
> > Please find a v2 of the patch that is in question. First version
> > uses the same rhp for all RCU flavors what is wrong. Initially
> > i had three different one per one flavor. But for some reason
> > end up with only one.
> > 
> > 
> > >From e7c6096af5a7916f29c0b4b05e1644b3b3a6c589 Mon Sep 17 00:00:00 2001
> > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > Subject: [PATCH v2 1/1] rcu-tasks: Add RCU-tasks self tests
> > 
> > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > call_rcu_tasks()) grace-period APIs.
> > 
> > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Much improved, thank you!  A few more comments below.
> 
> > ---
> >  kernel/rcu/tasks.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 36607551f966..7478d912734a 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1224,6 +1224,35 @@ void show_rcu_tasks_gp_kthreads(void)
> >  }
> >  #endif /* #ifndef CONFIG_TINY_RCU */
> > 
> > +struct test_desc {
> 
> Please use something like "struct rcu_tasks_test_desc" to help the poor
> people who might need to grep for this.  Feel free to shorten it, but
> please make it descriptive and thus more likely to stay unique.
> 
> > +       struct rcu_head rh;
> > +       const char *name;
> > +       bool run;
> 
> If you make this "bool notrun" you don't need to initialize.
> 
> > +};
> > +
> > +static struct test_desc tests[] = {
> > +       { .name = "call_rcu_tasks()" },
> > +       { .name = "call_rcu_rude()"  },
> > +       { .name = "call_rcu_trace()" },
> > +};
> > +
> > +static int rcu_executed_test_counter;
> > +
> > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > +{
> 
> 	struct rcu_tasks_test_desc *rttdp;
> 
> > +       int i;
> > +
> > +       pr_info("RCU-tasks test callback executed %d\n",
> > +               ++rcu_executed_test_counter);
> 
> 	rttdp = container_of(rhp, rh, struct rcu_tasks_test_desc);
> 	rttdp->notrun = true;
> 
> Or I suppose:
> 
> 	container_of(rhp, rh, struct rcu_tasks_test_desc)->notrun = true;
> 
> Then the loop below can go away.
> 
> > +
> > +       for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > +               if (rhp == &tests[i].rh) {
> > +                       tests[i].run = false;
> > +                       break;
> > +               }
> > +       }
> > +}
> > +
> >  void __init rcu_init_tasks_generic(void)
> >  {
> >  #ifdef CONFIG_TASKS_RCU
> > @@ -1237,7 +1266,47 @@ void __init rcu_init_tasks_generic(void)
> >  #ifdef CONFIG_TASKS_TRACE_RCU
> >         rcu_spawn_tasks_trace_kthread();
> >  #endif
> > +
> > +       // Run the self-tests.
> > +       if (IS_ENABLED(CONFIG_PROVE_RCU)) {
> > +               pr_info("Running RCU-tasks wait API self tests\n");
> > +#ifdef CONFIG_TASKS_RCU
> > +               tests[0].run = true;
> 
> The s/run/notrun/ allows the three initializations of .run to go away.
> 
> > +               call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
> > +               synchronize_rcu_tasks();
> 
> Why not reverse the order of these two statements?  That would test
> call_rcu_tasks*()'s ability to do a grace period on their own, without
> help from the corresponding synchronize_rcu_tasks*().
> 
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_RUDE_RCU
> > +               tests[1].run = true;
> > +               call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
> > +               synchronize_rcu_tasks_rude();
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_TRACE_RCU
> > +               tests[2].run = true;
> > +               call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
> > +               synchronize_rcu_tasks_trace();
> > +#endif
> > +       }
> > +}
> > +
> > +static int rcu_tasks_verify_self_tests(void)
> > +{
> > +       int ret, i;
> 
> Why not initialize "ret" in the declaration?
> 
> > +
> > +       for (i = 0, ret = 0; i < ARRAY_SIZE(tests); i++) {
> > +               if (tests[i].run) {             // still hanging.
> > +                       pr_err("%s has been failed.\n", tests[i].name);
> > +                       ret = -1;
> > +               }
> > +       }
> > +
> > +       if (ret)
> > +               WARN_ON(1);
> > +
> > +       return ret;
> >  }
> > +late_initcall(rcu_tasks_verify_self_tests);
> > 
> >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> >  static inline void rcu_tasks_bootup_oddness(void) {}
> > -- 
> > 2.20.1
> 
> Again, much improved!
> 
See below the v3 version. I hope i fixed all comments :)

From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Date: Wed, 9 Dec 2020 21:27:32 +0100
Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests

This commit adds self tests for early-boot use of RCU-tasks grace periods.
It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
call_rcu_tasks()) grace-period APIs.

Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tasks.h | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 36607551f966..670212ed9648 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1224,6 +1224,43 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+struct rcu_tasks_test_desc {
+	struct rcu_head rh;
+	const char *name;
+	bool notrun;
+};
+
+static struct rcu_tasks_test_desc tests[] = {
+	{
+		.name = "call_rcu_tasks()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_rude()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_trace()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
+	}
+};
+
+static int rcu_executed_test_counter;
+
+static void test_rcu_tasks_callback(struct rcu_head *rhp)
+{
+	struct rcu_tasks_test_desc *rttd =
+		container_of(rhp, struct rcu_tasks_test_desc, rh);
+
+	pr_info("RCU-tasks test callback executed %d\n",
+		++rcu_executed_test_counter);
+
+	rttd->notrun = true;
+}
+
 void __init rcu_init_tasks_generic(void)
 {
 #ifdef CONFIG_TASKS_RCU
@@ -1237,7 +1274,45 @@ void __init rcu_init_tasks_generic(void)
 #ifdef CONFIG_TASKS_TRACE_RCU
 	rcu_spawn_tasks_trace_kthread();
 #endif
+
+	// Run the self-tests.
+	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
+		pr_info("Running RCU-tasks wait API self tests\n");
+#ifdef CONFIG_TASKS_RCU
+		synchronize_rcu_tasks();
+		call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+		synchronize_rcu_tasks_rude();
+		call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+		synchronize_rcu_tasks_trace();
+		call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
+#endif
+	}
+}
+
+static int rcu_tasks_verify_self_tests(void)
+{
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (!tests[i].notrun) {		// still hanging.
+			pr_err("%s has been failed.\n", tests[i].name);
+			ret = -1;
+		}
+	}
+
+	if (ret)
+		WARN_ON(1);
+
+	return ret;
 }
+late_initcall(rcu_tasks_verify_self_tests);
 
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 static inline void rcu_tasks_bootup_oddness(void) {}
-- 
2.20.1

--
Vlad Rezki

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 15:38       ` Uladzislau Rezki
@ 2020-12-21 17:18         ` Paul E. McKenney
  2020-12-21 18:45           ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-21 17:18 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:

[ . . . ]

> > > 2.20.1
> > 
> > Again, much improved!
> > 
> See below the v3 version. I hope i fixed all comments :)
> 
> >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> Date: Wed, 9 Dec 2020 21:27:32 +0100
> Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> 
> This commit adds self tests for early-boot use of RCU-tasks grace periods.
> It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> call_rcu_tasks()) grace-period APIs.
> 
> Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Much better!

I pulled this in, but made one small additional change.  Please let me
know if this is problematic.

							Thanx, Paul

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

commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Dec 9 21:27:32 2020 +0100

    rcu-tasks: Add RCU-tasks self tests
    
    This commit adds self tests for early-boot use of RCU-tasks grace periods.
    It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
    both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
    call_rcu_tasks()) grace-period APIs.
    
    Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
    
    Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 3660755..35a2cd5 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+struct rcu_tasks_test_desc {
+	struct rcu_head rh;
+	const char *name;
+	bool notrun;
+};
+
+static struct rcu_tasks_test_desc tests[] = {
+	{
+		.name = "call_rcu_tasks()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_rude()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_trace()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
+	}
+};
+
+static void test_rcu_tasks_callback(struct rcu_head *rhp)
+{
+	struct rcu_tasks_test_desc *rttd =
+		container_of(rhp, struct rcu_tasks_test_desc, rh);
+
+	pr_info("Callback from %s invoked.\n", rttd->name);
+
+	rttd->notrun = true;
+}
+
 void __init rcu_init_tasks_generic(void)
 {
 #ifdef CONFIG_TASKS_RCU
@@ -1237,7 +1271,45 @@ void __init rcu_init_tasks_generic(void)
 #ifdef CONFIG_TASKS_TRACE_RCU
 	rcu_spawn_tasks_trace_kthread();
 #endif
+
+	// Run the self-tests.
+	if (IS_ENABLED(CONFIG_PROVE_RCU)) {
+		pr_info("Running RCU-tasks wait API self tests\n");
+#ifdef CONFIG_TASKS_RCU
+		synchronize_rcu_tasks();
+		call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+		synchronize_rcu_tasks_rude();
+		call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+		synchronize_rcu_tasks_trace();
+		call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
+#endif
+	}
+}
+
+static int rcu_tasks_verify_self_tests(void)
+{
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (!tests[i].notrun) {		// still hanging.
+			pr_err("%s has been failed.\n", tests[i].name);
+			ret = -1;
+		}
+	}
+
+	if (ret)
+		WARN_ON(1);
+
+	return ret;
 }
+late_initcall(rcu_tasks_verify_self_tests);
 
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 static inline void rcu_tasks_bootup_oddness(void) {}

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 17:18         ` Paul E. McKenney
@ 2020-12-21 18:45           ` Uladzislau Rezki
  2020-12-21 19:29             ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-21 18:45 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 09:18:05AM -0800, Paul E. McKenney wrote:
> On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> > On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> 
> [ . . . ]
> 
> > > > 2.20.1
> > > 
> > > Again, much improved!
> > > 
> > See below the v3 version. I hope i fixed all comments :)
> > 
> > >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> > 
> > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > call_rcu_tasks()) grace-period APIs.
> > 
> > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Much better!
> 
> I pulled this in, but made one small additional change.  Please let me
> know if this is problematic.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Wed Dec 9 21:27:32 2020 +0100
> 
>     rcu-tasks: Add RCU-tasks self tests
>     
>     This commit adds self tests for early-boot use of RCU-tasks grace periods.
>     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
>     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
>     call_rcu_tasks()) grace-period APIs.
>     
>     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
>     
>     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 3660755..35a2cd5 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
>  }
>  #endif /* #ifndef CONFIG_TINY_RCU */
>  
> +struct rcu_tasks_test_desc {
> +	struct rcu_head rh;
> +	const char *name;
> +	bool notrun;
> +};
> +
> +static struct rcu_tasks_test_desc tests[] = {
> +	{
> +		.name = "call_rcu_tasks()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> +	},
> +	{
> +		.name = "call_rcu_tasks_rude()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> +	},
> +	{
> +		.name = "call_rcu_tasks_trace()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> +	}
> +};
> +
> +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> +{
> +	struct rcu_tasks_test_desc *rttd =
> +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> +
> +	pr_info("Callback from %s invoked.\n", rttd->name);
That is fine! We can output the name instead of executed counter.
Doing so makes it completely clear who triggers the callback.

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 18:45           ` Uladzislau Rezki
@ 2020-12-21 19:29             ` Paul E. McKenney
  2020-12-21 19:48               ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-21 19:29 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 07:45:39PM +0100, Uladzislau Rezki wrote:
> On Mon, Dec 21, 2020 at 09:18:05AM -0800, Paul E. McKenney wrote:
> > On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> > > On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > > > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > 
> > [ . . . ]
> > 
> > > > > 2.20.1
> > > > 
> > > > Again, much improved!
> > > > 
> > > See below the v3 version. I hope i fixed all comments :)
> > > 
> > > >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> > > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > > Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> > > 
> > > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > call_rcu_tasks()) grace-period APIs.
> > > 
> > > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > 
> > Much better!
> > 
> > I pulled this in, but made one small additional change.  Please let me
> > know if this is problematic.
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
> > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > Date:   Wed Dec 9 21:27:32 2020 +0100
> > 
> >     rcu-tasks: Add RCU-tasks self tests
> >     
> >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> >     call_rcu_tasks()) grace-period APIs.
> >     
> >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> >     
> >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 3660755..35a2cd5 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
> >  }
> >  #endif /* #ifndef CONFIG_TINY_RCU */
> >  
> > +struct rcu_tasks_test_desc {
> > +	struct rcu_head rh;
> > +	const char *name;
> > +	bool notrun;
> > +};
> > +
> > +static struct rcu_tasks_test_desc tests[] = {
> > +	{
> > +		.name = "call_rcu_tasks()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > +	},
> > +	{
> > +		.name = "call_rcu_tasks_rude()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > +	},
> > +	{
> > +		.name = "call_rcu_tasks_trace()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > +	}
> > +};
> > +
> > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > +{
> > +	struct rcu_tasks_test_desc *rttd =
> > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > +
> > +	pr_info("Callback from %s invoked.\n", rttd->name);
> That is fine! We can output the name instead of executed counter.
> Doing so makes it completely clear who triggers the callback.

And we also need to make it not trigger when CONFIG_PROVE_RCU=n.
While in the area, we might as well leave anything that is needed only
by CONFIG_PROVE_RCU=y undefined when CONFIG_PROVE_RCU=n.

How about the following?

							Thanx, Paul

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

commit f7a1ac0d3504e0518745da7f98573c1b13587f3e
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Dec 9 21:27:32 2020 +0100

    rcu-tasks: Add RCU-tasks self tests
    
    This commit adds self tests for early-boot use of RCU-tasks grace periods.
    It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
    both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
    call_rcu_tasks()) grace-period APIs.
    
    Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
    
    Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
    [ paulmck: Handle CONFIG_PROVE_RCU=n and identify test cases' callbacks. ]
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 3660755..af7c194 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1224,6 +1224,82 @@ void show_rcu_tasks_gp_kthreads(void)
 }
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+#ifdef CONFIG_PROVE_RCU
+struct rcu_tasks_test_desc {
+	struct rcu_head rh;
+	const char *name;
+	bool notrun;
+};
+
+static struct rcu_tasks_test_desc tests[] = {
+	{
+		.name = "call_rcu_tasks()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_rude()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
+	},
+	{
+		.name = "call_rcu_tasks_trace()",
+		/* If not defined, the test is skipped. */
+		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
+	}
+};
+
+static void test_rcu_tasks_callback(struct rcu_head *rhp)
+{
+	struct rcu_tasks_test_desc *rttd =
+		container_of(rhp, struct rcu_tasks_test_desc, rh);
+
+	pr_info("Callback from %s invoked.\n", rttd->name);
+
+	rttd->notrun = true;
+}
+
+static void rcu_tasks_initiate_self_tests(void)
+{
+	pr_info("Running RCU-tasks wait API self tests\n");
+#ifdef CONFIG_TASKS_RCU
+	synchronize_rcu_tasks();
+	call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_RUDE_RCU
+	synchronize_rcu_tasks_rude();
+	call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
+#endif
+
+#ifdef CONFIG_TASKS_TRACE_RCU
+	synchronize_rcu_tasks_trace();
+	call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
+#endif
+}
+
+static int rcu_tasks_verify_self_tests(void)
+{
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (!tests[i].notrun) {		// still hanging.
+			pr_err("%s has been failed.\n", tests[i].name);
+			ret = -1;
+		}
+	}
+
+	if (ret)
+		WARN_ON(1);
+
+	return ret;
+}
+late_initcall(rcu_tasks_verify_self_tests);
+#else /* #ifdef CONFIG_PROVE_RCU */
+static void rcu_tasks_initiate_self_tests(void) { }
+#endif /* #else #ifdef CONFIG_PROVE_RCU */
+
 void __init rcu_init_tasks_generic(void)
 {
 #ifdef CONFIG_TASKS_RCU
@@ -1237,6 +1313,9 @@ void __init rcu_init_tasks_generic(void)
 #ifdef CONFIG_TASKS_TRACE_RCU
 	rcu_spawn_tasks_trace_kthread();
 #endif
+
+	// Run the self-tests.
+	rcu_tasks_initiate_self_tests();
 }
 
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 19:29             ` Paul E. McKenney
@ 2020-12-21 19:48               ` Uladzislau Rezki
  2020-12-21 20:45                 ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-21 19:48 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 11:29:06AM -0800, Paul E. McKenney wrote:
> On Mon, Dec 21, 2020 at 07:45:39PM +0100, Uladzislau Rezki wrote:
> > On Mon, Dec 21, 2020 at 09:18:05AM -0800, Paul E. McKenney wrote:
> > > On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> > > > On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > > > > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > > 
> > > [ . . . ]
> > > 
> > > > > > 2.20.1
> > > > > 
> > > > > Again, much improved!
> > > > > 
> > > > See below the v3 version. I hope i fixed all comments :)
> > > > 
> > > > >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> > > > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > > > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > > > Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> > > > 
> > > > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > > call_rcu_tasks()) grace-period APIs.
> > > > 
> > > > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > > 
> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > 
> > > Much better!
> > > 
> > > I pulled this in, but made one small additional change.  Please let me
> > > know if this is problematic.
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
> > > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > Date:   Wed Dec 9 21:27:32 2020 +0100
> > > 
> > >     rcu-tasks: Add RCU-tasks self tests
> > >     
> > >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > >     call_rcu_tasks()) grace-period APIs.
> > >     
> > >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > >     
> > >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > 
> > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > index 3660755..35a2cd5 100644
> > > --- a/kernel/rcu/tasks.h
> > > +++ b/kernel/rcu/tasks.h
> > > @@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
> > >  }
> > >  #endif /* #ifndef CONFIG_TINY_RCU */
> > >  
> > > +struct rcu_tasks_test_desc {
> > > +	struct rcu_head rh;
> > > +	const char *name;
> > > +	bool notrun;
> > > +};
> > > +
> > > +static struct rcu_tasks_test_desc tests[] = {
> > > +	{
> > > +		.name = "call_rcu_tasks()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > > +	},
> > > +	{
> > > +		.name = "call_rcu_tasks_rude()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > > +	},
> > > +	{
> > > +		.name = "call_rcu_tasks_trace()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > > +	}
> > > +};
> > > +
> > > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > +{
> > > +	struct rcu_tasks_test_desc *rttd =
> > > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > > +
> > > +	pr_info("Callback from %s invoked.\n", rttd->name);
> > That is fine! We can output the name instead of executed counter.
> > Doing so makes it completely clear who triggers the callback.
> 
> And we also need to make it not trigger when CONFIG_PROVE_RCU=n.
> While in the area, we might as well leave anything that is needed only
> by CONFIG_PROVE_RCU=y undefined when CONFIG_PROVE_RCU=n.
> 
> How about the following?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit f7a1ac0d3504e0518745da7f98573c1b13587f3e
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Wed Dec 9 21:27:32 2020 +0100
> 
>     rcu-tasks: Add RCU-tasks self tests
>     
>     This commit adds self tests for early-boot use of RCU-tasks grace periods.
>     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
>     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
>     call_rcu_tasks()) grace-period APIs.
>     
>     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
>     
>     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
>     [ paulmck: Handle CONFIG_PROVE_RCU=n and identify test cases' callbacks. ]
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 3660755..af7c194 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1224,6 +1224,82 @@ void show_rcu_tasks_gp_kthreads(void)
>  }
>  #endif /* #ifndef CONFIG_TINY_RCU */
>  
> +#ifdef CONFIG_PROVE_RCU
> +struct rcu_tasks_test_desc {
> +	struct rcu_head rh;
> +	const char *name;
> +	bool notrun;
> +};
> +
> +static struct rcu_tasks_test_desc tests[] = {
> +	{
> +		.name = "call_rcu_tasks()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> +	},
> +	{
> +		.name = "call_rcu_tasks_rude()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> +	},
> +	{
> +		.name = "call_rcu_tasks_trace()",
> +		/* If not defined, the test is skipped. */
> +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> +	}
> +};
> +
> +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> +{
> +	struct rcu_tasks_test_desc *rttd =
> +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> +
> +	pr_info("Callback from %s invoked.\n", rttd->name);
> +
> +	rttd->notrun = true;
> +}
> +
> +static void rcu_tasks_initiate_self_tests(void)
> +{
> +	pr_info("Running RCU-tasks wait API self tests\n");
> +#ifdef CONFIG_TASKS_RCU
> +	synchronize_rcu_tasks();
> +	call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
> +#endif
> +
> +#ifdef CONFIG_TASKS_RUDE_RCU
> +	synchronize_rcu_tasks_rude();
> +	call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
> +#endif
> +
> +#ifdef CONFIG_TASKS_TRACE_RCU
> +	synchronize_rcu_tasks_trace();
> +	call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
> +#endif
> +}
> +
> +static int rcu_tasks_verify_self_tests(void)
> +{
> +	int ret = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		if (!tests[i].notrun) {		// still hanging.
> +			pr_err("%s has been failed.\n", tests[i].name);
> +			ret = -1;
> +		}
> +	}
> +
> +	if (ret)
> +		WARN_ON(1);
> +
> +	return ret;
> +}
> +late_initcall(rcu_tasks_verify_self_tests);
> +#else /* #ifdef CONFIG_PROVE_RCU */
> +static void rcu_tasks_initiate_self_tests(void) { }
> +#endif /* #else #ifdef CONFIG_PROVE_RCU */
> +
>  void __init rcu_init_tasks_generic(void)
>  {
>  #ifdef CONFIG_TASKS_RCU
> @@ -1237,6 +1313,9 @@ void __init rcu_init_tasks_generic(void)
>  #ifdef CONFIG_TASKS_TRACE_RCU
>  	rcu_spawn_tasks_trace_kthread();
>  #endif
> +
> +	// Run the self-tests.
> +	rcu_tasks_initiate_self_tests();
>  }
>  
>  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
That makes sense to me. I missed that point. There is no
reason in wasting of extra cycles which affect a boot up
time if built without CONFIG_PROVE_RCU.

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 19:48               ` Uladzislau Rezki
@ 2020-12-21 20:45                 ` Paul E. McKenney
  2020-12-21 21:28                   ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2020-12-21 20:45 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: LKML, RCU, Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 08:48:48PM +0100, Uladzislau Rezki wrote:
> On Mon, Dec 21, 2020 at 11:29:06AM -0800, Paul E. McKenney wrote:
> > On Mon, Dec 21, 2020 at 07:45:39PM +0100, Uladzislau Rezki wrote:
> > > On Mon, Dec 21, 2020 at 09:18:05AM -0800, Paul E. McKenney wrote:
> > > > On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> > > > > On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > > > > > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > > 2.20.1
> > > > > > 
> > > > > > Again, much improved!
> > > > > > 
> > > > > See below the v3 version. I hope i fixed all comments :)
> > > > > 
> > > > > >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> > > > > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > > > > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > > > > Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> > > > > 
> > > > > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > > > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > > > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > > > call_rcu_tasks()) grace-period APIs.
> > > > > 
> > > > > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > > > 
> > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > 
> > > > Much better!
> > > > 
> > > > I pulled this in, but made one small additional change.  Please let me
> > > > know if this is problematic.
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > ------------------------------------------------------------------------
> > > > 
> > > > commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
> > > > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > Date:   Wed Dec 9 21:27:32 2020 +0100
> > > > 
> > > >     rcu-tasks: Add RCU-tasks self tests
> > > >     
> > > >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > >     call_rcu_tasks()) grace-period APIs.
> > > >     
> > > >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > >     
> > > >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > 
> > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > index 3660755..35a2cd5 100644
> > > > --- a/kernel/rcu/tasks.h
> > > > +++ b/kernel/rcu/tasks.h
> > > > @@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
> > > >  }
> > > >  #endif /* #ifndef CONFIG_TINY_RCU */
> > > >  
> > > > +struct rcu_tasks_test_desc {
> > > > +	struct rcu_head rh;
> > > > +	const char *name;
> > > > +	bool notrun;
> > > > +};
> > > > +
> > > > +static struct rcu_tasks_test_desc tests[] = {
> > > > +	{
> > > > +		.name = "call_rcu_tasks()",
> > > > +		/* If not defined, the test is skipped. */
> > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > > > +	},
> > > > +	{
> > > > +		.name = "call_rcu_tasks_rude()",
> > > > +		/* If not defined, the test is skipped. */
> > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > > > +	},
> > > > +	{
> > > > +		.name = "call_rcu_tasks_trace()",
> > > > +		/* If not defined, the test is skipped. */
> > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > > > +	}
> > > > +};
> > > > +
> > > > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > > +{
> > > > +	struct rcu_tasks_test_desc *rttd =
> > > > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > > > +
> > > > +	pr_info("Callback from %s invoked.\n", rttd->name);
> > > That is fine! We can output the name instead of executed counter.
> > > Doing so makes it completely clear who triggers the callback.
> > 
> > And we also need to make it not trigger when CONFIG_PROVE_RCU=n.
> > While in the area, we might as well leave anything that is needed only
> > by CONFIG_PROVE_RCU=y undefined when CONFIG_PROVE_RCU=n.
> > 
> > How about the following?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit f7a1ac0d3504e0518745da7f98573c1b13587f3e
> > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > Date:   Wed Dec 9 21:27:32 2020 +0100
> > 
> >     rcu-tasks: Add RCU-tasks self tests
> >     
> >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> >     call_rcu_tasks()) grace-period APIs.
> >     
> >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> >     
> >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> >     [ paulmck: Handle CONFIG_PROVE_RCU=n and identify test cases' callbacks. ]
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 3660755..af7c194 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1224,6 +1224,82 @@ void show_rcu_tasks_gp_kthreads(void)
> >  }
> >  #endif /* #ifndef CONFIG_TINY_RCU */
> >  
> > +#ifdef CONFIG_PROVE_RCU
> > +struct rcu_tasks_test_desc {
> > +	struct rcu_head rh;
> > +	const char *name;
> > +	bool notrun;
> > +};
> > +
> > +static struct rcu_tasks_test_desc tests[] = {
> > +	{
> > +		.name = "call_rcu_tasks()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > +	},
> > +	{
> > +		.name = "call_rcu_tasks_rude()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > +	},
> > +	{
> > +		.name = "call_rcu_tasks_trace()",
> > +		/* If not defined, the test is skipped. */
> > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > +	}
> > +};
> > +
> > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > +{
> > +	struct rcu_tasks_test_desc *rttd =
> > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > +
> > +	pr_info("Callback from %s invoked.\n", rttd->name);
> > +
> > +	rttd->notrun = true;
> > +}
> > +
> > +static void rcu_tasks_initiate_self_tests(void)
> > +{
> > +	pr_info("Running RCU-tasks wait API self tests\n");
> > +#ifdef CONFIG_TASKS_RCU
> > +	synchronize_rcu_tasks();
> > +	call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_RUDE_RCU
> > +	synchronize_rcu_tasks_rude();
> > +	call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
> > +#endif
> > +
> > +#ifdef CONFIG_TASKS_TRACE_RCU
> > +	synchronize_rcu_tasks_trace();
> > +	call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
> > +#endif
> > +}
> > +
> > +static int rcu_tasks_verify_self_tests(void)
> > +{
> > +	int ret = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > +		if (!tests[i].notrun) {		// still hanging.
> > +			pr_err("%s has been failed.\n", tests[i].name);
> > +			ret = -1;
> > +		}
> > +	}
> > +
> > +	if (ret)
> > +		WARN_ON(1);
> > +
> > +	return ret;
> > +}
> > +late_initcall(rcu_tasks_verify_self_tests);
> > +#else /* #ifdef CONFIG_PROVE_RCU */
> > +static void rcu_tasks_initiate_self_tests(void) { }
> > +#endif /* #else #ifdef CONFIG_PROVE_RCU */
> > +
> >  void __init rcu_init_tasks_generic(void)
> >  {
> >  #ifdef CONFIG_TASKS_RCU
> > @@ -1237,6 +1313,9 @@ void __init rcu_init_tasks_generic(void)
> >  #ifdef CONFIG_TASKS_TRACE_RCU
> >  	rcu_spawn_tasks_trace_kthread();
> >  #endif
> > +
> > +	// Run the self-tests.
> > +	rcu_tasks_initiate_self_tests();
> >  }
> >  
> >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> That makes sense to me. I missed that point. There is no
> reason in wasting of extra cycles which affect a boot up
> time if built without CONFIG_PROVE_RCU.

If CONFIG_PROVE_RCU=n, then rcu_tasks_initiate_self_tests is an empty
function.  So the compiler should be able to eliminate all runtime
overhead from rcu_tasks_initiate_self_tests() when CONFIG_PROVE_RCU=n.

Or am I missing your point?

							Thanx, Paul

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-21 20:45                 ` Paul E. McKenney
@ 2020-12-21 21:28                   ` Uladzislau Rezki
  0 siblings, 0 replies; 32+ messages in thread
From: Uladzislau Rezki @ 2020-12-21 21:28 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Sebastian Andrzej Siewior,
	Oleksiy Avramchenko

On Mon, Dec 21, 2020 at 12:45:13PM -0800, Paul E. McKenney wrote:
> On Mon, Dec 21, 2020 at 08:48:48PM +0100, Uladzislau Rezki wrote:
> > On Mon, Dec 21, 2020 at 11:29:06AM -0800, Paul E. McKenney wrote:
> > > On Mon, Dec 21, 2020 at 07:45:39PM +0100, Uladzislau Rezki wrote:
> > > > On Mon, Dec 21, 2020 at 09:18:05AM -0800, Paul E. McKenney wrote:
> > > > > On Mon, Dec 21, 2020 at 04:38:09PM +0100, Uladzislau Rezki wrote:
> > > > > > On Wed, Dec 16, 2020 at 03:29:55PM -0800, Paul E. McKenney wrote:
> > > > > > > On Wed, Dec 16, 2020 at 04:49:59PM +0100, Uladzislau Rezki wrote:
> > > > > 
> > > > > [ . . . ]
> > > > > 
> > > > > > > > 2.20.1
> > > > > > > 
> > > > > > > Again, much improved!
> > > > > > > 
> > > > > > See below the v3 version. I hope i fixed all comments :)
> > > > > > 
> > > > > > >From 06f7adfd84cbb1994d0e2693ee9dcdfd272a9bd0 Mon Sep 17 00:00:00 2001
> > > > > > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > > > > > Date: Wed, 9 Dec 2020 21:27:32 +0100
> > > > > > Subject: [PATCH v3 1/1] rcu-tasks: Add RCU-tasks self tests
> > > > > > 
> > > > > > This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > > > > It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > > > > both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > > > > call_rcu_tasks()) grace-period APIs.
> > > > > > 
> > > > > > Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > > > > 
> > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > 
> > > > > Much better!
> > > > > 
> > > > > I pulled this in, but made one small additional change.  Please let me
> > > > > know if this is problematic.
> > > > > 
> > > > > 							Thanx, Paul
> > > > > 
> > > > > ------------------------------------------------------------------------
> > > > > 
> > > > > commit 93372198b5c9efdfd288aa3b3ee41c1f90866886
> > > > > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > Date:   Wed Dec 9 21:27:32 2020 +0100
> > > > > 
> > > > >     rcu-tasks: Add RCU-tasks self tests
> > > > >     
> > > > >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > > > >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > > > >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > > > >     call_rcu_tasks()) grace-period APIs.
> > > > >     
> > > > >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > > > >     
> > > > >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > 
> > > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > > index 3660755..35a2cd5 100644
> > > > > --- a/kernel/rcu/tasks.h
> > > > > +++ b/kernel/rcu/tasks.h
> > > > > @@ -1224,6 +1224,40 @@ void show_rcu_tasks_gp_kthreads(void)
> > > > >  }
> > > > >  #endif /* #ifndef CONFIG_TINY_RCU */
> > > > >  
> > > > > +struct rcu_tasks_test_desc {
> > > > > +	struct rcu_head rh;
> > > > > +	const char *name;
> > > > > +	bool notrun;
> > > > > +};
> > > > > +
> > > > > +static struct rcu_tasks_test_desc tests[] = {
> > > > > +	{
> > > > > +		.name = "call_rcu_tasks()",
> > > > > +		/* If not defined, the test is skipped. */
> > > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > > > > +	},
> > > > > +	{
> > > > > +		.name = "call_rcu_tasks_rude()",
> > > > > +		/* If not defined, the test is skipped. */
> > > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > > > > +	},
> > > > > +	{
> > > > > +		.name = "call_rcu_tasks_trace()",
> > > > > +		/* If not defined, the test is skipped. */
> > > > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > > > > +	}
> > > > > +};
> > > > > +
> > > > > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > > > +{
> > > > > +	struct rcu_tasks_test_desc *rttd =
> > > > > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > > > > +
> > > > > +	pr_info("Callback from %s invoked.\n", rttd->name);
> > > > That is fine! We can output the name instead of executed counter.
> > > > Doing so makes it completely clear who triggers the callback.
> > > 
> > > And we also need to make it not trigger when CONFIG_PROVE_RCU=n.
> > > While in the area, we might as well leave anything that is needed only
> > > by CONFIG_PROVE_RCU=y undefined when CONFIG_PROVE_RCU=n.
> > > 
> > > How about the following?
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > commit f7a1ac0d3504e0518745da7f98573c1b13587f3e
> > > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > Date:   Wed Dec 9 21:27:32 2020 +0100
> > > 
> > >     rcu-tasks: Add RCU-tasks self tests
> > >     
> > >     This commit adds self tests for early-boot use of RCU-tasks grace periods.
> > >     It tests all three variants (Rude, Tasks, and Tasks Trace) and covers
> > >     both synchronous (e.g., synchronize_rcu_tasks()) and asynchronous (e.g.,
> > >     call_rcu_tasks()) grace-period APIs.
> > >     
> > >     Self-tests are run only in kernels built with CONFIG_PROVE_RCU=y.
> > >     
> > >     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > >     [ paulmck: Handle CONFIG_PROVE_RCU=n and identify test cases' callbacks. ]
> > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > 
> > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > index 3660755..af7c194 100644
> > > --- a/kernel/rcu/tasks.h
> > > +++ b/kernel/rcu/tasks.h
> > > @@ -1224,6 +1224,82 @@ void show_rcu_tasks_gp_kthreads(void)
> > >  }
> > >  #endif /* #ifndef CONFIG_TINY_RCU */
> > >  
> > > +#ifdef CONFIG_PROVE_RCU
> > > +struct rcu_tasks_test_desc {
> > > +	struct rcu_head rh;
> > > +	const char *name;
> > > +	bool notrun;
> > > +};
> > > +
> > > +static struct rcu_tasks_test_desc tests[] = {
> > > +	{
> > > +		.name = "call_rcu_tasks()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RCU),
> > > +	},
> > > +	{
> > > +		.name = "call_rcu_tasks_rude()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_RUDE_RCU),
> > > +	},
> > > +	{
> > > +		.name = "call_rcu_tasks_trace()",
> > > +		/* If not defined, the test is skipped. */
> > > +		.notrun = !IS_ENABLED(CONFIG_TASKS_TRACE_RCU)
> > > +	}
> > > +};
> > > +
> > > +static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > +{
> > > +	struct rcu_tasks_test_desc *rttd =
> > > +		container_of(rhp, struct rcu_tasks_test_desc, rh);
> > > +
> > > +	pr_info("Callback from %s invoked.\n", rttd->name);
> > > +
> > > +	rttd->notrun = true;
> > > +}
> > > +
> > > +static void rcu_tasks_initiate_self_tests(void)
> > > +{
> > > +	pr_info("Running RCU-tasks wait API self tests\n");
> > > +#ifdef CONFIG_TASKS_RCU
> > > +	synchronize_rcu_tasks();
> > > +	call_rcu_tasks(&tests[0].rh, test_rcu_tasks_callback);
> > > +#endif
> > > +
> > > +#ifdef CONFIG_TASKS_RUDE_RCU
> > > +	synchronize_rcu_tasks_rude();
> > > +	call_rcu_tasks_rude(&tests[1].rh, test_rcu_tasks_callback);
> > > +#endif
> > > +
> > > +#ifdef CONFIG_TASKS_TRACE_RCU
> > > +	synchronize_rcu_tasks_trace();
> > > +	call_rcu_tasks_trace(&tests[2].rh, test_rcu_tasks_callback);
> > > +#endif
> > > +}
> > > +
> > > +static int rcu_tasks_verify_self_tests(void)
> > > +{
> > > +	int ret = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > > +		if (!tests[i].notrun) {		// still hanging.
> > > +			pr_err("%s has been failed.\n", tests[i].name);
> > > +			ret = -1;
> > > +		}
> > > +	}
> > > +
> > > +	if (ret)
> > > +		WARN_ON(1);
> > > +
> > > +	return ret;
> > > +}
> > > +late_initcall(rcu_tasks_verify_self_tests);
> > > +#else /* #ifdef CONFIG_PROVE_RCU */
> > > +static void rcu_tasks_initiate_self_tests(void) { }
> > > +#endif /* #else #ifdef CONFIG_PROVE_RCU */
> > > +
> > >  void __init rcu_init_tasks_generic(void)
> > >  {
> > >  #ifdef CONFIG_TASKS_RCU
> > > @@ -1237,6 +1313,9 @@ void __init rcu_init_tasks_generic(void)
> > >  #ifdef CONFIG_TASKS_TRACE_RCU
> > >  	rcu_spawn_tasks_trace_kthread();
> > >  #endif
> > > +
> > > +	// Run the self-tests.
> > > +	rcu_tasks_initiate_self_tests();
> > >  }
> > >  
> > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > That makes sense to me. I missed that point. There is no
> > reason in wasting of extra cycles which affect a boot up
> > time if built without CONFIG_PROVE_RCU.
> 
> If CONFIG_PROVE_RCU=n, then rcu_tasks_initiate_self_tests is an empty
> function.  So the compiler should be able to eliminate all runtime
> overhead from rcu_tasks_initiate_self_tests() when CONFIG_PROVE_RCU=n.
> 
> Or am I missing your point?
> 
That is correct, i mean your description. I wanted to underline
that the late_initcall(rcu_tasks_verify_self_tests); was called
even for CONFIG_PROVE_RCU=n, what would affect a boot time with
disabled option. Of-course that extra time would be negligible.
From the other hand, why we should introduce it if it can be
avoided.

Your last change fixes that :)

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
  2020-12-16 15:49   ` Uladzislau Rezki
@ 2021-02-12 19:20   ` Sebastian Andrzej Siewior
  2021-02-12 21:12     ` Uladzislau Rezki
  1 sibling, 1 reply; 32+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-12 19:20 UTC (permalink / raw)
  To: Uladzislau Rezki (Sony)
  Cc: LKML, RCU, Paul E . McKenney, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Oleksiy Avramchenko

On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> Add self tests for checking of RCU-tasks API functionality.
> It covers:
>     - wait API functions;
>     - invoking/completion call_rcu_tasks*().
> 
> Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.

I just bisected to this commit. By booting with `threadirqs' I end up
with:
[    0.176533] Running RCU-tasks wait API self tests

No stall warning or so.
It boots again with:

diff --git a/init/main.c b/init/main.c
--- a/init/main.c
+++ b/init/main.c
@@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
 	fput(file);
 }
 
+void rcu_tasks_initiate_self_tests(void);
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
 
 	rcu_init_tasks_generic();
 	do_pre_smp_initcalls();
+	rcu_tasks_initiate_self_tests();
 	lockup_detector_init();
 
 	smp_init();
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
 	rttd->notrun = true;
 }
 
-static void rcu_tasks_initiate_self_tests(void)
+void rcu_tasks_initiate_self_tests(void)
 {
 	pr_info("Running RCU-tasks wait API self tests\n");
 #ifdef CONFIG_TASKS_RCU
@@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
 #endif
 
 	// Run the self-tests.
-	rcu_tasks_initiate_self_tests();
 }
 
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */

> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Sebastian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-12 19:20   ` Sebastian Andrzej Siewior
@ 2021-02-12 21:12     ` Uladzislau Rezki
  2021-02-12 23:48       ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2021-02-12 21:12 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Paul E . McKenney
  Cc: Uladzislau Rezki (Sony),
	LKML, RCU, Paul E . McKenney, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Oleksiy Avramchenko

On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > Add self tests for checking of RCU-tasks API functionality.
> > It covers:
> >     - wait API functions;
> >     - invoking/completion call_rcu_tasks*().
> > 
> > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> 
> I just bisected to this commit. By booting with `threadirqs' I end up
> with:
> [    0.176533] Running RCU-tasks wait API self tests
> 
> No stall warning or so.
> It boots again with:
> 
> diff --git a/init/main.c b/init/main.c
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
>  	fput(file);
>  }
>  
> +void rcu_tasks_initiate_self_tests(void);
>  static noinline void __init kernel_init_freeable(void)
>  {
>  	/*
> @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	rcu_init_tasks_generic();
>  	do_pre_smp_initcalls();
> +	rcu_tasks_initiate_self_tests();
>  	lockup_detector_init();
>  
>  	smp_init();
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
>  	rttd->notrun = true;
>  }
>  
> -static void rcu_tasks_initiate_self_tests(void)
> +void rcu_tasks_initiate_self_tests(void)
>  {
>  	pr_info("Running RCU-tasks wait API self tests\n");
>  #ifdef CONFIG_TASKS_RCU
> @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
>  #endif
>  
>  	// Run the self-tests.
> -	rcu_tasks_initiate_self_tests();
>  }
>  
>  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Sebastian
>
We should be able to use call_rcu_tasks() in the *initcall() callbacks.
The problem is that, ksoftirqd threads are not spawned by the time when
an rcu_init_tasks_generic() is invoked:

diff --git a/init/main.c b/init/main.c
index c68d784376ca..e6106bb12b2d 100644
--- a/init/main.c
+++ b/init/main.c
@@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 	rcu_init_nohz();
 	init_timers();
 	hrtimers_init();
-	softirq_init();
 	timekeeping_init();
 
 	/*
@@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
 
 	init_mm_internals();
 
+	softirq_init();
 	rcu_init_tasks_generic();
 	do_pre_smp_initcalls();
 	lockup_detector_init();
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 9d71046ea247..cafa55c496d0 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -630,6 +630,7 @@ void __init softirq_init(void)
 			&per_cpu(tasklet_hi_vec, cpu).head;
 	}
 
+	spawn_ksoftirqd();
 	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
 	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
 }
@@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
 
 	return 0;
 }
-early_initcall(spawn_ksoftirqd);
 
 /*
  * [ These __weak aliases are kept in a separate compilation unit, so that

Any thoughts?

--
Vlad Rezki

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-12 21:12     ` Uladzislau Rezki
@ 2021-02-12 23:48       ` Paul E. McKenney
  2021-02-13  0:37         ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-12 23:48 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Sebastian Andrzej Siewior, LKML, RCU, Michael Ellerman,
	Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o, Oleksiy Avramchenko

On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > Add self tests for checking of RCU-tasks API functionality.
> > > It covers:
> > >     - wait API functions;
> > >     - invoking/completion call_rcu_tasks*().
> > > 
> > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > 
> > I just bisected to this commit. By booting with `threadirqs' I end up
> > with:
> > [    0.176533] Running RCU-tasks wait API self tests
> > 
> > No stall warning or so.
> > It boots again with:
> > 
> > diff --git a/init/main.c b/init/main.c
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> >  	fput(file);
> >  }
> >  
> > +void rcu_tasks_initiate_self_tests(void);
> >  static noinline void __init kernel_init_freeable(void)
> >  {
> >  	/*
> > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> >  
> >  	rcu_init_tasks_generic();
> >  	do_pre_smp_initcalls();
> > +	rcu_tasks_initiate_self_tests();
> >  	lockup_detector_init();
> >  
> >  	smp_init();
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> >  	rttd->notrun = true;
> >  }
> >  
> > -static void rcu_tasks_initiate_self_tests(void)
> > +void rcu_tasks_initiate_self_tests(void)
> >  {
> >  	pr_info("Running RCU-tasks wait API self tests\n");
> >  #ifdef CONFIG_TASKS_RCU
> > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> >  #endif
> >  
> >  	// Run the self-tests.
> > -	rcu_tasks_initiate_self_tests();
> >  }
> >  
> >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Apologies for the hassle!  My testing clearly missed this combination
of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(

But at least I can easily reproduce this hang as follows:

tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make

Sadly, I cannot take your patch because that simply papers over the
fact that early boot use of synchronize_rcu_tasks() is broken in this
particular configuration, which will likely eventually bite others now
that init_kprobes() has been moved earlier in boot:

1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")

> > Sebastian
> >
> We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> The problem is that, ksoftirqd threads are not spawned by the time when
> an rcu_init_tasks_generic() is invoked:
> 
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..e6106bb12b2d 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>  	rcu_init_nohz();
>  	init_timers();
>  	hrtimers_init();
> -	softirq_init();
>  	timekeeping_init();
>  
>  	/*
> @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	init_mm_internals();
>  
> +	softirq_init();
>  	rcu_init_tasks_generic();
>  	do_pre_smp_initcalls();
>  	lockup_detector_init();
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 9d71046ea247..cafa55c496d0 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -630,6 +630,7 @@ void __init softirq_init(void)
>  			&per_cpu(tasklet_hi_vec, cpu).head;
>  	}
>  
> +	spawn_ksoftirqd();

We need a forward reference to allow this to build, but with that added,
my test case passes.  Good show!

>  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
>  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
>  }
> @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
>  
>  	return 0;
>  }
> -early_initcall(spawn_ksoftirqd);
>  
>  /*
>   * [ These __weak aliases are kept in a separate compilation unit, so that
> 
> Any thoughts?

One likely problem is that there are almost certainly parts of the kernel
that need softirq_init() to stay roughly where it is.  So, is it possible
to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
to be invoked just before rcu_init_tasks_generic() is called?

For my part, I will look into what is required to make Tasks RCU do
without softirq during boot, for example, by looking carefully at where
in boot RCU grace periods are unconditionally expedited.  Just in case
adjusting softirq has unforeseen side effects.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-12 23:48       ` Paul E. McKenney
@ 2021-02-13  0:37         ` Paul E. McKenney
  2021-02-13  0:43           ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-13  0:37 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Sebastian Andrzej Siewior, LKML, RCU, Michael Ellerman,
	Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o, Oleksiy Avramchenko

On Fri, Feb 12, 2021 at 03:48:51PM -0800, Paul E. McKenney wrote:
> On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> > On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > > Add self tests for checking of RCU-tasks API functionality.
> > > > It covers:
> > > >     - wait API functions;
> > > >     - invoking/completion call_rcu_tasks*().
> > > > 
> > > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > 
> > > I just bisected to this commit. By booting with `threadirqs' I end up
> > > with:
> > > [    0.176533] Running RCU-tasks wait API self tests
> > > 
> > > No stall warning or so.
> > > It boots again with:
> > > 
> > > diff --git a/init/main.c b/init/main.c
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> > >  	fput(file);
> > >  }
> > >  
> > > +void rcu_tasks_initiate_self_tests(void);
> > >  static noinline void __init kernel_init_freeable(void)
> > >  {
> > >  	/*
> > > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> > >  
> > >  	rcu_init_tasks_generic();
> > >  	do_pre_smp_initcalls();
> > > +	rcu_tasks_initiate_self_tests();
> > >  	lockup_detector_init();
> > >  
> > >  	smp_init();
> > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > --- a/kernel/rcu/tasks.h
> > > +++ b/kernel/rcu/tasks.h
> > > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > >  	rttd->notrun = true;
> > >  }
> > >  
> > > -static void rcu_tasks_initiate_self_tests(void)
> > > +void rcu_tasks_initiate_self_tests(void)
> > >  {
> > >  	pr_info("Running RCU-tasks wait API self tests\n");
> > >  #ifdef CONFIG_TASKS_RCU
> > > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> > >  #endif
> > >  
> > >  	// Run the self-tests.
> > > -	rcu_tasks_initiate_self_tests();
> > >  }
> > >  
> > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > > 
> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Apologies for the hassle!  My testing clearly missed this combination
> of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(
> 
> But at least I can easily reproduce this hang as follows:
> 
> tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> 
> Sadly, I cannot take your patch because that simply papers over the
> fact that early boot use of synchronize_rcu_tasks() is broken in this
> particular configuration, which will likely eventually bite others now
> that init_kprobes() has been moved earlier in boot:
> 
> 1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
> Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> 
> > > Sebastian
> > >
> > We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> > The problem is that, ksoftirqd threads are not spawned by the time when
> > an rcu_init_tasks_generic() is invoked:
> > 
> > diff --git a/init/main.c b/init/main.c
> > index c68d784376ca..e6106bb12b2d 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> >  	rcu_init_nohz();
> >  	init_timers();
> >  	hrtimers_init();
> > -	softirq_init();
> >  	timekeeping_init();
> >  
> >  	/*
> > @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
> >  
> >  	init_mm_internals();
> >  
> > +	softirq_init();
> >  	rcu_init_tasks_generic();
> >  	do_pre_smp_initcalls();
> >  	lockup_detector_init();
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index 9d71046ea247..cafa55c496d0 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -630,6 +630,7 @@ void __init softirq_init(void)
> >  			&per_cpu(tasklet_hi_vec, cpu).head;
> >  	}
> >  
> > +	spawn_ksoftirqd();
> 
> We need a forward reference to allow this to build, but with that added,
> my test case passes.  Good show!
> 
> >  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
> >  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
> >  }
> > @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
> >  
> >  	return 0;
> >  }
> > -early_initcall(spawn_ksoftirqd);
> >  
> >  /*
> >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > 
> > Any thoughts?
> 
> One likely problem is that there are almost certainly parts of the kernel
> that need softirq_init() to stay roughly where it is.  So, is it possible
> to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
> to be invoked just before rcu_init_tasks_generic() is called?

This still seems worth trying (and doing so is next on my list), but just
in case there are objections to it, please see below.  The advantage
of the approach below is that it renders irrelevant the exact point at
which ksoftirqd is spawned.  The advantage of moving spawn_ksoftirqd()
earlier is that it avoids adding checks to the softirq-handler code.

Me, I prefer the robustness of the approach below, but the important
thing is of course to solve the problem one way or another.

> For my part, I will look into what is required to make Tasks RCU do
> without softirq during boot, for example, by looking carefully at where
> in boot RCU grace periods are unconditionally expedited.  Just in case
> adjusting softirq has unforeseen side effects.

I should hasten to add that my test case sets tree.use_softirq=0, so
it is not RCU_SOFTIRQ that is lacking, but rather one of the others.
TIMER_SOFTIRQ?

Given that, maybe the code that switches from softirq to ksoftirqd
is doing so too early at boot.  And the patch below allows my test
case to complete successfully.  I hope this patch is uncontroversial.
After all, unless I am suffering a massive failure of imagination, it
is rather difficult to wake up kthreads that have not yet been created,
which is what current mainline looks to be doing.

There are three places invoking wakeup_softirqd(), and this patch
deals only with the first two.  They suffice thus far, and I would like
to hear what others think before I pull something like irq-work into
raise_softirq_irqoff().  :-/

Thoughts?

							Thanx, Paul

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

commit be5b0b6d9c8f22b86ea5591885bf38987ef777d9
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Fri Feb 12 16:20:40 2021 -0800

    softirq: Don't try waking ksoftirqd before it has been spawned
    
    If there is heavy softirq activity, the softirq system will attempt
    to awaken ksoftirqd and will stop the traditional back-of-interrupt
    softirq processing.  This is all well and good, but only if the
    ksoftirqd kthreads already exist, which is not the case during early
    boot, in which case the system hangs.
    
    One reproducer is as follows:
    
    tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
    
    This commit therefore adds a couple of existence checks for ksoftirqd
    and forces back-of-interrupt softirq processing when ksoftirqd does not
    yet exist.  With this change, the above test passes.
    
    Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    Reported-by: Uladzislau Rezki <urezki@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 9d71046..ba78e63 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -209,7 +209,7 @@ static inline void invoke_softirq(void)
 	if (ksoftirqd_running(local_softirq_pending()))
 		return;
 
-	if (!force_irqthreads) {
+	if (!force_irqthreads || !__this_cpu_read(ksoftirqd)) {
 #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
 		/*
 		 * We can safely execute softirq on the current stack if
@@ -358,8 +358,8 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 
 	pending = local_softirq_pending();
 	if (pending) {
-		if (time_before(jiffies, end) && !need_resched() &&
-		    --max_restart)
+		if (!__this_cpu_read(ksoftirqd) ||
+		    (time_before(jiffies, end) && !need_resched() && --max_restart))
 			goto restart;
 
 		wakeup_softirqd();

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-13  0:37         ` Paul E. McKenney
@ 2021-02-13  0:43           ` Paul E. McKenney
  2021-02-13 11:30             ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-13  0:43 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Sebastian Andrzej Siewior, LKML, RCU, Michael Ellerman,
	Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o, Oleksiy Avramchenko

On Fri, Feb 12, 2021 at 04:37:09PM -0800, Paul E. McKenney wrote:
> On Fri, Feb 12, 2021 at 03:48:51PM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> > > On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > > > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > > > Add self tests for checking of RCU-tasks API functionality.
> > > > > It covers:
> > > > >     - wait API functions;
> > > > >     - invoking/completion call_rcu_tasks*().
> > > > > 
> > > > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > > 
> > > > I just bisected to this commit. By booting with `threadirqs' I end up
> > > > with:
> > > > [    0.176533] Running RCU-tasks wait API self tests
> > > > 
> > > > No stall warning or so.
> > > > It boots again with:
> > > > 
> > > > diff --git a/init/main.c b/init/main.c
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> > > >  	fput(file);
> > > >  }
> > > >  
> > > > +void rcu_tasks_initiate_self_tests(void);
> > > >  static noinline void __init kernel_init_freeable(void)
> > > >  {
> > > >  	/*
> > > > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> > > >  
> > > >  	rcu_init_tasks_generic();
> > > >  	do_pre_smp_initcalls();
> > > > +	rcu_tasks_initiate_self_tests();
> > > >  	lockup_detector_init();
> > > >  
> > > >  	smp_init();
> > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > --- a/kernel/rcu/tasks.h
> > > > +++ b/kernel/rcu/tasks.h
> > > > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > >  	rttd->notrun = true;
> > > >  }
> > > >  
> > > > -static void rcu_tasks_initiate_self_tests(void)
> > > > +void rcu_tasks_initiate_self_tests(void)
> > > >  {
> > > >  	pr_info("Running RCU-tasks wait API self tests\n");
> > > >  #ifdef CONFIG_TASKS_RCU
> > > > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> > > >  #endif
> > > >  
> > > >  	// Run the self-tests.
> > > > -	rcu_tasks_initiate_self_tests();
> > > >  }
> > > >  
> > > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > > > 
> > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > 
> > Apologies for the hassle!  My testing clearly missed this combination
> > of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(
> > 
> > But at least I can easily reproduce this hang as follows:
> > 
> > tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> > 
> > Sadly, I cannot take your patch because that simply papers over the
> > fact that early boot use of synchronize_rcu_tasks() is broken in this
> > particular configuration, which will likely eventually bite others now
> > that init_kprobes() has been moved earlier in boot:
> > 
> > 1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
> > Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
> > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> > 
> > > > Sebastian
> > > >
> > > We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> > > The problem is that, ksoftirqd threads are not spawned by the time when
> > > an rcu_init_tasks_generic() is invoked:
> > > 
> > > diff --git a/init/main.c b/init/main.c
> > > index c68d784376ca..e6106bb12b2d 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > >  	rcu_init_nohz();
> > >  	init_timers();
> > >  	hrtimers_init();
> > > -	softirq_init();
> > >  	timekeeping_init();
> > >  
> > >  	/*
> > > @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
> > >  
> > >  	init_mm_internals();
> > >  
> > > +	softirq_init();
> > >  	rcu_init_tasks_generic();
> > >  	do_pre_smp_initcalls();
> > >  	lockup_detector_init();
> > > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > > index 9d71046ea247..cafa55c496d0 100644
> > > --- a/kernel/softirq.c
> > > +++ b/kernel/softirq.c
> > > @@ -630,6 +630,7 @@ void __init softirq_init(void)
> > >  			&per_cpu(tasklet_hi_vec, cpu).head;
> > >  	}
> > >  
> > > +	spawn_ksoftirqd();
> > 
> > We need a forward reference to allow this to build, but with that added,
> > my test case passes.  Good show!
> > 
> > >  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
> > >  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
> > >  }
> > > @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
> > >  
> > >  	return 0;
> > >  }
> > > -early_initcall(spawn_ksoftirqd);
> > >  
> > >  /*
> > >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > > 
> > > Any thoughts?
> > 
> > One likely problem is that there are almost certainly parts of the kernel
> > that need softirq_init() to stay roughly where it is.  So, is it possible
> > to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
> > to be invoked just before rcu_init_tasks_generic() is called?
> 
> This still seems worth trying (and doing so is next on my list), but just

And the patch below takes this approach, which also causes the tests to
pass.

Thoughts?

								Thanx, Paul

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

commit f4cd768e341486655c8c196e1f2b48a4463541f3
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Fri Feb 12 16:41:05 2021 -0800

    softirq: Don't try waking ksoftirqd before it has been spawned
    
    If there is heavy softirq activity, the softirq system will attempt
    to awaken ksoftirqd and will stop the traditional back-of-interrupt
    softirq processing.  This is all well and good, but only if the
    ksoftirqd kthreads already exist, which is not the case during early
    boot, in which case the system hangs.
    
    One reproducer is as follows:
    
    tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
    
    This commit therefore moves the spawning of the ksoftirqd kthreads
    earlier in boot.  With this change, the above test passes.
    
    Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    Reported-by: Uladzislau Rezki <urezki@gmail.com>
    Inspired-by: Uladzislau Rezki <urezki@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index bb8ff90..283a02d 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -592,6 +592,8 @@ static inline struct task_struct *this_cpu_ksoftirqd(void)
 	return this_cpu_read(ksoftirqd);
 }
 
+int spawn_ksoftirqd(void);
+
 /* Tasklets --- multithreaded analogue of BHs.
 
    This API is deprecated. Please consider using threaded IRQs instead:
diff --git a/init/main.c b/init/main.c
index c68d784..99835bb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1512,6 +1512,7 @@ static noinline void __init kernel_init_freeable(void)
 
 	init_mm_internals();
 
+	spawn_ksoftirqd();
 	rcu_init_tasks_generic();
 	do_pre_smp_initcalls();
 	lockup_detector_init();
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 9d71046..45d50d4 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -724,7 +724,7 @@ static struct smp_hotplug_thread softirq_threads = {
 	.thread_comm		= "ksoftirqd/%u",
 };
 
-static __init int spawn_ksoftirqd(void)
+__init int spawn_ksoftirqd(void)
 {
 	cpuhp_setup_state_nocalls(CPUHP_SOFTIRQ_DEAD, "softirq:dead", NULL,
 				  takeover_tasklets);
@@ -732,7 +732,6 @@ static __init int spawn_ksoftirqd(void)
 
 	return 0;
 }
-early_initcall(spawn_ksoftirqd);
 
 /*
  * [ These __weak aliases are kept in a separate compilation unit, so that

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-13  0:43           ` Paul E. McKenney
@ 2021-02-13 11:30             ` Uladzislau Rezki
  2021-02-13 16:45               ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2021-02-13 11:30 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Uladzislau Rezki, Sebastian Andrzej Siewior, LKML, RCU,
	Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Oleksiy Avramchenko

On Fri, Feb 12, 2021 at 04:43:28PM -0800, Paul E. McKenney wrote:
> On Fri, Feb 12, 2021 at 04:37:09PM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 12, 2021 at 03:48:51PM -0800, Paul E. McKenney wrote:
> > > On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> > > > On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > > > > Add self tests for checking of RCU-tasks API functionality.
> > > > > > It covers:
> > > > > >     - wait API functions;
> > > > > >     - invoking/completion call_rcu_tasks*().
> > > > > > 
> > > > > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > > > 
> > > > > I just bisected to this commit. By booting with `threadirqs' I end up
> > > > > with:
> > > > > [    0.176533] Running RCU-tasks wait API self tests
> > > > > 
> > > > > No stall warning or so.
> > > > > It boots again with:
> > > > > 
> > > > > diff --git a/init/main.c b/init/main.c
> > > > > --- a/init/main.c
> > > > > +++ b/init/main.c
> > > > > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> > > > >  	fput(file);
> > > > >  }
> > > > >  
> > > > > +void rcu_tasks_initiate_self_tests(void);
> > > > >  static noinline void __init kernel_init_freeable(void)
> > > > >  {
> > > > >  	/*
> > > > > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> > > > >  
> > > > >  	rcu_init_tasks_generic();
> > > > >  	do_pre_smp_initcalls();
> > > > > +	rcu_tasks_initiate_self_tests();
> > > > >  	lockup_detector_init();
> > > > >  
> > > > >  	smp_init();
> > > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > > --- a/kernel/rcu/tasks.h
> > > > > +++ b/kernel/rcu/tasks.h
> > > > > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > > >  	rttd->notrun = true;
> > > > >  }
> > > > >  
> > > > > -static void rcu_tasks_initiate_self_tests(void)
> > > > > +void rcu_tasks_initiate_self_tests(void)
> > > > >  {
> > > > >  	pr_info("Running RCU-tasks wait API self tests\n");
> > > > >  #ifdef CONFIG_TASKS_RCU
> > > > > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> > > > >  #endif
> > > > >  
> > > > >  	// Run the self-tests.
> > > > > -	rcu_tasks_initiate_self_tests();
> > > > >  }
> > > > >  
> > > > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > > > > 
> > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > 
> > > Apologies for the hassle!  My testing clearly missed this combination
> > > of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(
> > > 
> > > But at least I can easily reproduce this hang as follows:
> > > 
> > > tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> > > 
> > > Sadly, I cannot take your patch because that simply papers over the
> > > fact that early boot use of synchronize_rcu_tasks() is broken in this
> > > particular configuration, which will likely eventually bite others now
> > > that init_kprobes() has been moved earlier in boot:
> > > 
> > > 1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
> > > Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
> > > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> > > 
> > > > > Sebastian
> > > > >
> > > > We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> > > > The problem is that, ksoftirqd threads are not spawned by the time when
> > > > an rcu_init_tasks_generic() is invoked:
> > > > 
> > > > diff --git a/init/main.c b/init/main.c
> > > > index c68d784376ca..e6106bb12b2d 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > > @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > > >  	rcu_init_nohz();
> > > >  	init_timers();
> > > >  	hrtimers_init();
> > > > -	softirq_init();
> > > >  	timekeeping_init();
> > > >  
> > > >  	/*
> > > > @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
> > > >  
> > > >  	init_mm_internals();
> > > >  
> > > > +	softirq_init();
> > > >  	rcu_init_tasks_generic();
> > > >  	do_pre_smp_initcalls();
> > > >  	lockup_detector_init();
> > > > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > > > index 9d71046ea247..cafa55c496d0 100644
> > > > --- a/kernel/softirq.c
> > > > +++ b/kernel/softirq.c
> > > > @@ -630,6 +630,7 @@ void __init softirq_init(void)
> > > >  			&per_cpu(tasklet_hi_vec, cpu).head;
> > > >  	}
> > > >  
> > > > +	spawn_ksoftirqd();
> > > 
> > > We need a forward reference to allow this to build, but with that added,
> > > my test case passes.  Good show!
> > > 
> > > >  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
> > > >  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
> > > >  }
> > > > @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -early_initcall(spawn_ksoftirqd);
> > > >  
> > > >  /*
> > > >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > > > 
> > > > Any thoughts?
> > > 
> > > One likely problem is that there are almost certainly parts of the kernel
> > > that need softirq_init() to stay roughly where it is.  So, is it possible
> > > to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
> > > to be invoked just before rcu_init_tasks_generic() is called?
> > 
> > This still seems worth trying (and doing so is next on my list), but just
> 
> And the patch below takes this approach, which also causes the tests to
> pass.
> 
> Thoughts?
> 
> 								Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit f4cd768e341486655c8c196e1f2b48a4463541f3
> Author: Paul E. McKenney <paulmck@kernel.org>
> Date:   Fri Feb 12 16:41:05 2021 -0800
> 
>     softirq: Don't try waking ksoftirqd before it has been spawned
>     
>     If there is heavy softirq activity, the softirq system will attempt
>     to awaken ksoftirqd and will stop the traditional back-of-interrupt
>     softirq processing.  This is all well and good, but only if the
>     ksoftirqd kthreads already exist, which is not the case during early
>     boot, in which case the system hangs.
>     
>     One reproducer is as follows:
>     
>     tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
>     
>     This commit therefore moves the spawning of the ksoftirqd kthreads
>     earlier in boot.  With this change, the above test passes.
>     
>     Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>     Reported-by: Uladzislau Rezki <urezki@gmail.com>
>     Inspired-by: Uladzislau Rezki <urezki@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index bb8ff90..283a02d 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -592,6 +592,8 @@ static inline struct task_struct *this_cpu_ksoftirqd(void)
>  	return this_cpu_read(ksoftirqd);
>  }
>  
> +int spawn_ksoftirqd(void);
> +
>  /* Tasklets --- multithreaded analogue of BHs.
>  
>     This API is deprecated. Please consider using threaded IRQs instead:
> diff --git a/init/main.c b/init/main.c
> index c68d784..99835bb 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1512,6 +1512,7 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	init_mm_internals();
>  
> +	spawn_ksoftirqd();
>  	rcu_init_tasks_generic();
>  	do_pre_smp_initcalls();
>  	lockup_detector_init();
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 9d71046..45d50d4 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -724,7 +724,7 @@ static struct smp_hotplug_thread softirq_threads = {
>  	.thread_comm		= "ksoftirqd/%u",
>  };
>  
> -static __init int spawn_ksoftirqd(void)
> +__init int spawn_ksoftirqd(void)
>  {
>  	cpuhp_setup_state_nocalls(CPUHP_SOFTIRQ_DEAD, "softirq:dead", NULL,
>  				  takeover_tasklets);
> @@ -732,7 +732,6 @@ static __init int spawn_ksoftirqd(void)
>  
>  	return 0;
>  }
> -early_initcall(spawn_ksoftirqd);
>  
>  /*
>   * [ These __weak aliases are kept in a separate compilation unit, so that
>
I thought about this approach as a first step how to fix it, but then came up with 
moving the spawn_ksoftirqd(void); into the softirq_init() to make it consolidated
at one place and not spread.

Then moving the softirq_init() down may cause other drawbacks, like you mentioned
if somebody needs it earlier.

I agree with your approach. Invoking the spawn_ksoftirqd() before the rcu_init_tasks_generic()
makes it safe. At least it prevents other parts to be broken comparing with touching
and moving softirq_init().

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-13 11:30             ` Uladzislau Rezki
@ 2021-02-13 16:45               ` Paul E. McKenney
  2021-02-13 20:00                 ` Uladzislau Rezki
  2021-02-15 11:28                 ` Sebastian Andrzej Siewior
  0 siblings, 2 replies; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-13 16:45 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Sebastian Andrzej Siewior, LKML, RCU, Michael Ellerman,
	Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Peter Zijlstra, Michal Hocko,
	Thomas Gleixner, Theodore Y . Ts'o, Oleksiy Avramchenko

On Sat, Feb 13, 2021 at 12:30:30PM +0100, Uladzislau Rezki wrote:
> On Fri, Feb 12, 2021 at 04:43:28PM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 12, 2021 at 04:37:09PM -0800, Paul E. McKenney wrote:
> > > On Fri, Feb 12, 2021 at 03:48:51PM -0800, Paul E. McKenney wrote:
> > > > On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> > > > > On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > > > > > Add self tests for checking of RCU-tasks API functionality.
> > > > > > > It covers:
> > > > > > >     - wait API functions;
> > > > > > >     - invoking/completion call_rcu_tasks*().
> > > > > > > 
> > > > > > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > > > > 
> > > > > > I just bisected to this commit. By booting with `threadirqs' I end up
> > > > > > with:
> > > > > > [    0.176533] Running RCU-tasks wait API self tests
> > > > > > 
> > > > > > No stall warning or so.
> > > > > > It boots again with:
> > > > > > 
> > > > > > diff --git a/init/main.c b/init/main.c
> > > > > > --- a/init/main.c
> > > > > > +++ b/init/main.c
> > > > > > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> > > > > >  	fput(file);
> > > > > >  }
> > > > > >  
> > > > > > +void rcu_tasks_initiate_self_tests(void);
> > > > > >  static noinline void __init kernel_init_freeable(void)
> > > > > >  {
> > > > > >  	/*
> > > > > > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> > > > > >  
> > > > > >  	rcu_init_tasks_generic();
> > > > > >  	do_pre_smp_initcalls();
> > > > > > +	rcu_tasks_initiate_self_tests();
> > > > > >  	lockup_detector_init();
> > > > > >  
> > > > > >  	smp_init();
> > > > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > > > --- a/kernel/rcu/tasks.h
> > > > > > +++ b/kernel/rcu/tasks.h
> > > > > > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > > > >  	rttd->notrun = true;
> > > > > >  }
> > > > > >  
> > > > > > -static void rcu_tasks_initiate_self_tests(void)
> > > > > > +void rcu_tasks_initiate_self_tests(void)
> > > > > >  {
> > > > > >  	pr_info("Running RCU-tasks wait API self tests\n");
> > > > > >  #ifdef CONFIG_TASKS_RCU
> > > > > > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> > > > > >  #endif
> > > > > >  
> > > > > >  	// Run the self-tests.
> > > > > > -	rcu_tasks_initiate_self_tests();
> > > > > >  }
> > > > > >  
> > > > > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > > > > > 
> > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > 
> > > > Apologies for the hassle!  My testing clearly missed this combination
> > > > of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(
> > > > 
> > > > But at least I can easily reproduce this hang as follows:
> > > > 
> > > > tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> > > > 
> > > > Sadly, I cannot take your patch because that simply papers over the
> > > > fact that early boot use of synchronize_rcu_tasks() is broken in this
> > > > particular configuration, which will likely eventually bite others now
> > > > that init_kprobes() has been moved earlier in boot:
> > > > 
> > > > 1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
> > > > Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
> > > > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> > > > 
> > > > > > Sebastian
> > > > > >
> > > > > We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> > > > > The problem is that, ksoftirqd threads are not spawned by the time when
> > > > > an rcu_init_tasks_generic() is invoked:
> > > > > 
> > > > > diff --git a/init/main.c b/init/main.c
> > > > > index c68d784376ca..e6106bb12b2d 100644
> > > > > --- a/init/main.c
> > > > > +++ b/init/main.c
> > > > > @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > > > >  	rcu_init_nohz();
> > > > >  	init_timers();
> > > > >  	hrtimers_init();
> > > > > -	softirq_init();
> > > > >  	timekeeping_init();
> > > > >  
> > > > >  	/*
> > > > > @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
> > > > >  
> > > > >  	init_mm_internals();
> > > > >  
> > > > > +	softirq_init();
> > > > >  	rcu_init_tasks_generic();
> > > > >  	do_pre_smp_initcalls();
> > > > >  	lockup_detector_init();
> > > > > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > > > > index 9d71046ea247..cafa55c496d0 100644
> > > > > --- a/kernel/softirq.c
> > > > > +++ b/kernel/softirq.c
> > > > > @@ -630,6 +630,7 @@ void __init softirq_init(void)
> > > > >  			&per_cpu(tasklet_hi_vec, cpu).head;
> > > > >  	}
> > > > >  
> > > > > +	spawn_ksoftirqd();
> > > > 
> > > > We need a forward reference to allow this to build, but with that added,
> > > > my test case passes.  Good show!
> > > > 
> > > > >  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
> > > > >  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
> > > > >  }
> > > > > @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -early_initcall(spawn_ksoftirqd);
> > > > >  
> > > > >  /*
> > > > >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > > > > 
> > > > > Any thoughts?
> > > > 
> > > > One likely problem is that there are almost certainly parts of the kernel
> > > > that need softirq_init() to stay roughly where it is.  So, is it possible
> > > > to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
> > > > to be invoked just before rcu_init_tasks_generic() is called?
> > > 
> > > This still seems worth trying (and doing so is next on my list), but just
> > 
> > And the patch below takes this approach, which also causes the tests to
> > pass.
> > 
> > Thoughts?
> > 
> > 								Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit f4cd768e341486655c8c196e1f2b48a4463541f3
> > Author: Paul E. McKenney <paulmck@kernel.org>
> > Date:   Fri Feb 12 16:41:05 2021 -0800
> > 
> >     softirq: Don't try waking ksoftirqd before it has been spawned
> >     
> >     If there is heavy softirq activity, the softirq system will attempt
> >     to awaken ksoftirqd and will stop the traditional back-of-interrupt
> >     softirq processing.  This is all well and good, but only if the
> >     ksoftirqd kthreads already exist, which is not the case during early
> >     boot, in which case the system hangs.
> >     
> >     One reproducer is as follows:
> >     
> >     tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> >     
> >     This commit therefore moves the spawning of the ksoftirqd kthreads
> >     earlier in boot.  With this change, the above test passes.
> >     
> >     Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >     Reported-by: Uladzislau Rezki <urezki@gmail.com>
> >     Inspired-by: Uladzislau Rezki <urezki@gmail.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index bb8ff90..283a02d 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -592,6 +592,8 @@ static inline struct task_struct *this_cpu_ksoftirqd(void)
> >  	return this_cpu_read(ksoftirqd);
> >  }
> >  
> > +int spawn_ksoftirqd(void);
> > +
> >  /* Tasklets --- multithreaded analogue of BHs.
> >  
> >     This API is deprecated. Please consider using threaded IRQs instead:
> > diff --git a/init/main.c b/init/main.c
> > index c68d784..99835bb 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1512,6 +1512,7 @@ static noinline void __init kernel_init_freeable(void)
> >  
> >  	init_mm_internals();
> >  
> > +	spawn_ksoftirqd();
> >  	rcu_init_tasks_generic();
> >  	do_pre_smp_initcalls();
> >  	lockup_detector_init();
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index 9d71046..45d50d4 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -724,7 +724,7 @@ static struct smp_hotplug_thread softirq_threads = {
> >  	.thread_comm		= "ksoftirqd/%u",
> >  };
> >  
> > -static __init int spawn_ksoftirqd(void)
> > +__init int spawn_ksoftirqd(void)
> >  {
> >  	cpuhp_setup_state_nocalls(CPUHP_SOFTIRQ_DEAD, "softirq:dead", NULL,
> >  				  takeover_tasklets);
> > @@ -732,7 +732,6 @@ static __init int spawn_ksoftirqd(void)
> >  
> >  	return 0;
> >  }
> > -early_initcall(spawn_ksoftirqd);
> >  
> >  /*
> >   * [ These __weak aliases are kept in a separate compilation unit, so that
> >
> I thought about this approach as a first step how to fix it, but then came up with 
> moving the spawn_ksoftirqd(void); into the softirq_init() to make it consolidated
> at one place and not spread.
> 
> Then moving the softirq_init() down may cause other drawbacks, like you mentioned
> if somebody needs it earlier.
> 
> I agree with your approach. Invoking the spawn_ksoftirqd() before the rcu_init_tasks_generic()
> makes it safe. At least it prevents other parts to be broken comparing with touching
> and moving softirq_init().

Glad you like it!  But let's see which (if any) of these patches solves
the problem for Sebastian.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-13 16:45               ` Paul E. McKenney
@ 2021-02-13 20:00                 ` Uladzislau Rezki
  2021-02-15 11:28                 ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 32+ messages in thread
From: Uladzislau Rezki @ 2021-02-13 20:00 UTC (permalink / raw)
  To: Paul E. McKenney, Sebastian Andrzej Siewior
  Cc: Uladzislau Rezki, Sebastian Andrzej Siewior, LKML, RCU,
	Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Peter Zijlstra, Michal Hocko, Thomas Gleixner,
	Theodore Y . Ts'o, Oleksiy Avramchenko

On Sat, Feb 13, 2021 at 08:45:54AM -0800, Paul E. McKenney wrote:
> On Sat, Feb 13, 2021 at 12:30:30PM +0100, Uladzislau Rezki wrote:
> > On Fri, Feb 12, 2021 at 04:43:28PM -0800, Paul E. McKenney wrote:
> > > On Fri, Feb 12, 2021 at 04:37:09PM -0800, Paul E. McKenney wrote:
> > > > On Fri, Feb 12, 2021 at 03:48:51PM -0800, Paul E. McKenney wrote:
> > > > > On Fri, Feb 12, 2021 at 10:12:07PM +0100, Uladzislau Rezki wrote:
> > > > > > On Fri, Feb 12, 2021 at 08:20:59PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > > > On 2020-12-09 21:27:32 [+0100], Uladzislau Rezki (Sony) wrote:
> > > > > > > > Add self tests for checking of RCU-tasks API functionality.
> > > > > > > > It covers:
> > > > > > > >     - wait API functions;
> > > > > > > >     - invoking/completion call_rcu_tasks*().
> > > > > > > > 
> > > > > > > > Self-tests are run when CONFIG_PROVE_RCU kernel parameter is set.
> > > > > > > 
> > > > > > > I just bisected to this commit. By booting with `threadirqs' I end up
> > > > > > > with:
> > > > > > > [    0.176533] Running RCU-tasks wait API self tests
> > > > > > > 
> > > > > > > No stall warning or so.
> > > > > > > It boots again with:
> > > > > > > 
> > > > > > > diff --git a/init/main.c b/init/main.c
> > > > > > > --- a/init/main.c
> > > > > > > +++ b/init/main.c
> > > > > > > @@ -1489,6 +1489,7 @@ void __init console_on_rootfs(void)
> > > > > > >  	fput(file);
> > > > > > >  }
> > > > > > >  
> > > > > > > +void rcu_tasks_initiate_self_tests(void);
> > > > > > >  static noinline void __init kernel_init_freeable(void)
> > > > > > >  {
> > > > > > >  	/*
> > > > > > > @@ -1514,6 +1515,7 @@ static noinline void __init kernel_init_freeable(void)
> > > > > > >  
> > > > > > >  	rcu_init_tasks_generic();
> > > > > > >  	do_pre_smp_initcalls();
> > > > > > > +	rcu_tasks_initiate_self_tests();
> > > > > > >  	lockup_detector_init();
> > > > > > >  
> > > > > > >  	smp_init();
> > > > > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > > > > --- a/kernel/rcu/tasks.h
> > > > > > > +++ b/kernel/rcu/tasks.h
> > > > > > > @@ -1266,7 +1266,7 @@ static void test_rcu_tasks_callback(struct rcu_head *rhp)
> > > > > > >  	rttd->notrun = true;
> > > > > > >  }
> > > > > > >  
> > > > > > > -static void rcu_tasks_initiate_self_tests(void)
> > > > > > > +void rcu_tasks_initiate_self_tests(void)
> > > > > > >  {
> > > > > > >  	pr_info("Running RCU-tasks wait API self tests\n");
> > > > > > >  #ifdef CONFIG_TASKS_RCU
> > > > > > > @@ -1322,7 +1322,6 @@ void __init rcu_init_tasks_generic(void)
> > > > > > >  #endif
> > > > > > >  
> > > > > > >  	// Run the self-tests.
> > > > > > > -	rcu_tasks_initiate_self_tests();
> > > > > > >  }
> > > > > > >  
> > > > > > >  #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
> > > > > > > 
> > > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > 
> > > > > Apologies for the hassle!  My testing clearly missed this combination
> > > > > of CONFIG_PROVE_RCU=y and threadirqs=1.  :-(
> > > > > 
> > > > > But at least I can easily reproduce this hang as follows:
> > > > > 
> > > > > tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> > > > > 
> > > > > Sadly, I cannot take your patch because that simply papers over the
> > > > > fact that early boot use of synchronize_rcu_tasks() is broken in this
> > > > > particular configuration, which will likely eventually bite others now
> > > > > that init_kprobes() has been moved earlier in boot:
> > > > > 
> > > > > 1b04fa990026 ("rcu-tasks: Move RCU-tasks initialization to before early_initcall()")
> > > > > Link: https://lore.kernel.org/rcu/87eekfh80a.fsf@dja-thinkpad.axtens.net/
> > > > > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> > > > > 
> > > > > > > Sebastian
> > > > > > >
> > > > > > We should be able to use call_rcu_tasks() in the *initcall() callbacks.
> > > > > > The problem is that, ksoftirqd threads are not spawned by the time when
> > > > > > an rcu_init_tasks_generic() is invoked:
> > > > > > 
> > > > > > diff --git a/init/main.c b/init/main.c
> > > > > > index c68d784376ca..e6106bb12b2d 100644
> > > > > > --- a/init/main.c
> > > > > > +++ b/init/main.c
> > > > > > @@ -954,7 +954,6 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > > > > >  	rcu_init_nohz();
> > > > > >  	init_timers();
> > > > > >  	hrtimers_init();
> > > > > > -	softirq_init();
> > > > > >  	timekeeping_init();
> > > > > >  
> > > > > >  	/*
> > > > > > @@ -1512,6 +1511,7 @@ static noinline void __init kernel_init_freeable(void)
> > > > > >  
> > > > > >  	init_mm_internals();
> > > > > >  
> > > > > > +	softirq_init();
> > > > > >  	rcu_init_tasks_generic();
> > > > > >  	do_pre_smp_initcalls();
> > > > > >  	lockup_detector_init();
> > > > > > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > > > > > index 9d71046ea247..cafa55c496d0 100644
> > > > > > --- a/kernel/softirq.c
> > > > > > +++ b/kernel/softirq.c
> > > > > > @@ -630,6 +630,7 @@ void __init softirq_init(void)
> > > > > >  			&per_cpu(tasklet_hi_vec, cpu).head;
> > > > > >  	}
> > > > > >  
> > > > > > +	spawn_ksoftirqd();
> > > > > 
> > > > > We need a forward reference to allow this to build, but with that added,
> > > > > my test case passes.  Good show!
> > > > > 
> > > > > >  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
> > > > > >  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
> > > > > >  }
> > > > > > @@ -732,7 +733,6 @@ static __init int spawn_ksoftirqd(void)
> > > > > >  
> > > > > >  	return 0;
> > > > > >  }
> > > > > > -early_initcall(spawn_ksoftirqd);
> > > > > >  
> > > > > >  /*
> > > > > >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > > > > > 
> > > > > > Any thoughts?
> > > > > 
> > > > > One likely problem is that there are almost certainly parts of the kernel
> > > > > that need softirq_init() to stay roughly where it is.  So, is it possible
> > > > > to leave softirq_init() where it is, and to arrange for spawn_ksoftirqd()
> > > > > to be invoked just before rcu_init_tasks_generic() is called?
> > > > 
> > > > This still seems worth trying (and doing so is next on my list), but just
> > > 
> > > And the patch below takes this approach, which also causes the tests to
> > > pass.
> > > 
> > > Thoughts?
> > > 
> > > 								Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > commit f4cd768e341486655c8c196e1f2b48a4463541f3
> > > Author: Paul E. McKenney <paulmck@kernel.org>
> > > Date:   Fri Feb 12 16:41:05 2021 -0800
> > > 
> > >     softirq: Don't try waking ksoftirqd before it has been spawned
> > >     
> > >     If there is heavy softirq activity, the softirq system will attempt
> > >     to awaken ksoftirqd and will stop the traditional back-of-interrupt
> > >     softirq processing.  This is all well and good, but only if the
> > >     ksoftirqd kthreads already exist, which is not the case during early
> > >     boot, in which case the system hangs.
> > >     
> > >     One reproducer is as follows:
> > >     
> > >     tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE03" --kconfig "CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y" --bootargs "threadirqs=1" --trust-make
> > >     
> > >     This commit therefore moves the spawning of the ksoftirqd kthreads
> > >     earlier in boot.  With this change, the above test passes.
> > >     
> > >     Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > >     Reported-by: Uladzislau Rezki <urezki@gmail.com>
> > >     Inspired-by: Uladzislau Rezki <urezki@gmail.com>
> > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > 
> > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > > index bb8ff90..283a02d 100644
> > > --- a/include/linux/interrupt.h
> > > +++ b/include/linux/interrupt.h
> > > @@ -592,6 +592,8 @@ static inline struct task_struct *this_cpu_ksoftirqd(void)
> > >  	return this_cpu_read(ksoftirqd);
> > >  }
> > >  
> > > +int spawn_ksoftirqd(void);
> > > +
> > >  /* Tasklets --- multithreaded analogue of BHs.
> > >  
> > >     This API is deprecated. Please consider using threaded IRQs instead:
> > > diff --git a/init/main.c b/init/main.c
> > > index c68d784..99835bb 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -1512,6 +1512,7 @@ static noinline void __init kernel_init_freeable(void)
> > >  
> > >  	init_mm_internals();
> > >  
> > > +	spawn_ksoftirqd();
> > >  	rcu_init_tasks_generic();
> > >  	do_pre_smp_initcalls();
> > >  	lockup_detector_init();
> > > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > > index 9d71046..45d50d4 100644
> > > --- a/kernel/softirq.c
> > > +++ b/kernel/softirq.c
> > > @@ -724,7 +724,7 @@ static struct smp_hotplug_thread softirq_threads = {
> > >  	.thread_comm		= "ksoftirqd/%u",
> > >  };
> > >  
> > > -static __init int spawn_ksoftirqd(void)
> > > +__init int spawn_ksoftirqd(void)
> > >  {
> > >  	cpuhp_setup_state_nocalls(CPUHP_SOFTIRQ_DEAD, "softirq:dead", NULL,
> > >  				  takeover_tasklets);
> > > @@ -732,7 +732,6 @@ static __init int spawn_ksoftirqd(void)
> > >  
> > >  	return 0;
> > >  }
> > > -early_initcall(spawn_ksoftirqd);
> > >  
> > >  /*
> > >   * [ These __weak aliases are kept in a separate compilation unit, so that
> > >
> > I thought about this approach as a first step how to fix it, but then came up with 
> > moving the spawn_ksoftirqd(void); into the softirq_init() to make it consolidated
> > at one place and not spread.
> > 
> > Then moving the softirq_init() down may cause other drawbacks, like you mentioned
> > if somebody needs it earlier.
> > 
> > I agree with your approach. Invoking the spawn_ksoftirqd() before the rcu_init_tasks_generic()
> > makes it safe. At least it prevents other parts to be broken comparing with touching
> > and moving softirq_init().
> 
> Glad you like it!  But let's see which (if any) of these patches solves
> the problem for Sebastian.
> 
I tried to reproduce it on my box and i succeed. Both patches solve it for me.
But let's see if it fixes Sebastian setup :)

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-13 16:45               ` Paul E. McKenney
  2021-02-13 20:00                 ` Uladzislau Rezki
@ 2021-02-15 11:28                 ` Sebastian Andrzej Siewior
  2021-02-16 17:30                   ` Paul E. McKenney
  1 sibling, 1 reply; 32+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-15 11:28 UTC (permalink / raw)
  To: Paul E. McKenney, Peter Zijlstra, Thomas Gleixner
  Cc: Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Michal Hocko, Theodore Y . Ts'o,
	Oleksiy Avramchenko

On 2021-02-13 08:45:54 [-0800], Paul E. McKenney wrote:
> Glad you like it!  But let's see which (if any) of these patches solves
> the problem for Sebastian.

Looking at that, is there any reason for doing this that can not be
solved by moving the self-test a little later? Maybe once we reached at
least SYSTEM_SCHEDULING?
This happens now even before lockdep is up or the console is registered.
So if something bad happens, you end up with a blank terminal.

There is nothing else that early in the boot process that requires
working softirq. The only exception to this is wait_task_inactive()
which is used while starting a new thread (including the ksoftirqd)
which is why it was moved to schedule_hrtimeout().

> 							Thanx, Paul

Sebastian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-15 11:28                 ` Sebastian Andrzej Siewior
@ 2021-02-16 17:30                   ` Paul E. McKenney
  2021-02-17 14:47                     ` Masami Hiramatsu
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-16 17:30 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Peter Zijlstra, Thomas Gleixner, Uladzislau Rezki, LKML, RCU,
	Michael Ellerman, Andrew Morton, Daniel Axtens,
	Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
	Michal Hocko, Theodore Y . Ts'o, Oleksiy Avramchenko,
	mhiramat, rostedt

On Mon, Feb 15, 2021 at 12:28:26PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-02-13 08:45:54 [-0800], Paul E. McKenney wrote:
> > Glad you like it!  But let's see which (if any) of these patches solves
> > the problem for Sebastian.
> 
> Looking at that, is there any reason for doing this that can not be
> solved by moving the self-test a little later? Maybe once we reached at
> least SYSTEM_SCHEDULING?

One problem is that ksoftirqd and the kprobes use are early_initcall(),
so we cannot count on ksoftirqd being spawned when kprobes first uses
synchronize_rcu_tasks().  Moving the selftest later won't fix this
problem, but rather just paper it over.

> This happens now even before lockdep is up or the console is registered.
> So if something bad happens, you end up with a blank terminal.

I was getting a splat, but I could easily believe that there are
configurations where the hang is totally silent.  In other words, I do
agree that this needs a proper fix.  All we need do is work out an
agreeable value of "proper".  ;-)

> There is nothing else that early in the boot process that requires
> working softirq. The only exception to this is wait_task_inactive()
> which is used while starting a new thread (including the ksoftirqd)
> which is why it was moved to schedule_hrtimeout().

Moving kprobes initialization to early_initcall() [1] means that there
can be a call to synchronize_rcu_tasks() before the current spawning of
ksoftirqd.  Because synchronize_rcu_tasks() needs timers to work, it needs
softirq to work.  I know two straightforward ways to make that happen:

1.	Spawn ksoftirqd earlier.

2.	Suppress attempts to awaken ksoftirqd before it exists,
	forcing all ksoftirq execution on the back of interrupts.

Uladzislau and I each produced patches for #1, and I produced a patch
for #2.

The only other option I know of is to push the call to init_kprobes()
later in the boot sequence, perhaps to its original subsys_initcall(),
or maybe only as late as core_initcall().  I added Masami and Steve on
CC for their thoughts on this.

Is there some other proper fix that I am missing?

						Thanx, Paul

[1] 36dadef23fcc ("kprobes: Init kprobes in early_initcall")

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-16 17:30                   ` Paul E. McKenney
@ 2021-02-17 14:47                     ` Masami Hiramatsu
  2021-02-17 18:17                       ` Paul E. McKenney
  0 siblings, 1 reply; 32+ messages in thread
From: Masami Hiramatsu @ 2021-02-17 14:47 UTC (permalink / raw)
  To: paulmck
  Cc: Sebastian Andrzej Siewior, Peter Zijlstra, Thomas Gleixner,
	Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Michal Hocko, Theodore Y . Ts'o,
	Oleksiy Avramchenko, mhiramat, rostedt

On Tue, 16 Feb 2021 09:30:03 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Mon, Feb 15, 2021 at 12:28:26PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2021-02-13 08:45:54 [-0800], Paul E. McKenney wrote:
> > > Glad you like it!  But let's see which (if any) of these patches solves
> > > the problem for Sebastian.
> > 
> > Looking at that, is there any reason for doing this that can not be
> > solved by moving the self-test a little later? Maybe once we reached at
> > least SYSTEM_SCHEDULING?
> 
> One problem is that ksoftirqd and the kprobes use are early_initcall(),
> so we cannot count on ksoftirqd being spawned when kprobes first uses
> synchronize_rcu_tasks().  Moving the selftest later won't fix this
> problem, but rather just paper it over.
> 
> > This happens now even before lockdep is up or the console is registered.
> > So if something bad happens, you end up with a blank terminal.
> 
> I was getting a splat, but I could easily believe that there are
> configurations where the hang is totally silent.  In other words, I do
> agree that this needs a proper fix.  All we need do is work out an
> agreeable value of "proper".  ;-)
> 
> > There is nothing else that early in the boot process that requires
> > working softirq. The only exception to this is wait_task_inactive()
> > which is used while starting a new thread (including the ksoftirqd)
> > which is why it was moved to schedule_hrtimeout().
> 
> Moving kprobes initialization to early_initcall() [1] means that there
> can be a call to synchronize_rcu_tasks() before the current spawning of
> ksoftirqd.  Because synchronize_rcu_tasks() needs timers to work, it needs
> softirq to work.  I know two straightforward ways to make that happen:
> 
> 1.	Spawn ksoftirqd earlier.
> 
> 2.	Suppress attempts to awaken ksoftirqd before it exists,
> 	forcing all ksoftirq execution on the back of interrupts.
> 
> Uladzislau and I each produced patches for #1, and I produced a patch
> for #2.
> 
> The only other option I know of is to push the call to init_kprobes()
> later in the boot sequence, perhaps to its original subsys_initcall(),
> or maybe only as late as core_initcall().  I added Masami and Steve on
> CC for their thoughts on this.
> 
> Is there some other proper fix that I am missing?

Oh, I missed that the synchronize_rcu_tasks() will be involved the kprobes
in early stage. Does the problem only exist in the synchronize_rcu_tasks()
instead of synchronize_rcu()? If so I can just stop optimizer in early stage
because I just want to enable kprobes in early stage, but not optprobes.

Does the following patch help?

From e5fafcda3ff918cd52619f795a3f22fb95c72b11 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Wed, 17 Feb 2021 23:35:20 +0900
Subject: [PATCH] kprobes: Fix to delay the kprobes jump optimization

Since the kprobes jump optimization involves synchronize_rcu_tasks()
which depends on the ksoftirqd, that can not be enabled at the
early_initcall() boot stage. So this makes the kprobe optimization
disabled in the early_initcall() and enables it in subsys_initcall().

Note that non-optimized kprobes is still available after
early_initcall(). Only jump optimization is delayed.

Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d5a3eb74a657..779d8322e307 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -861,7 +861,6 @@ static void try_to_optimize_kprobe(struct kprobe *p)
 	cpus_read_unlock();
 }
 
-#ifdef CONFIG_SYSCTL
 static void optimize_all_kprobes(void)
 {
 	struct hlist_head *head;
@@ -887,6 +886,7 @@ static void optimize_all_kprobes(void)
 	mutex_unlock(&kprobe_mutex);
 }
 
+#ifdef CONFIG_SYSCTL
 static void unoptimize_all_kprobes(void)
 {
 	struct hlist_head *head;
@@ -2497,18 +2497,14 @@ static int __init init_kprobes(void)
 		}
 	}
 
-#if defined(CONFIG_OPTPROBES)
-#if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
-	/* Init kprobe_optinsn_slots */
-	kprobe_optinsn_slots.insn_size = MAX_OPTINSN_SIZE;
-#endif
-	/* By default, kprobes can be optimized */
-	kprobes_allow_optimization = true;
-#endif
-
 	/* By default, kprobes are armed */
 	kprobes_all_disarmed = false;
 
+#if defined(CONFIG_OPTPROBES) && defined(__ARCH_WANT_KPROBES_INSN_SLOT)
+	/* Init kprobe_optinsn_slots for allocation */
+	kprobe_optinsn_slots.insn_size = MAX_OPTINSN_SIZE;
+#endif
+
 	err = arch_init_kprobes();
 	if (!err)
 		err = register_die_notifier(&kprobe_exceptions_nb);
@@ -2523,6 +2519,21 @@ static int __init init_kprobes(void)
 }
 early_initcall(init_kprobes);
 
+#if defined(CONFIG_OPTPROBES)
+static int __init init_optprobes(void)
+{
+	/*
+	 * Enable kprobe optimization - this kicks the optimizer which
+	 * depends on synchronize_rcu_tasks() and ksoftirqd, that is
+	 * not spawned in early initcall. So delay the optimization.
+	 */
+	optimize_all_kprobes();
+
+	return 0;
+}
+subsys_initcall(init_optprobes);
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 static void report_probe(struct seq_file *pi, struct kprobe *p,
 		const char *sym, int offset, char *modname, struct kprobe *pp)
-- 
2.25.1


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-17 14:47                     ` Masami Hiramatsu
@ 2021-02-17 18:17                       ` Paul E. McKenney
  2021-02-18  5:03                         ` Masami Hiramatsu
  0 siblings, 1 reply; 32+ messages in thread
From: Paul E. McKenney @ 2021-02-17 18:17 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Sebastian Andrzej Siewior, Peter Zijlstra, Thomas Gleixner,
	Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Michal Hocko, Theodore Y . Ts'o,
	Oleksiy Avramchenko, rostedt

On Wed, Feb 17, 2021 at 11:47:59PM +0900, Masami Hiramatsu wrote:
> On Tue, 16 Feb 2021 09:30:03 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Mon, Feb 15, 2021 at 12:28:26PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2021-02-13 08:45:54 [-0800], Paul E. McKenney wrote:
> > > > Glad you like it!  But let's see which (if any) of these patches solves
> > > > the problem for Sebastian.
> > > 
> > > Looking at that, is there any reason for doing this that can not be
> > > solved by moving the self-test a little later? Maybe once we reached at
> > > least SYSTEM_SCHEDULING?
> > 
> > One problem is that ksoftirqd and the kprobes use are early_initcall(),
> > so we cannot count on ksoftirqd being spawned when kprobes first uses
> > synchronize_rcu_tasks().  Moving the selftest later won't fix this
> > problem, but rather just paper it over.
> > 
> > > This happens now even before lockdep is up or the console is registered.
> > > So if something bad happens, you end up with a blank terminal.
> > 
> > I was getting a splat, but I could easily believe that there are
> > configurations where the hang is totally silent.  In other words, I do
> > agree that this needs a proper fix.  All we need do is work out an
> > agreeable value of "proper".  ;-)
> > 
> > > There is nothing else that early in the boot process that requires
> > > working softirq. The only exception to this is wait_task_inactive()
> > > which is used while starting a new thread (including the ksoftirqd)
> > > which is why it was moved to schedule_hrtimeout().
> > 
> > Moving kprobes initialization to early_initcall() [1] means that there
> > can be a call to synchronize_rcu_tasks() before the current spawning of
> > ksoftirqd.  Because synchronize_rcu_tasks() needs timers to work, it needs
> > softirq to work.  I know two straightforward ways to make that happen:
> > 
> > 1.	Spawn ksoftirqd earlier.
> > 
> > 2.	Suppress attempts to awaken ksoftirqd before it exists,
> > 	forcing all ksoftirq execution on the back of interrupts.
> > 
> > Uladzislau and I each produced patches for #1, and I produced a patch
> > for #2.
> > 
> > The only other option I know of is to push the call to init_kprobes()
> > later in the boot sequence, perhaps to its original subsys_initcall(),
> > or maybe only as late as core_initcall().  I added Masami and Steve on
> > CC for their thoughts on this.
> > 
> > Is there some other proper fix that I am missing?
> 
> Oh, I missed that the synchronize_rcu_tasks() will be involved the kprobes
> in early stage. Does the problem only exist in the synchronize_rcu_tasks()
> instead of synchronize_rcu()? If so I can just stop optimizer in early stage
> because I just want to enable kprobes in early stage, but not optprobes.
> 
> Does the following patch help?

It does look to me like it would!  I clearly should have asked you about
this a couple of months ago.  ;-)

The proof of the pudding would be whether the powerpc guys can apply
this to v5.10-rc7 and have their kernel come up without hanging at boot.

							Thanx, Paul

> >From e5fafcda3ff918cd52619f795a3f22fb95c72b11 Mon Sep 17 00:00:00 2001
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Date: Wed, 17 Feb 2021 23:35:20 +0900
> Subject: [PATCH] kprobes: Fix to delay the kprobes jump optimization
> 
> Since the kprobes jump optimization involves synchronize_rcu_tasks()
> which depends on the ksoftirqd, that can not be enabled at the
> early_initcall() boot stage. So this makes the kprobe optimization
> disabled in the early_initcall() and enables it in subsys_initcall().
> 
> Note that non-optimized kprobes is still available after
> early_initcall(). Only jump optimization is delayed.
> 
> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index d5a3eb74a657..779d8322e307 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -861,7 +861,6 @@ static void try_to_optimize_kprobe(struct kprobe *p)
>  	cpus_read_unlock();
>  }
>  
> -#ifdef CONFIG_SYSCTL
>  static void optimize_all_kprobes(void)
>  {
>  	struct hlist_head *head;
> @@ -887,6 +886,7 @@ static void optimize_all_kprobes(void)
>  	mutex_unlock(&kprobe_mutex);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static void unoptimize_all_kprobes(void)
>  {
>  	struct hlist_head *head;
> @@ -2497,18 +2497,14 @@ static int __init init_kprobes(void)
>  		}
>  	}
>  
> -#if defined(CONFIG_OPTPROBES)
> -#if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
> -	/* Init kprobe_optinsn_slots */
> -	kprobe_optinsn_slots.insn_size = MAX_OPTINSN_SIZE;
> -#endif
> -	/* By default, kprobes can be optimized */
> -	kprobes_allow_optimization = true;
> -#endif
> -
>  	/* By default, kprobes are armed */
>  	kprobes_all_disarmed = false;
>  
> +#if defined(CONFIG_OPTPROBES) && defined(__ARCH_WANT_KPROBES_INSN_SLOT)
> +	/* Init kprobe_optinsn_slots for allocation */
> +	kprobe_optinsn_slots.insn_size = MAX_OPTINSN_SIZE;
> +#endif
> +
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
> @@ -2523,6 +2519,21 @@ static int __init init_kprobes(void)
>  }
>  early_initcall(init_kprobes);
>  
> +#if defined(CONFIG_OPTPROBES)
> +static int __init init_optprobes(void)
> +{
> +	/*
> +	 * Enable kprobe optimization - this kicks the optimizer which
> +	 * depends on synchronize_rcu_tasks() and ksoftirqd, that is
> +	 * not spawned in early initcall. So delay the optimization.
> +	 */
> +	optimize_all_kprobes();
> +
> +	return 0;
> +}
> +subsys_initcall(init_optprobes);
> +#endif
> +
>  #ifdef CONFIG_DEBUG_FS
>  static void report_probe(struct seq_file *pi, struct kprobe *p,
>  		const char *sym, int offset, char *modname, struct kprobe *pp)
> -- 
> 2.25.1
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-17 18:17                       ` Paul E. McKenney
@ 2021-02-18  5:03                         ` Masami Hiramatsu
  2021-02-18  8:36                           ` Uladzislau Rezki
  0 siblings, 1 reply; 32+ messages in thread
From: Masami Hiramatsu @ 2021-02-18  5:03 UTC (permalink / raw)
  To: paulmck
  Cc: Sebastian Andrzej Siewior, Peter Zijlstra, Thomas Gleixner,
	Uladzislau Rezki, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Michal Hocko, Theodore Y . Ts'o,
	Oleksiy Avramchenko, rostedt

On Wed, 17 Feb 2021 10:17:38 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> > > 1.	Spawn ksoftirqd earlier.
> > > 
> > > 2.	Suppress attempts to awaken ksoftirqd before it exists,
> > > 	forcing all ksoftirq execution on the back of interrupts.
> > > 
> > > Uladzislau and I each produced patches for #1, and I produced a patch
> > > for #2.
> > > 
> > > The only other option I know of is to push the call to init_kprobes()
> > > later in the boot sequence, perhaps to its original subsys_initcall(),
> > > or maybe only as late as core_initcall().  I added Masami and Steve on
> > > CC for their thoughts on this.
> > > 
> > > Is there some other proper fix that I am missing?
> > 
> > Oh, I missed that the synchronize_rcu_tasks() will be involved the kprobes
> > in early stage. Does the problem only exist in the synchronize_rcu_tasks()
> > instead of synchronize_rcu()? If so I can just stop optimizer in early stage
> > because I just want to enable kprobes in early stage, but not optprobes.
> > 
> > Does the following patch help?
> 
> It does look to me like it would!  I clearly should have asked you about
> this a couple of months ago.  ;-)
> 
> The proof of the pudding would be whether the powerpc guys can apply
> this to v5.10-rc7 and have their kernel come up without hanging at boot.

Who could I ask for testing this patch, Uladzislau?
I think the test machine which enough slow or the kernel has much initcall
to run optimization thread while booting.
In my environment, I could not reproduce that issue because the optimizer
was sheduled after some tick passed. At that point, ksoftirqd has already
been initialized.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-18  5:03                         ` Masami Hiramatsu
@ 2021-02-18  8:36                           ` Uladzislau Rezki
  2021-02-18 14:29                             ` Masami Hiramatsu
  0 siblings, 1 reply; 32+ messages in thread
From: Uladzislau Rezki @ 2021-02-18  8:36 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: paulmck, Sebastian Andrzej Siewior, Peter Zijlstra,
	Thomas Gleixner, Uladzislau Rezki, LKML, RCU, Michael Ellerman,
	Andrew Morton, Daniel Axtens, Frederic Weisbecker,
	Neeraj Upadhyay, Joel Fernandes, Michal Hocko,
	Theodore Y . Ts'o, Oleksiy Avramchenko, rostedt

On Thu, Feb 18, 2021 at 02:03:07PM +0900, Masami Hiramatsu wrote:
> On Wed, 17 Feb 2021 10:17:38 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > > > 1.	Spawn ksoftirqd earlier.
> > > > 
> > > > 2.	Suppress attempts to awaken ksoftirqd before it exists,
> > > > 	forcing all ksoftirq execution on the back of interrupts.
> > > > 
> > > > Uladzislau and I each produced patches for #1, and I produced a patch
> > > > for #2.
> > > > 
> > > > The only other option I know of is to push the call to init_kprobes()
> > > > later in the boot sequence, perhaps to its original subsys_initcall(),
> > > > or maybe only as late as core_initcall().  I added Masami and Steve on
> > > > CC for their thoughts on this.
> > > > 
> > > > Is there some other proper fix that I am missing?
> > > 
> > > Oh, I missed that the synchronize_rcu_tasks() will be involved the kprobes
> > > in early stage. Does the problem only exist in the synchronize_rcu_tasks()
> > > instead of synchronize_rcu()? If so I can just stop optimizer in early stage
> > > because I just want to enable kprobes in early stage, but not optprobes.
> > > 
> > > Does the following patch help?
> > 
> > It does look to me like it would!  I clearly should have asked you about
> > this a couple of months ago.  ;-)
> > 
> > The proof of the pudding would be whether the powerpc guys can apply
> > this to v5.10-rc7 and have their kernel come up without hanging at boot.
> 
> Who could I ask for testing this patch, Uladzislau?
> I think the test machine which enough slow or the kernel has much initcall
> to run optimization thread while booting.
> In my environment, I could not reproduce that issue because the optimizer
> was sheduled after some tick passed. At that point, ksoftirqd has already
> been initialized.
> 
From my end i did some simulation and had a look at your change. So the
patch works on my setup. I see that optimization of kprobes is deferred
and can be initiated only from the subsys_initcall() phase. So the sequence
should be correct for v5.10-rc7:

1. ksoftirq is setup early_initcall();
2. rcu_spawn_tasks_* are setup in the core_initcall();
3. an optimization of kprobes are invoked from subsys_initcall().

For real test on power-pc you can ask Daniel Axtens <dja@axtens.net> for help. 

--
Vlad Rezki

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/2] rcu-tasks: add RCU-tasks self tests
  2021-02-18  8:36                           ` Uladzislau Rezki
@ 2021-02-18 14:29                             ` Masami Hiramatsu
  0 siblings, 0 replies; 32+ messages in thread
From: Masami Hiramatsu @ 2021-02-18 14:29 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: paulmck, Sebastian Andrzej Siewior, Peter Zijlstra,
	Thomas Gleixner, LKML, RCU, Michael Ellerman, Andrew Morton,
	Daniel Axtens, Frederic Weisbecker, Neeraj Upadhyay,
	Joel Fernandes, Michal Hocko, Theodore Y . Ts'o,
	Oleksiy Avramchenko, rostedt

On Thu, 18 Feb 2021 09:36:36 +0100
Uladzislau Rezki <urezki@gmail.com> wrote:

> On Thu, Feb 18, 2021 at 02:03:07PM +0900, Masami Hiramatsu wrote:
> > On Wed, 17 Feb 2021 10:17:38 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > > > 1.	Spawn ksoftirqd earlier.
> > > > > 
> > > > > 2.	Suppress attempts to awaken ksoftirqd before it exists,
> > > > > 	forcing all ksoftirq execution on the back of interrupts.
> > > > > 
> > > > > Uladzislau and I each produced patches for #1, and I produced a patch
> > > > > for #2.
> > > > > 
> > > > > The only other option I know of is to push the call to init_kprobes()
> > > > > later in the boot sequence, perhaps to its original subsys_initcall(),
> > > > > or maybe only as late as core_initcall().  I added Masami and Steve on
> > > > > CC for their thoughts on this.
> > > > > 
> > > > > Is there some other proper fix that I am missing?
> > > > 
> > > > Oh, I missed that the synchronize_rcu_tasks() will be involved the kprobes
> > > > in early stage. Does the problem only exist in the synchronize_rcu_tasks()
> > > > instead of synchronize_rcu()? If so I can just stop optimizer in early stage
> > > > because I just want to enable kprobes in early stage, but not optprobes.
> > > > 
> > > > Does the following patch help?
> > > 
> > > It does look to me like it would!  I clearly should have asked you about
> > > this a couple of months ago.  ;-)
> > > 
> > > The proof of the pudding would be whether the powerpc guys can apply
> > > this to v5.10-rc7 and have their kernel come up without hanging at boot.
> > 
> > Who could I ask for testing this patch, Uladzislau?
> > I think the test machine which enough slow or the kernel has much initcall
> > to run optimization thread while booting.
> > In my environment, I could not reproduce that issue because the optimizer
> > was sheduled after some tick passed. At that point, ksoftirqd has already
> > been initialized.
> > 
> From my end i did some simulation and had a look at your change. So the
> patch works on my setup. I see that optimization of kprobes is deferred
> and can be initiated only from the subsys_initcall() phase. So the sequence
> should be correct for v5.10-rc7:
> 
> 1. ksoftirq is setup early_initcall();

    also kprobe framework is initialized in early_initcall() and
    kprobes smoketest (register/unregister probes) executed.

> 2. rcu_spawn_tasks_* are setup in the core_initcall();

    and kprobe events are setup in core_initcall_sync() (via trace_boot);

> 3. an optimization of kprobes are invoked from subsys_initcall().

   At this point, all kprobes which can be optimized will be actually
   optimized.


> For real test on power-pc you can ask Daniel Axtens <dja@axtens.net> for help. 

OK, I'll CC to Daniel.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2021-02-18 17:24 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09 20:27 [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki (Sony)
2020-12-09 20:27 ` [PATCH 2/2] rcu-tasks: add RCU-tasks self tests Uladzislau Rezki (Sony)
2020-12-16 15:49   ` Uladzislau Rezki
2020-12-16 23:29     ` Paul E. McKenney
2020-12-21 15:38       ` Uladzislau Rezki
2020-12-21 17:18         ` Paul E. McKenney
2020-12-21 18:45           ` Uladzislau Rezki
2020-12-21 19:29             ` Paul E. McKenney
2020-12-21 19:48               ` Uladzislau Rezki
2020-12-21 20:45                 ` Paul E. McKenney
2020-12-21 21:28                   ` Uladzislau Rezki
2021-02-12 19:20   ` Sebastian Andrzej Siewior
2021-02-12 21:12     ` Uladzislau Rezki
2021-02-12 23:48       ` Paul E. McKenney
2021-02-13  0:37         ` Paul E. McKenney
2021-02-13  0:43           ` Paul E. McKenney
2021-02-13 11:30             ` Uladzislau Rezki
2021-02-13 16:45               ` Paul E. McKenney
2021-02-13 20:00                 ` Uladzislau Rezki
2021-02-15 11:28                 ` Sebastian Andrzej Siewior
2021-02-16 17:30                   ` Paul E. McKenney
2021-02-17 14:47                     ` Masami Hiramatsu
2021-02-17 18:17                       ` Paul E. McKenney
2021-02-18  5:03                         ` Masami Hiramatsu
2021-02-18  8:36                           ` Uladzislau Rezki
2021-02-18 14:29                             ` Masami Hiramatsu
2020-12-09 20:37 ` [PATCH 1/2] rcu-tasks: move RCU-tasks initialization out of core_initcall() Uladzislau Rezki
2020-12-10 13:39   ` Daniel Axtens
2020-12-10 17:32     ` Paul E. McKenney
2020-12-10 18:17     ` Uladzislau Rezki
2020-12-10  3:26 ` Paul E. McKenney
2020-12-10 13:04   ` Uladzislau Rezki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.