[v2] rcu: Fix the start_poll_synchronize_rcu_expedited() be invoked very early

  Currently, the start_poll_synchronize_rcu_expedited() can be invoked
very early. before rcu_init(), the rcu_data structure's->mynode is not
initialized, if invoke start_poll_synchronize_rcu_expedited() before
rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.

This commit add boot_exp_seq_poll_rq member to rcu_state structure to
store seq number return by invoke start_poll_synchronize_rcu_expedited()
very early.

Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 kernel/rcu/tree.c     | 3 ++-
 kernel/rcu/tree.h     | 1 +
 kernel/rcu/tree_exp.h | 6 ++++--
 3 files changed, 7 insertions(+), 3 deletions(-)
  

On Thu, Jan 12, 2023 at 03:56:29PM +0800, Zqiang wrote:
> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Just a nit below:

> ---
>  kernel/rcu/tree.c     | 3 ++-
>  kernel/rcu/tree.h     | 1 +
>  kernel/rcu/tree_exp.h | 6 ++++--
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 63545d79da51..12f0891ce7f4 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -92,6 +92,7 @@ static struct rcu_state rcu_state = {
>  	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
>  	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
>  	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
> +	.boot_exp_seq_poll_rq = RCU_GET_STATE_COMPLETED,
>  };
>  
>  /* Dump rcu_node combining tree at boot to verify correct setup. */
> @@ -4938,7 +4939,7 @@ void __init rcu_init(void)
>  		qovld_calc = qovld;
>  
>  	// Kick-start any polled grace periods that started early.
> -	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> +	if (!(rcu_state.boot_exp_seq_poll_rq & 0x1))

This can be "if (!(rcu_state.boot_exp_seq_poll_rq == RCU_GET_STATE_COMPLETED))" 

>  		(void)start_poll_synchronize_rcu_expedited();
>  
>  	rcu_test_sync_prims();
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 192536916f9a..ae50ca6853ad 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -397,6 +397,7 @@ struct rcu_state {
>  						/* Synchronize offline with */
>  						/*  GP pre-initialization. */
>  	int nocb_is_setup;			/* nocb is setup from boot */
> +	unsigned long boot_exp_seq_poll_rq;

A comment on the right can mention: "/* exp seq poll request before rcu_init() */"

Thanks!

>  };
>  
>  /* Values for rcu_state structure's gp_flags field. */
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 956cd459ba7f..1b35a1e233d9 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -1068,9 +1068,11 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
>  	if (rcu_init_invoked())
>  		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
>  	if (!poll_state_synchronize_rcu(s)) {
> -		rnp->exp_seq_poll_rq = s;
> -		if (rcu_init_invoked())
> +		if (rcu_init_invoked()) {
> +			rnp->exp_seq_poll_rq = s;
>  			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> +		} else
> +			rcu_state.boot_exp_seq_poll_rq = s;
>  	}
>  	if (rcu_init_invoked())
>  		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
> -- 
> 2.25.1
>
  

> On Jan 12, 2023, at 2:51 AM, Zqiang <qiang1.zhang@intel.com> wrote:
> 
> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks.



