| Message ID | 20240117102616.18302-1-qiang.zhang1211@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 22c834462458099beeedd9ef6e7a55797022dd1b |
| Headers | show |
| Series | rcu/nocb: Check rdp_gp->nocb_timer in __call_rcu_nocb_wake() | expand |
Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > no-rdp_gp structure, the timer_pending() is always return false, > this commit therefore need to check rdp_gp->nocb_timer in > __call_rcu_nocb_wake(). > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > --- > kernel/rcu/tree_nocb.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > index 54971afc3a9b..3f85577bddd4 100644 > --- a/kernel/rcu/tree_nocb.h > +++ b/kernel/rcu/tree_nocb.h > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > long lazy_len; > long len; > struct task_struct *t; > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > // If we are being polled or there is no kthread, just leave. > t = READ_ONCE(rdp->nocb_gp_kthread); > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > smp_mb(); /* Enqueue before timer_pending(). */ > if ((rdp->nocb_cb_sleep || > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > - !timer_pending(&rdp->nocb_timer)) { > + !timer_pending(&rdp_gp->nocb_timer)) { Hehe, good eyes ;-) I had that change in mind but while checking that area further I actually wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If we reach that place, it means that the nocb_gp kthread should be awaken already (or the timer pending), so what does a force wake up solve in that case? Paul, any recollection of that? In the meantime: Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > no-rdp_gp structure, the timer_pending() is always return false, > > this commit therefore need to check rdp_gp->nocb_timer in > > __call_rcu_nocb_wake(). > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > --- > > kernel/rcu/tree_nocb.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > index 54971afc3a9b..3f85577bddd4 100644 > > --- a/kernel/rcu/tree_nocb.h > > +++ b/kernel/rcu/tree_nocb.h > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > long lazy_len; > > long len; > > struct task_struct *t; > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > // If we are being polled or there is no kthread, just leave. > > t = READ_ONCE(rdp->nocb_gp_kthread); > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > smp_mb(); /* Enqueue before timer_pending(). */ > > if ((rdp->nocb_cb_sleep || > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > - !timer_pending(&rdp->nocb_timer)) { > > + !timer_pending(&rdp_gp->nocb_timer)) { > > Hehe, good eyes ;-) > > I had that change in mind but while checking that area further I actually > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > we reach that place, it means that the nocb_gp kthread should be awaken > already (or the timer pending), so what does a force wake up solve in that > case? > > Paul, any recollection of that? Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed all the code paths correctly. Historically, I have been worried about lost wakeups. Also, there used to be code paths in which a wakeup was not needed, for example, because we knew that the ending of the current grace period would take care of things. Unless there was some huge pile of callbacks, in which case an immediate wakeup could avoid falling behind a callback flood. Given that rcutorture does test callback flooding, we appear to be OK, but maybe it is time to crank up the flooding more. On the other hand, I have started seeing the (very) occasional OOM on TREE03. (In addition to those that show up from time to time on the single-CPU TREE09 scenario.) Thanx, Paul > In the meantime: > > Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
On Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney wrote: > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > no-rdp_gp structure, the timer_pending() is always return false, > > > this commit therefore need to check rdp_gp->nocb_timer in > > > __call_rcu_nocb_wake(). > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > --- > > > kernel/rcu/tree_nocb.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > index 54971afc3a9b..3f85577bddd4 100644 > > > --- a/kernel/rcu/tree_nocb.h > > > +++ b/kernel/rcu/tree_nocb.h > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > long lazy_len; > > > long len; > > > struct task_struct *t; > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > // If we are being polled or there is no kthread, just leave. > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > if ((rdp->nocb_cb_sleep || > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > - !timer_pending(&rdp->nocb_timer)) { > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > Hehe, good eyes ;-) > > > > I had that change in mind but while checking that area further I actually > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > we reach that place, it means that the nocb_gp kthread should be awaken > > already (or the timer pending), so what does a force wake up solve in that > > case? > > > > Paul, any recollection of that? > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > all the code paths correctly. > > Historically, I have been worried about lost wakeups. Also, there > used to be code paths in which a wakeup was not needed, for example, > because we knew that the ending of the current grace period would take > care of things. Unless there was some huge pile of callbacks, in which > case an immediate wakeup could avoid falling behind a callback flood. > > Given that rcutorture does test callback flooding, we appear to be OK, > but maybe it is time to crank up the flooding more. > > On the other hand, I have started seeing the (very) occasional OOM > on TREE03. (In addition to those that show up from time to time on the > single-CPU TREE09 scenario.) Oh, and queued for further review and testing, thank you both! Thanx, Paul > > In the meantime: > > > > Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > no-rdp_gp structure, the timer_pending() is always return false, > > > this commit therefore need to check rdp_gp->nocb_timer in > > > __call_rcu_nocb_wake(). > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > --- > > > kernel/rcu/tree_nocb.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > index 54971afc3a9b..3f85577bddd4 100644 > > > --- a/kernel/rcu/tree_nocb.h > > > +++ b/kernel/rcu/tree_nocb.h > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > long lazy_len; > > > long len; > > > struct task_struct *t; > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > // If we are being polled or there is no kthread, just leave. > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > if ((rdp->nocb_cb_sleep || > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > - !timer_pending(&rdp->nocb_timer)) { > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > Hehe, good eyes ;-) > > > > I had that change in mind but while checking that area further I actually > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > we reach that place, it means that the nocb_gp kthread should be awaken > > already (or the timer pending), so what does a force wake up solve in that > > case? > > > > Paul, any recollection of that? > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > all the code paths correctly. > > Historically, I have been worried about lost wakeups. Also, there > used to be code paths in which a wakeup was not needed, for example, > because we knew that the ending of the current grace period would take > care of things. Unless there was some huge pile of callbacks, in which > case an immediate wakeup could avoid falling behind a callback flood. Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in my TODO list...unless Zqiang would like to give it a try? :-) > > Given that rcutorture does test callback flooding, we appear to be OK, > but maybe it is time to crank up the flooding more. > > On the other hand, I have started seeing the (very) occasional OOM > on TREE03. > (In addition to those that show up from time to time on the > single-CPU TREE09 scenario.) Interesting, are those recent? Bisectable? Thanks!
On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > __call_rcu_nocb_wake(). > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > --- a/kernel/rcu/tree_nocb.h > > > > +++ b/kernel/rcu/tree_nocb.h > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > long lazy_len; > > > > long len; > > > > struct task_struct *t; > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > if ((rdp->nocb_cb_sleep || > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > Hehe, good eyes ;-) > > > > > > I had that change in mind but while checking that area further I actually > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > already (or the timer pending), so what does a force wake up solve in that > > > case? > > > > > > Paul, any recollection of that? > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > all the code paths correctly. > > > > Historically, I have been worried about lost wakeups. Also, there > > used to be code paths in which a wakeup was not needed, for example, > > because we knew that the ending of the current grace period would take > > care of things. Unless there was some huge pile of callbacks, in which > > case an immediate wakeup could avoid falling behind a callback flood. > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > my TODO list...unless Zqiang would like to give it a try? :-) > > > Given that rcutorture does test callback flooding, we appear to be OK, > > but maybe it is time to crank up the flooding more. > > > > On the other hand, I have started seeing the (very) occasional OOM > > on TREE03. > > (In addition to those that show up from time to time on the > > single-CPU TREE09 scenario.) > > Interesting, are those recent? Bisectable? Bisection in progress, got it down to 10 commits. Yet again about ten hours per step on 20 systems... Though maybe I should have put more time into making it happen faster. Except that I was on travel, so I doubt that I would have made all that much progress. ;-) Thanx, Paul Thanx, Paul
On Fri, Jan 19, 2024 at 04:29:52PM -0800, Paul E. McKenney wrote: > On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > > __call_rcu_nocb_wake(). > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > --- > > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > > --- a/kernel/rcu/tree_nocb.h > > > > > +++ b/kernel/rcu/tree_nocb.h > > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > long lazy_len; > > > > > long len; > > > > > struct task_struct *t; > > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > > if ((rdp->nocb_cb_sleep || > > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > > > Hehe, good eyes ;-) > > > > > > > > I had that change in mind but while checking that area further I actually > > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > > already (or the timer pending), so what does a force wake up solve in that > > > > case? > > > > > > > > Paul, any recollection of that? > > > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > > all the code paths correctly. > > > > > > Historically, I have been worried about lost wakeups. Also, there > > > used to be code paths in which a wakeup was not needed, for example, > > > because we knew that the ending of the current grace period would take > > > care of things. Unless there was some huge pile of callbacks, in which > > > case an immediate wakeup could avoid falling behind a callback flood. > > > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > > my TODO list...unless Zqiang would like to give it a try? :-) > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > > but maybe it is time to crank up the flooding more. > > > > > > On the other hand, I have started seeing the (very) occasional OOM > > > on TREE03. > > > (In addition to those that show up from time to time on the > > > single-CPU TREE09 scenario.) > > > > Interesting, are those recent? Bisectable? > > Bisection in progress, got it down to 10 commits. Yet again about > ten hours per step on 20 systems... > > Though maybe I should have put more time into making it happen faster. > Except that I was on travel, so I doubt that I would have made all that > much progress. ;-) And it hit this one, which you encountered earlier: 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") Which you fixed with this guy: 600310bd7ea8 ("rcu: Defer RCU kthreads wakeup when CPU is dying") Which is not yet in -next. Which means I have spent an embarrassing amount of time bisecting a bug that you already fixed. C'est la vie! Given that v6.8-rc1 is now out, it is time to get a bunch of RCU commits into -next, including that one! ;-) Thanx, Paul
On Mon, Jan 22, 2024 at 04:07:09AM -0800, Paul E. McKenney wrote: > On Fri, Jan 19, 2024 at 04:29:52PM -0800, Paul E. McKenney wrote: > > On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > > > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > > > __call_rcu_nocb_wake(). > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > --- > > > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > > > --- a/kernel/rcu/tree_nocb.h > > > > > > +++ b/kernel/rcu/tree_nocb.h > > > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > long lazy_len; > > > > > > long len; > > > > > > struct task_struct *t; > > > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > > > if ((rdp->nocb_cb_sleep || > > > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > > > > > Hehe, good eyes ;-) > > > > > > > > > > I had that change in mind but while checking that area further I actually > > > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > > > already (or the timer pending), so what does a force wake up solve in that > > > > > case? > > > > > > > > > > Paul, any recollection of that? > > > > > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > > > all the code paths correctly. > > > > > > > > Historically, I have been worried about lost wakeups. Also, there > > > > used to be code paths in which a wakeup was not needed, for example, > > > > because we knew that the ending of the current grace period would take > > > > care of things. Unless there was some huge pile of callbacks, in which > > > > case an immediate wakeup could avoid falling behind a callback flood. > > > > > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > > > my TODO list...unless Zqiang would like to give it a try? :-) > > > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > > > but maybe it is time to crank up the flooding more. > > > > > > > > On the other hand, I have started seeing the (very) occasional OOM > > > > on TREE03. > > > > (In addition to those that show up from time to time on the > > > > single-CPU TREE09 scenario.) > > > > > > Interesting, are those recent? Bisectable? > > > > Bisection in progress, got it down to 10 commits. Yet again about > > ten hours per step on 20 systems... > > > > Though maybe I should have put more time into making it happen faster. > > Except that I was on travel, so I doubt that I would have made all that > > much progress. ;-) > > And it hit this one, which you encountered earlier: > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > Which you fixed with this guy: > > 600310bd7ea8 ("rcu: Defer RCU kthreads wakeup when CPU is dying") > > Which is not yet in -next. Which means I have spent an embarrassing > amount of time bisecting a bug that you already fixed. C'est la vie! > > Given that v6.8-rc1 is now out, it is time to get a bunch of RCU > commits into -next, including that one! ;-) Now to test your fix on top of the bad commit, and also on top of next-20240110. Just in case... Thanx, Paul
Le Mon, Jan 22, 2024 at 04:11:20AM -0800, Paul E. McKenney a écrit : > On Mon, Jan 22, 2024 at 04:07:09AM -0800, Paul E. McKenney wrote: > > On Fri, Jan 19, 2024 at 04:29:52PM -0800, Paul E. McKenney wrote: > > > On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > > > > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > > > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > > > > __call_rcu_nocb_wake(). > > > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > > --- > > > > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > > > > --- a/kernel/rcu/tree_nocb.h > > > > > > > +++ b/kernel/rcu/tree_nocb.h > > > > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > long lazy_len; > > > > > > > long len; > > > > > > > struct task_struct *t; > > > > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > > > > if ((rdp->nocb_cb_sleep || > > > > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > > > > > > > Hehe, good eyes ;-) > > > > > > > > > > > > I had that change in mind but while checking that area further I actually > > > > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > > > > already (or the timer pending), so what does a force wake up solve in that > > > > > > case? > > > > > > > > > > > > Paul, any recollection of that? > > > > > > > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > > > > all the code paths correctly. > > > > > > > > > > Historically, I have been worried about lost wakeups. Also, there > > > > > used to be code paths in which a wakeup was not needed, for example, > > > > > because we knew that the ending of the current grace period would take > > > > > care of things. Unless there was some huge pile of callbacks, in which > > > > > case an immediate wakeup could avoid falling behind a callback flood. > > > > > > > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > > > > my TODO list...unless Zqiang would like to give it a try? :-) > > > > > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > > > > but maybe it is time to crank up the flooding more. > > > > > > > > > > On the other hand, I have started seeing the (very) occasional OOM > > > > > on TREE03. > > > > > (In addition to those that show up from time to time on the > > > > > single-CPU TREE09 scenario.) > > > > > > > > Interesting, are those recent? Bisectable? > > > > > > Bisection in progress, got it down to 10 commits. Yet again about > > > ten hours per step on 20 systems... > > > > > > Though maybe I should have put more time into making it happen faster. > > > Except that I was on travel, so I doubt that I would have made all that > > > much progress. ;-) > > > > And it hit this one, which you encountered earlier: > > > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > > > Which you fixed with this guy: > > > > 600310bd7ea8 ("rcu: Defer RCU kthreads wakeup when CPU is dying") > > > > Which is not yet in -next. Which means I have spent an embarrassing > > amount of time bisecting a bug that you already fixed. C'est la vie! > > > > Given that v6.8-rc1 is now out, it is time to get a bunch of RCU > > commits into -next, including that one! ;-) > > Now to test your fix on top of the bad commit, and also on top of > next-20240110. Just in case... Thanks! I may send a v2 at some point within next week. No big change, just a comment and also expand the swake_up_one_online() logic to nocb kthreads as well. > > Thanx, Paul
On Mon, Jan 22, 2024 at 10:41:04PM +0100, Frederic Weisbecker wrote: > Le Mon, Jan 22, 2024 at 04:11:20AM -0800, Paul E. McKenney a écrit : > > On Mon, Jan 22, 2024 at 04:07:09AM -0800, Paul E. McKenney wrote: > > > On Fri, Jan 19, 2024 at 04:29:52PM -0800, Paul E. McKenney wrote: > > > > On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > > > > > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > > > > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > > > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > > > > > __call_rcu_nocb_wake(). > > > > > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > > > --- > > > > > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > > > > > --- a/kernel/rcu/tree_nocb.h > > > > > > > > +++ b/kernel/rcu/tree_nocb.h > > > > > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > > long lazy_len; > > > > > > > > long len; > > > > > > > > struct task_struct *t; > > > > > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > > > > > if ((rdp->nocb_cb_sleep || > > > > > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > > > > > > > > > Hehe, good eyes ;-) > > > > > > > > > > > > > > I had that change in mind but while checking that area further I actually > > > > > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > > > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > > > > > already (or the timer pending), so what does a force wake up solve in that > > > > > > > case? > > > > > > > > > > > > > > Paul, any recollection of that? > > > > > > > > > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > > > > > all the code paths correctly. > > > > > > > > > > > > Historically, I have been worried about lost wakeups. Also, there > > > > > > used to be code paths in which a wakeup was not needed, for example, > > > > > > because we knew that the ending of the current grace period would take > > > > > > care of things. Unless there was some huge pile of callbacks, in which > > > > > > case an immediate wakeup could avoid falling behind a callback flood. > > > > > > > > > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > > > > > my TODO list...unless Zqiang would like to give it a try? :-) > > > > > > > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > > > > > but maybe it is time to crank up the flooding more. > > > > > > > > > > > > On the other hand, I have started seeing the (very) occasional OOM > > > > > > on TREE03. > > > > > > (In addition to those that show up from time to time on the > > > > > > single-CPU TREE09 scenario.) > > > > > > > > > > Interesting, are those recent? Bisectable? > > > > > > > > Bisection in progress, got it down to 10 commits. Yet again about > > > > ten hours per step on 20 systems... > > > > > > > > Though maybe I should have put more time into making it happen faster. > > > > Except that I was on travel, so I doubt that I would have made all that > > > > much progress. ;-) > > > > > > And it hit this one, which you encountered earlier: > > > > > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > > > > > Which you fixed with this guy: > > > > > > 600310bd7ea8 ("rcu: Defer RCU kthreads wakeup when CPU is dying") > > > > > > Which is not yet in -next. Which means I have spent an embarrassing > > > amount of time bisecting a bug that you already fixed. C'est la vie! > > > > > > Given that v6.8-rc1 is now out, it is time to get a bunch of RCU > > > commits into -next, including that one! ;-) > > > > Now to test your fix on top of the bad commit, and also on top of > > next-20240110. Just in case... > > Thanks! > > I may send a v2 at some point within next week. No big change, just a comment > and also expand the swake_up_one_online() logic to nocb kthreads as well. Looking forward to it! Thanx, Paul
On Tue, Jan 23, 2024 at 02:48:19AM -0800, Paul E. McKenney wrote: > On Mon, Jan 22, 2024 at 10:41:04PM +0100, Frederic Weisbecker wrote: > > Le Mon, Jan 22, 2024 at 04:11:20AM -0800, Paul E. McKenney a écrit : > > > On Mon, Jan 22, 2024 at 04:07:09AM -0800, Paul E. McKenney wrote: > > > > On Fri, Jan 19, 2024 at 04:29:52PM -0800, Paul E. McKenney wrote: > > > > > On Fri, Jan 19, 2024 at 10:47:23PM +0100, Frederic Weisbecker wrote: > > > > > > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > > > > > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > > > > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > > > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > > > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > > > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > > > > > > __call_rcu_nocb_wake(). > > > > > > > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > > > > --- > > > > > > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > > > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > > > > > > --- a/kernel/rcu/tree_nocb.h > > > > > > > > > +++ b/kernel/rcu/tree_nocb.h > > > > > > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > > > long lazy_len; > > > > > > > > > long len; > > > > > > > > > struct task_struct *t; > > > > > > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > > > > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > > > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > > > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > > > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > > > > > > if ((rdp->nocb_cb_sleep || > > > > > > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > > > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > > > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > > > > > > > > > > > Hehe, good eyes ;-) > > > > > > > > > > > > > > > > I had that change in mind but while checking that area further I actually > > > > > > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > > > > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > > > > > > already (or the timer pending), so what does a force wake up solve in that > > > > > > > > case? > > > > > > > > > > > > > > > > Paul, any recollection of that? > > > > > > > > > > > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > > > > > > all the code paths correctly. > > > > > > > > > > > > > > Historically, I have been worried about lost wakeups. Also, there > > > > > > > used to be code paths in which a wakeup was not needed, for example, > > > > > > > because we knew that the ending of the current grace period would take > > > > > > > care of things. Unless there was some huge pile of callbacks, in which > > > > > > > case an immediate wakeup could avoid falling behind a callback flood. > > > > > > > > > > > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > > > > > > my TODO list...unless Zqiang would like to give it a try? :-) > > > > > > > > > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > > > > > > but maybe it is time to crank up the flooding more. > > > > > > > > > > > > > > On the other hand, I have started seeing the (very) occasional OOM > > > > > > > on TREE03. > > > > > > > (In addition to those that show up from time to time on the > > > > > > > single-CPU TREE09 scenario.) > > > > > > > > > > > > Interesting, are those recent? Bisectable? > > > > > > > > > > Bisection in progress, got it down to 10 commits. Yet again about > > > > > ten hours per step on 20 systems... > > > > > > > > > > Though maybe I should have put more time into making it happen faster. > > > > > Except that I was on travel, so I doubt that I would have made all that > > > > > much progress. ;-) > > > > > > > > And it hit this one, which you encountered earlier: > > > > > > > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > > > > > > > Which you fixed with this guy: > > > > > > > > 600310bd7ea8 ("rcu: Defer RCU kthreads wakeup when CPU is dying") > > > > > > > > Which is not yet in -next. Which means I have spent an embarrassing > > > > amount of time bisecting a bug that you already fixed. C'est la vie! > > > > > > > > Given that v6.8-rc1 is now out, it is time to get a bunch of RCU > > > > commits into -next, including that one! ;-) > > > > > > Now to test your fix on top of the bad commit, and also on top of > > > next-20240110. Just in case... > > > > Thanks! > > > > I may send a v2 at some point within next week. No big change, just a comment > > and also expand the swake_up_one_online() logic to nocb kthreads as well. > > Looking forward to it! And your fix on top of next-20240110 did the trick. Whew!!! ;-) Thanx, Paul
> > Le Thu, Jan 18, 2024 at 06:51:57AM -0800, Paul E. McKenney a écrit : > > On Wed, Jan 17, 2024 at 01:07:25PM +0100, Frederic Weisbecker wrote: > > > Le Wed, Jan 17, 2024 at 06:26:16PM +0800, Zqiang a écrit : > > > > Currently, only rdp_gp->nocb_timer is used, for nocb_timer of > > > > no-rdp_gp structure, the timer_pending() is always return false, > > > > this commit therefore need to check rdp_gp->nocb_timer in > > > > __call_rcu_nocb_wake(). > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/tree_nocb.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h > > > > index 54971afc3a9b..3f85577bddd4 100644 > > > > --- a/kernel/rcu/tree_nocb.h > > > > +++ b/kernel/rcu/tree_nocb.h > > > > @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > long lazy_len; > > > > long len; > > > > struct task_struct *t; > > > > + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; > > > > > > > > // If we are being polled or there is no kthread, just leave. > > > > t = READ_ONCE(rdp->nocb_gp_kthread); > > > > @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, > > > > smp_mb(); /* Enqueue before timer_pending(). */ > > > > if ((rdp->nocb_cb_sleep || > > > > !rcu_segcblist_ready_cbs(&rdp->cblist)) && > > > > - !timer_pending(&rdp->nocb_timer)) { > > > > + !timer_pending(&rdp_gp->nocb_timer)) { > > > > > > Hehe, good eyes ;-) > > > > > > I had that change in mind but while checking that area further I actually > > > wondered what is the actual purpose of this RCU_NOCB_WAKE_FORCE thing. If > > > we reach that place, it means that the nocb_gp kthread should be awaken > > > already (or the timer pending), so what does a force wake up solve in that > > > case? > > > > > > Paul, any recollection of that? > > > > Huh. We never actually do RCU_NOCB_WAKE_FORCE in v6.7, if I followed > > all the code paths correctly. > > > > Historically, I have been worried about lost wakeups. Also, there > > used to be code paths in which a wakeup was not needed, for example, > > because we knew that the ending of the current grace period would take > > care of things. Unless there was some huge pile of callbacks, in which > > case an immediate wakeup could avoid falling behind a callback flood. > > Ok then looks like it's time for me to add RCU_NOCB_WAKE_FORCE removal in > my TODO list...unless Zqiang would like to give it a try? :-) Thank you Frederic, apologies for the delayed reply. I'm currently traveling, after my vacation is over, I would like to try it if it hasn't been done yet :-) . Thanks Zqiang > > > > > Given that rcutorture does test callback flooding, we appear to be OK, > > but maybe it is time to crank up the flooding more. > > > > On the other hand, I have started seeing the (very) occasional OOM > > on TREE03. > > (In addition to those that show up from time to time on the > > single-CPU TREE09 scenario.) > > Interesting, are those recent? Bisectable? > > Thanks!
diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h index 54971afc3a9b..3f85577bddd4 100644 --- a/kernel/rcu/tree_nocb.h +++ b/kernel/rcu/tree_nocb.h @@ -564,6 +564,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, long lazy_len; long len; struct task_struct *t; + struct rcu_data *rdp_gp = rdp->nocb_gp_rdp; // If we are being polled or there is no kthread, just leave. t = READ_ONCE(rdp->nocb_gp_kthread); @@ -608,7 +609,7 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone, smp_mb(); /* Enqueue before timer_pending(). */ if ((rdp->nocb_cb_sleep || !rcu_segcblist_ready_cbs(&rdp->cblist)) && - !timer_pending(&rdp->nocb_timer)) { + !timer_pending(&rdp_gp->nocb_timer)) { rcu_nocb_unlock(rdp); wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_FORCE, TPS("WakeOvfIsDeferred"));
Currently, only rdp_gp->nocb_timer is used, for nocb_timer of no-rdp_gp structure, the timer_pending() is always return false, this commit therefore need to check rdp_gp->nocb_timer in __call_rcu_nocb_wake(). Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> --- kernel/rcu/tree_nocb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)