| Message ID | 20230112075629.1661429-1-qiang1.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] rcu: Fix the start_poll_synchronize_rcu_expedited() be invoked very early | expand |
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
> 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
> 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
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
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
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
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);
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(-)