> ---
> kernel/rcu/tree.c     | 3 ++-
> kernel/rcu/tree.h     | 1 +
> kernel/rcu/tree_exp.h | 6 ++++--
> 3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 63545d79da51..12f0891ce7f4 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -92,6 +92,7 @@ static struct rcu_state rcu_state = {
>    .exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
>    .exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
>    .ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
> +    .boot_exp_seq_poll_rq = RCU_GET_STATE_COMPLETED,
> };
> 
> /* Dump rcu_node combining tree at boot to verify correct setup. */
> @@ -4938,7 +4939,7 @@ void __init rcu_init(void)
>        qovld_calc = qovld;
> 
>    // Kick-start any polled grace periods that started early.
> -    if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> +    if (!(rcu_state.boot_exp_seq_poll_rq & 0x1))
>        (void)start_poll_synchronize_rcu_expedited();
> 
>    rcu_test_sync_prims();
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 192536916f9a..ae50ca6853ad 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -397,6 +397,7 @@ struct rcu_state {
>                        /* Synchronize offline with */
>                        /*  GP pre-initialization. */
>    int nocb_is_setup;            /* nocb is setup from boot */
> +    unsigned long boot_exp_seq_poll_rq;
> };
> 
> /* Values for rcu_state structure's gp_flags field. */
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 956cd459ba7f..1b35a1e233d9 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -1068,9 +1068,11 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
>    if (rcu_init_invoked())
>        raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
>    if (!poll_state_synchronize_rcu(s)) {
> -        rnp->exp_seq_poll_rq = s;
> -        if (rcu_init_invoked())
> +        if (rcu_init_invoked()) {
> +            rnp->exp_seq_poll_rq = s;
>            queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> +        } else
> +            rcu_state.boot_exp_seq_poll_rq = s;
>    }
>    if (rcu_init_invoked())
>        raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
> -- 
> 2.25.1
>
  

On Thu, Jan 12, 2023 at 03:56:29PM +0800, Zqiang wrote:
> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

First off, excellent catch, Zqiang!!!

And thank you for Frederic and Joel for your reviews.

But I believe that this can be simplified, for example, as shown in
the (untested) patch below.

Thoughts?

And yes, I did presumptuously add Frederic's and Joel's reviews.
Please let me know if you disagree, and if so what different approach
you would prefer.  (Though of course simple disagreement is sufficient
for me to remove your tag.  Not holding you hostage for improvements,
not yet, anyway!)

							Thanx, Paul

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

commit e05af5cb3858e669c9e6b70e0aca708cc70457da
Author: Zqiang <qiang1.zhang@intel.com>
Date:   Thu Jan 12 10:48:29 2023 -0800

    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
    
    According to the commit log of the patch that added it to the kernel,
    start_poll_synchronize_rcu_expedited() can be invoked very early, as
    in long before rcu_init() has been invoked.  But before rcu_init(),
    the rcu_data structure's ->mynode field has not yet been initialized.
    This means that the start_poll_synchronize_rcu_expedited() function's
    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
    field will result in a segmentation fault.
    
    This commit therefore causes start_poll_synchronize_rcu_expedited() to
    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
    rcu_data structures' ->mynode fields.  It also removes the check from
    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
    is unconditionally invoked.  Yes, this might result in an unnecessary
    boot-time grace period, but this is down in the noise.  Besides, there
    only has to be one call_rcu() invoked prior to scheduler initialization
    to make this boot-time grace period necessary.
    
    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 63545d79da51c..f2e3a23778c06 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
 	else
 		qovld_calc = qovld;
 
-	// Kick-start any polled grace periods that started early.
-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
-		(void)start_poll_synchronize_rcu_expedited();
+	// Kick-start in case any polled grace periods started early.
+	(void)start_poll_synchronize_rcu_expedited();
 
 	rcu_test_sync_prims();
 }
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 956cd459ba7f3..3b7abb58157df 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
 	if (rcu_init_invoked())
 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
 	if (!poll_state_synchronize_rcu(s)) {
-		rnp->exp_seq_poll_rq = s;
-		if (rcu_init_invoked())
+		if (rcu_init_invoked()) {
+			rnp->exp_seq_poll_rq = s;
 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
+		}
 	}
 	if (rcu_init_invoked())
 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

On Thu, Jan 12, 2023 at 03:56:29PM +0800, Zqiang wrote:
> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>First off, excellent catch, Zqiang!!!
>
>And thank you for Frederic and Joel for your reviews.
>	
>But I believe that this can be simplified, for example, as shown in
>the (untested) patch below.
>
>Thoughts?

Agree, thanks for wordsmithed  😊.

