[PATCH 0/2] Optimize synchronize_rcu_tasks_generic() on early booting

[PATCH 0/2] Optimize synchronize_rcu_tasks_generic() on early booting

[Zqiang]

This commit fix warning trigger when call synchronize_rcu_tasks_generic() on
early booting and make synchronize_rcu_tasks_generic() directly return when
the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE.

Zqiang (2):
  rcu-tasks: Convert RCU_LOCKDEP_WARN() to WARN_ONCE()
  rcu-tasks: Make synchronize_rcu_tasks_generic() no-ops on early
    booting

 kernel/rcu/tasks.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] rcu-tasks: Convert RCU_LOCKDEP_WARN() to WARN_ONCE()

[Zqiang]

For kernel build with CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LOCK_ALLOC=y,
when the synchronize_rcu_tasks_generic() called occurs during the
rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE, the warnings
will be trigger, however the warnings is not actually triggered because
the debug_lockdep_rcu_enabled() return false(the rcu_scheduler_active !=
RCU_SCHEDULER_INACTIVE condition is false).

This commit replace RCU_LOCKDEP_WARN() with WARN_ONCE() to trigger
warnings.

Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 kernel/rcu/tasks.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 83c7e6620d40..469bf2a3b505 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -560,7 +560,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
 {
 	/* Complain if the scheduler has not started.  */
-	RCU_LOCKDEP_WARN(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
+	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
 			 "synchronize_rcu_tasks called too soon");
 
 	// If the grace-period kthread is running, use it.
-- 
2.25.1

Re: [PATCH 1/2] rcu-tasks: Convert RCU_LOCKDEP_WARN() to WARN_ONCE()

[Paul E. McKenney]

On Tue, Jul 12, 2022 at 04:26:05PM +0800, Zqiang wrote:
> For kernel build with CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LOCK_ALLOC=y,
> when the synchronize_rcu_tasks_generic() called occurs during the
> rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE, the warnings
> will be trigger, however the warnings is not actually triggered because
> the debug_lockdep_rcu_enabled() return false(the rcu_scheduler_active !=
> RCU_SCHEDULER_INACTIVE condition is false).
> 
> This commit replace RCU_LOCKDEP_WARN() with WARN_ONCE() to trigger
> warnings.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

Good catch, thank you!

Queued with wordsmithing as follows.  As always, please check to see
if I wordsmithed something out of existence.

						Thanx, Paul

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

commit 4dbfea53caf3e226e3273fc5947e9041e2ad34ab
Author: Zqiang <qiang1.zhang@intel.com>
Date:   Tue Jul 12 16:26:05 2022 +0800

    rcu-tasks: Convert RCU_LOCKDEP_WARN() to WARN_ONCE()
    
    Kernels built with CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LOCK_ALLOC=y
    attempt to emit a warning when the synchronize_rcu_tasks_generic()
    function is called during early boot while the rcu_scheduler_active
    variable is RCU_SCHEDULER_INACTIVE.  However the warnings is not
    actually be printed because the debug_lockdep_rcu_enabled() returns
    false, exactly because the rcu_scheduler_active variable is still equal
    to RCU_SCHEDULER_INACTIVE.
    
    This commit therefore replaces RCU_LOCKDEP_WARN() with WARN_ONCE()
    to force these warnings to actually be printed.
    
    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 83c7e6620d403..469bf2a3b505e 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -560,7 +560,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
 {
 	/* Complain if the scheduler has not started.  */
-	RCU_LOCKDEP_WARN(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
+	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
 			 "synchronize_rcu_tasks called too soon");
 
 	// If the grace-period kthread is running, use it.

[PATCH 2/2] rcu-tasks: Make synchronize_rcu_tasks_generic() no-ops on early booting

[Zqiang]

When the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE and not
yet converted to RCU_SCHEDULER_INIT, there is only idle task, any legal
call synchronize_rcu_tasks_generic() is a quiescent state. this commit
make synchronize_rcu_tasks_generic() no-ops when the rcu_scheduler_active
variable is RCU_SCHEDULER_INACTIVE.

Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 kernel/rcu/tasks.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 469bf2a3b505..0237e765c28e 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -560,8 +560,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
 {
 	/* Complain if the scheduler has not started.  */
-	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
-			 "synchronize_rcu_tasks called too soon");
+	if (WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
+			 "synchronize_rcu_tasks called too soon"))
+		return;
 
 	// If the grace-period kthread is running, use it.
 	if (READ_ONCE(rtp->kthread_ptr)) {
-- 
2.25.1

Re: [PATCH 2/2] rcu-tasks: Make synchronize_rcu_tasks_generic() no-ops on early booting

[Paul E. McKenney]

On Tue, Jul 12, 2022 at 04:26:06PM +0800, Zqiang wrote:
> When the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE and not
> yet converted to RCU_SCHEDULER_INIT, there is only idle task, any legal
> call synchronize_rcu_tasks_generic() is a quiescent state. this commit
> make synchronize_rcu_tasks_generic() no-ops when the rcu_scheduler_active
> variable is RCU_SCHEDULER_INACTIVE.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---

It looks like this would be a good way to provide early boot access
to synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), and
synchronize_rcu_tasks_trace().

But do we really need early boot access to these functions?  As in has
the below WARN_ON() actually triggered?

And if it has triggered, and in a context that means that these functions
really are needed during early boot, how should the testing strategy
change to test these at the relevant portions of the boot sequence?

From what I know, hitting these during early boot would indicate that
something was removing a trace during early boot, and I know of no way
to make that happen.  Hence my skepticism.  ;-)