>
>And yes, I did presumptuously add Frederic's and Joel's reviews.
>Please let me know if you disagree, and if so what different approach
>you would prefer.  (Though of course simple disagreement is sufficient
>for me to remove your tag.  Not holding you hostage for improvements,
>not yet, anyway!)
>
>							Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit e05af5cb3858e669c9e6b70e0aca708cc70457da
>Author: Zqiang <qiang1.zhang@intel.com>
>Date:   Thu Jan 12 10:48:29 2023 -0800
>
>    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
>    
>    According to the commit log of the patch that added it to the kernel,
>    start_poll_synchronize_rcu_expedited() can be invoked very early, as
>    in long before rcu_init() has been invoked.  But before rcu_init(),
>    the rcu_data structure's ->mynode field has not yet been initialized.
>    This means that the start_poll_synchronize_rcu_expedited() function's
>    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
>    field will result in a segmentation fault.
>    
>    This commit therefore causes start_poll_synchronize_rcu_expedited() to
>    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
>    rcu_data structures' ->mynode fields.  It also removes the check from
>    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
>    is unconditionally invoked.  Yes, this might result in an unnecessary
>    boot-time grace period, but this is down in the noise.  Besides, there
>    only has to be one call_rcu() invoked prior to scheduler initialization
>    to make this boot-time grace period necessary.
>    
>    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>index 63545d79da51c..f2e3a23778c06 100644
>--- a/kernel/rcu/tree.c
>+++ b/kernel/rcu/tree.c
>@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> 	else
> 		qovld_calc = qovld;
> 
>-	// Kick-start any polled grace periods that started early.
>-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
>-		(void)start_poll_synchronize_rcu_expedited();
>+	// Kick-start in case any polled grace periods started early.
>+	(void)start_poll_synchronize_rcu_expedited();
> 
> 	rcu_test_sync_prims();
> }
>diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
>index 956cd459ba7f3..3b7abb58157df 100644
>--- a/kernel/rcu/tree_exp.h
>+++ b/kernel/rcu/tree_exp.h
>@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> 	if (rcu_init_invoked())
> 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> 	if (!poll_state_synchronize_rcu(s)) {
>-		rnp->exp_seq_poll_rq = s;
>-		if (rcu_init_invoked())
>+		if (rcu_init_invoked()) {
>+			rnp->exp_seq_poll_rq = s;
> 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
>+		}
> 	}
> 	if (rcu_init_invoked())
> 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>First off, excellent catch, Zqiang!!!
>
>And thank you for Frederic and Joel for your reviews.
>	
>But I believe that this can be simplified, for example, as shown in
>the (untested) patch below.
>
>Thoughts?
>
>Agree, thanks for wordsmithed  😊.
>
>
>And yes, I did presumptuously add Frederic's and Joel's reviews.
>Please let me know if you disagree, and if so what different approach
>you would prefer.  (Though of course simple disagreement is sufficient
>for me to remove your tag.  Not holding you hostage for improvements,
>not yet, anyway!)
>
>							Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit e05af5cb3858e669c9e6b70e0aca708cc70457da
>Author: Zqiang <qiang1.zhang@intel.com>
>Date:   Thu Jan 12 10:48:29 2023 -0800
>
>    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
>    
>    According to the commit log of the patch that added it to the kernel,
>    start_poll_synchronize_rcu_expedited() can be invoked very early, as
>    in long before rcu_init() has been invoked.  But before rcu_init(),
>    the rcu_data structure's ->mynode field has not yet been initialized.
>    This means that the start_poll_synchronize_rcu_expedited() function's
>    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
>    field will result in a segmentation fault.
>    
>    This commit therefore causes start_poll_synchronize_rcu_expedited() to
>    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
>    rcu_data structures' ->mynode fields.  It also removes the check from
>    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
>    is unconditionally invoked.  Yes, this might result in an unnecessary
>    boot-time grace period, but this is down in the noise.  Besides, there
>    only has to be one call_rcu() invoked prior to scheduler initialization
>    to make this boot-time grace period necessary.

A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
grace period necessary 😊?

Thanks
Zqiang


>    
>    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>index 63545d79da51c..f2e3a23778c06 100644
>--- a/kernel/rcu/tree.c
>+++ b/kernel/rcu/tree.c
>@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> 	else
> 		qovld_calc = qovld;
> 
>-	// Kick-start any polled grace periods that started early.
>-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
>-		(void)start_poll_synchronize_rcu_expedited();
>+	// Kick-start in case any polled grace periods started early.
>+	(void)start_poll_synchronize_rcu_expedited();
> 
> 	rcu_test_sync_prims();
> }
>diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
>index 956cd459ba7f3..3b7abb58157df 100644
>--- a/kernel/rcu/tree_exp.h
>+++ b/kernel/rcu/tree_exp.h
>@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> 	if (rcu_init_invoked())
> 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> 	if (!poll_state_synchronize_rcu(s)) {
>-		rnp->exp_seq_poll_rq = s;
>-		if (rcu_init_invoked())
>+		if (rcu_init_invoked()) {
>+			rnp->exp_seq_poll_rq = s;
> 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
>+		}
> 	}
> 	if (rcu_init_invoked())
> 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

> Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> very early. before rcu_init(), the rcu_data structure's->mynode is not
> initialized, if invoke start_poll_synchronize_rcu_expedited() before
> rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> 
> This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> store seq number return by invoke start_poll_synchronize_rcu_expedited()
> very early.
> 
> Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>First off, excellent catch, Zqiang!!!
>
>And thank you for Frederic and Joel for your reviews.
>	
>But I believe that this can be simplified, for example, as shown in
>the (untested) patch below.
>
>Thoughts?
>
>Agree, thanks for wordsmithed  😊.
>
>
>And yes, I did presumptuously add Frederic's and Joel's reviews.
>Please let me know if you disagree, and if so what different approach
>you would prefer.  (Though of course simple disagreement is sufficient
>for me to remove your tag.  Not holding you hostage for improvements,
>not yet, anyway!)
>
>							Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit e05af5cb3858e669c9e6b70e0aca708cc70457da
>Author: Zqiang <qiang1.zhang@intel.com>
>Date:   Thu Jan 12 10:48:29 2023 -0800
>
>    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
>    
>    According to the commit log of the patch that added it to the kernel,
>    start_poll_synchronize_rcu_expedited() can be invoked very early, as
>    in long before rcu_init() has been invoked.  But before rcu_init(),
>    the rcu_data structure's ->mynode field has not yet been initialized.
>    This means that the start_poll_synchronize_rcu_expedited() function's
>    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
>    field will result in a segmentation fault.
>    
>    This commit therefore causes start_poll_synchronize_rcu_expedited() to
>    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
>    rcu_data structures' ->mynode fields.  It also removes the check from
>    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
>    is unconditionally invoked.  Yes, this might result in an unnecessary
>    boot-time grace period, but this is down in the noise.  Besides, there
>    only has to be one call_rcu() invoked prior to scheduler initialization
>    to make this boot-time grace period necessary.

A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
grace period necessary 😊?  affects call_rcu() is the normal grace period, but we are polling is
expedited grace period.  

Thanks
Zqiang


>    
>    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>index 63545d79da51c..f2e3a23778c06 100644
>--- a/kernel/rcu/tree.c
>+++ b/kernel/rcu/tree.c
>@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> 	else
> 		qovld_calc = qovld;
> 
>-	// Kick-start any polled grace periods that started early.
>-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
>-		(void)start_poll_synchronize_rcu_expedited();
>+	// Kick-start in case any polled grace periods started early.
>+	(void)start_poll_synchronize_rcu_expedited();
> 
> 	rcu_test_sync_prims();
> }
>diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
>index 956cd459ba7f3..3b7abb58157df 100644
>--- a/kernel/rcu/tree_exp.h
>+++ b/kernel/rcu/tree_exp.h
>@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> 	if (rcu_init_invoked())
> 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> 	if (!poll_state_synchronize_rcu(s)) {
>-		rnp->exp_seq_poll_rq = s;
>-		if (rcu_init_invoked())
>+		if (rcu_init_invoked()) {
>+			rnp->exp_seq_poll_rq = s;
> 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
>+		}
> 	}
> 	if (rcu_init_invoked())
> 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