But *if* this was really needed, this looks to be a reasonable way to
implement it.

							Thanx, Paul

>  kernel/rcu/tasks.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 469bf2a3b505..0237e765c28e 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -560,8 +560,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
>  static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
>  {
>  	/* Complain if the scheduler has not started.  */
> -	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> -			 "synchronize_rcu_tasks called too soon");
> +	if (WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> +			 "synchronize_rcu_tasks called too soon"))
> +		return;
>  
>  	// If the grace-period kthread is running, use it.
>  	if (READ_ONCE(rtp->kthread_ptr)) {
> -- 
> 2.25.1
> 

RE: [PATCH 2/2] rcu-tasks: Make synchronize_rcu_tasks_generic() no-ops on early booting

[Zhang, Qiang1]

On Tue, Jul 12, 2022 at 04:26:06PM +0800, Zqiang wrote:
> When the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE and not
> yet converted to RCU_SCHEDULER_INIT, there is only idle task, any legal
> call synchronize_rcu_tasks_generic() is a quiescent state. this commit
> make synchronize_rcu_tasks_generic() no-ops when the rcu_scheduler_active
> variable is RCU_SCHEDULER_INACTIVE.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---

>It looks like this would be a good way to provide early boot access
>to synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), and
>synchronize_rcu_tasks_trace().
>
>But do we really need early boot access to these functions?  As in has
>the below WARN_ON() actually triggered?

when the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE,
invoke synchronize_rcu_tasks_generic(), in addition to triggering a warning, 
also need to make it return directly, if not, the rcu_tasks_one_gp() will be
called directly, but due to the rtp structure's -> pregp_func is not initialized,
A null pointer bug will appear.

But like said, I don't see the need to call synchronize_rcu_tasks_generic() on 
early booting.  maybe this change is  not necessary.

Thanks
Zqiang

>
>And if it has triggered, and in a context that means that these functions
>really are needed during early boot, how should the testing strategy
>change to test these at the relevant portions of the boot sequence?
>
>From what I know, hitting these during early boot would indicate that
>something was removing a trace during early boot, and I know of no way
>to make that happen.  Hence my skepticism.  ;-)
>
>But *if* this was really needed, this looks to be a reasonable way to
>implement it.
>
>							Thanx, Paul

>  kernel/rcu/tasks.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 469bf2a3b505..0237e765c28e 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -560,8 +560,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
>  static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
>  {
>  	/* Complain if the scheduler has not started.  */
> -	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> -			 "synchronize_rcu_tasks called too soon");
> +	if (WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> +			 "synchronize_rcu_tasks called too soon"))
> +		return;
>  
>  	// If the grace-period kthread is running, use it.
>  	if (READ_ONCE(rtp->kthread_ptr)) {
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] rcu-tasks: Make synchronize_rcu_tasks_generic() no-ops on early booting

[Paul E. McKenney]

On Thu, Jul 14, 2022 at 01:53:20AM +0000, Zhang, Qiang1 wrote:
> 
> On Tue, Jul 12, 2022 at 04:26:06PM +0800, Zqiang wrote:
> > When the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE and not
> > yet converted to RCU_SCHEDULER_INIT, there is only idle task, any legal
> > call synchronize_rcu_tasks_generic() is a quiescent state. this commit
> > make synchronize_rcu_tasks_generic() no-ops when the rcu_scheduler_active
> > variable is RCU_SCHEDULER_INACTIVE.
> > 
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > ---
> 
> >It looks like this would be a good way to provide early boot access
> >to synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), and
> >synchronize_rcu_tasks_trace().
> >
> >But do we really need early boot access to these functions?  As in has
> >the below WARN_ON() actually triggered?
> 
> when the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE,
> invoke synchronize_rcu_tasks_generic(), in addition to triggering a warning, 
> also need to make it return directly, if not, the rcu_tasks_one_gp() will be
> called directly, but due to the rtp structure's -> pregp_func is not initialized,
> A null pointer bug will appear.
> 
> But like said, I don't see the need to call synchronize_rcu_tasks_generic() on 
> early booting.  maybe this change is  not necessary.

Not yet, anyway.  And adding this would require more testing.

However, if the current warning does trigger, and the caller has a
legitimate reason for invoking this function so early, please remember
this patch.  ;-)

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >And if it has triggered, and in a context that means that these functions
> >really are needed during early boot, how should the testing strategy
> >change to test these at the relevant portions of the boot sequence?
> >
> >>From what I know, hitting these during early boot would indicate that
> >something was removing a trace during early boot, and I know of no way
> >to make that happen.  Hence my skepticism.  ;-)
> >
> >But *if* this was really needed, this looks to be a reasonable way to
> >implement it.
> >
> >							Thanx, Paul
> 
> >  kernel/rcu/tasks.h | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 469bf2a3b505..0237e765c28e 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -560,8 +560,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
> >  static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
> >  {
> >  	/* Complain if the scheduler has not started.  */
> > -	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> > -			 "synchronize_rcu_tasks called too soon");
> > +	if (WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> > +			 "synchronize_rcu_tasks called too soon"))
> > +		return;
> >  
> >  	// If the grace-period kthread is running, use it.
> >  	if (READ_ONCE(rtp->kthread_ptr)) {
> > -- 
> > 2.25.1
> >