On Fri, Jan 13, 2023 at 12:10:47PM +0000, Zhang, Qiang1 wrote:
> 
> > Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> > very early. before rcu_init(), the rcu_data structure's->mynode is not
> > initialized, if invoke start_poll_synchronize_rcu_expedited() before
> > rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> > 
> > This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> > store seq number return by invoke start_poll_synchronize_rcu_expedited()
> > very early.
> > 
> > Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> >
> >First off, excellent catch, Zqiang!!!
> >
> >And thank you for Frederic and Joel for your reviews.
> >	
> >But I believe that this can be simplified, for example, as shown in
> >the (untested) patch below.
> >
> >Thoughts?
> >
> >Agree, thanks for wordsmithed  😊.
> >
> >
> >And yes, I did presumptuously add Frederic's and Joel's reviews.
> >Please let me know if you disagree, and if so what different approach
> >you would prefer.  (Though of course simple disagreement is sufficient
> >for me to remove your tag.  Not holding you hostage for improvements,
> >not yet, anyway!)
> >
> >							Thanx, Paul
> >
> >------------------------------------------------------------------------
> >
> >commit e05af5cb3858e669c9e6b70e0aca708cc70457da
> >Author: Zqiang <qiang1.zhang@intel.com>
> >Date:   Thu Jan 12 10:48:29 2023 -0800
> >
> >    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
> >    
> >    According to the commit log of the patch that added it to the kernel,
> >    start_poll_synchronize_rcu_expedited() can be invoked very early, as
> >    in long before rcu_init() has been invoked.  But before rcu_init(),
> >    the rcu_data structure's ->mynode field has not yet been initialized.
> >    This means that the start_poll_synchronize_rcu_expedited() function's
> >    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
> >    field will result in a segmentation fault.
> >    
> >    This commit therefore causes start_poll_synchronize_rcu_expedited() to
> >    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
> >    rcu_data structures' ->mynode fields.  It also removes the check from
> >    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
> >    is unconditionally invoked.  Yes, this might result in an unnecessary
> >    boot-time grace period, but this is down in the noise.  Besides, there
> >    only has to be one call_rcu() invoked prior to scheduler initialization
> >    to make this boot-time grace period necessary.
> 
> A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
> grace period necessary 😊?

Because then there will be a callback queued that will require a grace
period to run anyway.

Or maybe you are asking if those callbacks will really be able to use
that first grace period?

							Thanx, Paul

> Thanks
> Zqiang
> 
> 
> >    
> >    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> >    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> >    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >
> >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >index 63545d79da51c..f2e3a23778c06 100644
> >--- a/kernel/rcu/tree.c
> >+++ b/kernel/rcu/tree.c
> >@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> > 	else
> > 		qovld_calc = qovld;
> > 
> >-	// Kick-start any polled grace periods that started early.
> >-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> >-		(void)start_poll_synchronize_rcu_expedited();
> >+	// Kick-start in case any polled grace periods started early.
> >+	(void)start_poll_synchronize_rcu_expedited();
> > 
> > 	rcu_test_sync_prims();
> > }
> >diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> >index 956cd459ba7f3..3b7abb58157df 100644
> >--- a/kernel/rcu/tree_exp.h
> >+++ b/kernel/rcu/tree_exp.h
> >@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > 	if (rcu_init_invoked())
> > 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > 	if (!poll_state_synchronize_rcu(s)) {
> >-		rnp->exp_seq_poll_rq = s;
> >-		if (rcu_init_invoked())
> >+		if (rcu_init_invoked()) {
> >+			rnp->exp_seq_poll_rq = s;
> > 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> >+		}
> > 	}
> > 	if (rcu_init_invoked())
> > 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

On Fri, Jan 13, 2023 at 12:10:47PM +0000, Zhang, Qiang1 wrote:
> 
> > Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> > very early. before rcu_init(), the rcu_data structure's->mynode is not
> > initialized, if invoke start_poll_synchronize_rcu_expedited() before
> > rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> > 
> > This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> > store seq number return by invoke start_poll_synchronize_rcu_expedited()
> > very early.
> > 
> > Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> >
> >First off, excellent catch, Zqiang!!!
> >
> >And thank you for Frederic and Joel for your reviews.
> >	
> >But I believe that this can be simplified, for example, as shown in
> >the (untested) patch below.
> >
> >Thoughts?
> >
> >Agree, thanks for wordsmithed  😊.
> >
> >
> >And yes, I did presumptuously add Frederic's and Joel's reviews.
> >Please let me know if you disagree, and if so what different approach
> >you would prefer.  (Though of course simple disagreement is sufficient
> >for me to remove your tag.  Not holding you hostage for improvements,
> >not yet, anyway!)
> >
> >							Thanx, Paul
> >
> >------------------------------------------------------------------------
> >
> >commit e05af5cb3858e669c9e6b70e0aca708cc70457da
> >Author: Zqiang <qiang1.zhang@intel.com>
> >Date:   Thu Jan 12 10:48:29 2023 -0800
> >
> >    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
> >    
> >    According to the commit log of the patch that added it to the kernel,
> >    start_poll_synchronize_rcu_expedited() can be invoked very early, as
> >    in long before rcu_init() has been invoked.  But before rcu_init(),
> >    the rcu_data structure's ->mynode field has not yet been initialized.
> >    This means that the start_poll_synchronize_rcu_expedited() function's
> >    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
> >    field will result in a segmentation fault.
> >    
> >    This commit therefore causes start_poll_synchronize_rcu_expedited() to
> >    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
> >    rcu_data structures' ->mynode fields.  It also removes the check from
> >    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
> >    is unconditionally invoked.  Yes, this might result in an unnecessary
> >    boot-time grace period, but this is down in the noise.  Besides, there
> >    only has to be one call_rcu() invoked prior to scheduler initialization
> >    to make this boot-time grace period necessary.
> 
> A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
> grace period necessary 😊?
>
>Because then there will be a callback queued that will require a grace
>period to run anyway.
>
>Or maybe you are asking if those callbacks will really be able to use
>that first grace period?

Yes, because even if we queue work to rcu_gp_wq workqueue, this also requires us to wait until
the workqueue_init() is invoked, our work can be execute. and also need to wait for the
rcu_gp kthread to be created, after this, our first grace period can begin, the callbacks has the
opportunity to be called.  the call_rcu() require a grace period, but we require is expedited grace period.

Thanks
Zqiang

>
>							Thanx, Paul

> Thanks
> Zqiang
> 
> 
> >    
> >    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> >    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> >    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >
> >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >index 63545d79da51c..f2e3a23778c06 100644
> >--- a/kernel/rcu/tree.c
> >+++ b/kernel/rcu/tree.c
> >@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> > 	else
> > 		qovld_calc = qovld;
> > 
> >-	// Kick-start any polled grace periods that started early.
> >-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> >-		(void)start_poll_synchronize_rcu_expedited();
> >+	// Kick-start in case any polled grace periods started early.
> >+	(void)start_poll_synchronize_rcu_expedited();
> > 
> > 	rcu_test_sync_prims();
> > }
> >diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> >index 956cd459ba7f3..3b7abb58157df 100644
> >--- a/kernel/rcu/tree_exp.h
> >+++ b/kernel/rcu/tree_exp.h
> >@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > 	if (rcu_init_invoked())
> > 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > 	if (!poll_state_synchronize_rcu(s)) {
> >-		rnp->exp_seq_poll_rq = s;
> >-		if (rcu_init_invoked())
> >+		if (rcu_init_invoked()) {
> >+			rnp->exp_seq_poll_rq = s;
> > 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> >+		}
> > 	}
> > 	if (rcu_init_invoked())
> > 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);
  

On Fri, Jan 13, 2023 at 03:02:54PM +0000, Zhang, Qiang1 wrote:
> On Fri, Jan 13, 2023 at 12:10:47PM +0000, Zhang, Qiang1 wrote:
> > 
> > > Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> > > very early. before rcu_init(), the rcu_data structure's->mynode is not
> > > initialized, if invoke start_poll_synchronize_rcu_expedited() before
> > > rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> > > 
> > > This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> > > store seq number return by invoke start_poll_synchronize_rcu_expedited()
> > > very early.
> > > 
> > > Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> > > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > >
> > >First off, excellent catch, Zqiang!!!
> > >
> > >And thank you for Frederic and Joel for your reviews.
> > >	
> > >But I believe that this can be simplified, for example, as shown in
> > >the (untested) patch below.
> > >
> > >Thoughts?
> > >
> > >Agree, thanks for wordsmithed  😊.
> > >
> > >
> > >And yes, I did presumptuously add Frederic's and Joel's reviews.
> > >Please let me know if you disagree, and if so what different approach
> > >you would prefer.  (Though of course simple disagreement is sufficient
> > >for me to remove your tag.  Not holding you hostage for improvements,
> > >not yet, anyway!)
> > >
> > >							Thanx, Paul
> > >
> > >------------------------------------------------------------------------
> > >
> > >commit e05af5cb3858e669c9e6b70e0aca708cc70457da
> > >Author: Zqiang <qiang1.zhang@intel.com>
> > >Date:   Thu Jan 12 10:48:29 2023 -0800
> > >
> > >    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
> > >    
> > >    According to the commit log of the patch that added it to the kernel,
> > >    start_poll_synchronize_rcu_expedited() can be invoked very early, as
> > >    in long before rcu_init() has been invoked.  But before rcu_init(),
> > >    the rcu_data structure's ->mynode field has not yet been initialized.
> > >    This means that the start_poll_synchronize_rcu_expedited() function's
> > >    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
> > >    field will result in a segmentation fault.
> > >    
> > >    This commit therefore causes start_poll_synchronize_rcu_expedited() to
> > >    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
> > >    rcu_data structures' ->mynode fields.  It also removes the check from
> > >    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
> > >    is unconditionally invoked.  Yes, this might result in an unnecessary
> > >    boot-time grace period, but this is down in the noise.  Besides, there
> > >    only has to be one call_rcu() invoked prior to scheduler initialization
> > >    to make this boot-time grace period necessary.
> > 
> > A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
> > grace period necessary 😊?
> >
> >Because then there will be a callback queued that will require a grace
> >period to run anyway.
> >
> >Or maybe you are asking if those callbacks will really be able to use
> >that first grace period?
> 
> Yes, because even if we queue work to rcu_gp_wq workqueue, this also requires us to wait until
> the workqueue_init() is invoked, our work can be execute. and also need to wait for the
> rcu_gp kthread to be created, after this, our first grace period can begin, the callbacks has the
> opportunity to be called.  the call_rcu() require a grace period, but we require is expedited grace period.

Good catch, thank you!  I will update the commit log accordingly.

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >							Thanx, Paul
> 
> > Thanks
> > Zqiang
> > 
> > 
> > >    
> > >    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > >    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> > >    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > >    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > >
> > >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > >index 63545d79da51c..f2e3a23778c06 100644
> > >--- a/kernel/rcu/tree.c
> > >+++ b/kernel/rcu/tree.c
> > >@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> > > 	else
> > > 		qovld_calc = qovld;
> > > 
> > >-	// Kick-start any polled grace periods that started early.
> > >-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> > >-		(void)start_poll_synchronize_rcu_expedited();
> > >+	// Kick-start in case any polled grace periods started early.
> > >+	(void)start_poll_synchronize_rcu_expedited();
> > > 
> > > 	rcu_test_sync_prims();
> > > }
> > >diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > >index 956cd459ba7f3..3b7abb58157df 100644
> > >--- a/kernel/rcu/tree_exp.h
> > >+++ b/kernel/rcu/tree_exp.h
> > >@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > > 	if (rcu_init_invoked())
> > > 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > > 	if (!poll_state_synchronize_rcu(s)) {
> > >-		rnp->exp_seq_poll_rq = s;
> > >-		if (rcu_init_invoked())
> > >+		if (rcu_init_invoked()) {
> > >+			rnp->exp_seq_poll_rq = s;
> > > 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> > >+		}
> > > 	}
> > > 	if (rcu_init_invoked())
> > > 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);