| Message ID | 20230109221756.1361168-1-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-rcu] rcu: Disable lazy if call_rcu() called when GPs expedited | expand |
On Mon, Jan 09, 2023 at 10:17:56PM +0000, Joel Fernandes (Google) wrote: > During suspend, we see failures to suspend 1 in 300-500 suspends. > Looking closer, it appears that we are queuing lazy callbacks even > though rcu_gp_is_expedited(). These delays appear to not be very welcome > by the suspend/resume code as evidenced by these occasional suspend > failures. > > This commit therefore checks if rcu_gp_is_expedited() and ignores the > lazy hint if so. > > Ignoring the lazy hint if rcu_gp_is_expedited() makes the 3000 > suspend/resume cycles pass reliably on a 12th gen 12-core Intel CPU. Yow!!! ;-) > Fixes: 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power") > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > Paul, could we take this for 6.2 -rc cycle? Thanks. > > kernel/rcu/tree.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 63545d79da51..93eb03f8ed99 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2594,12 +2594,12 @@ static void check_cb_ovld(struct rcu_data *rdp) > } > > static void > -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) > +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in) > { > static atomic_t doublefrees; > unsigned long flags; > struct rcu_data *rdp; > - bool was_alldone; > + bool was_alldone, lazy; Please put "lazy" in alpha order. Except that... > /* Misaligned rcu_head! */ > WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); > @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) > kasan_record_aux_stack_noalloc(head); > local_irq_save(flags); > rdp = this_cpu_ptr(&rcu_data); > + lazy = lazy_in && !rcu_gp_is_expedited(); Doesn't this completely disable laziness on Android? Thanx, Paul > /* Add the callback to our list. */ > if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) { > -- > 2.39.0.314.g84b9a713c41-goog
> On Jan 9, 2023, at 6:14 PM, Paul E. McKenney <paulmck@kernel.org> wrote: > > On Mon, Jan 09, 2023 at 10:17:56PM +0000, Joel Fernandes (Google) wrote: >> During suspend, we see failures to suspend 1 in 300-500 suspends. >> Looking closer, it appears that we are queuing lazy callbacks even >> though rcu_gp_is_expedited(). These delays appear to not be very welcome >> by the suspend/resume code as evidenced by these occasional suspend >> failures. >> >> This commit therefore checks if rcu_gp_is_expedited() and ignores the >> lazy hint if so. >> >> Ignoring the lazy hint if rcu_gp_is_expedited() makes the 3000 >> suspend/resume cycles pass reliably on a 12th gen 12-core Intel CPU. > > Yow!!! ;-) :-D >> Fixes: 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power") >> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> >> --- >> Paul, could we take this for 6.2 -rc cycle? Thanks. >> >> kernel/rcu/tree.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >> index 63545d79da51..93eb03f8ed99 100644 >> --- a/kernel/rcu/tree.c >> +++ b/kernel/rcu/tree.c >> @@ -2594,12 +2594,12 @@ static void check_cb_ovld(struct rcu_data *rdp) >> } >> >> static void >> -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) >> +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in) >> { >> static atomic_t doublefrees; >> unsigned long flags; >> struct rcu_data *rdp; >> - bool was_alldone; >> + bool was_alldone, lazy; > > Please put "lazy" in alpha order. Except that... Ah sure. > >> /* Misaligned rcu_head! */ >> WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); >> @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) >> kasan_record_aux_stack_noalloc(head); >> local_irq_save(flags); >> rdp = this_cpu_ptr(&rcu_data); >> + lazy = lazy_in && !rcu_gp_is_expedited(); > > Doesn't this completely disable laziness on Android? Good point, I am not sure but it could be. Maybe it is safer that I add a new suspend-indicator then, with corresponding suspend entry/exit calls like we do for expedited. That way anyone doing it this way will not disable lazy fully. Thoughts? Thanks! - Joel > > Thanx, Paul > >> /* Add the callback to our list. */ >> if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) { >> -- >> 2.39.0.314.g84b9a713c41-goog
On Mon, Jan 09, 2023 at 06:20:55PM -0500, Joel Fernandes wrote: > > > > On Jan 9, 2023, at 6:14 PM, Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Mon, Jan 09, 2023 at 10:17:56PM +0000, Joel Fernandes (Google) wrote: > >> During suspend, we see failures to suspend 1 in 300-500 suspends. > >> Looking closer, it appears that we are queuing lazy callbacks even > >> though rcu_gp_is_expedited(). These delays appear to not be very welcome > >> by the suspend/resume code as evidenced by these occasional suspend > >> failures. > >> > >> This commit therefore checks if rcu_gp_is_expedited() and ignores the > >> lazy hint if so. > >> > >> Ignoring the lazy hint if rcu_gp_is_expedited() makes the 3000 > >> suspend/resume cycles pass reliably on a 12th gen 12-core Intel CPU. > > > > Yow!!! ;-) > > :-D > > >> Fixes: 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power") > >> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > >> --- > >> Paul, could we take this for 6.2 -rc cycle? Thanks. > >> > >> kernel/rcu/tree.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 63545d79da51..93eb03f8ed99 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -2594,12 +2594,12 @@ static void check_cb_ovld(struct rcu_data *rdp) > >> } > >> > >> static void > >> -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) > >> +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in) > >> { > >> static atomic_t doublefrees; > >> unsigned long flags; > >> struct rcu_data *rdp; > >> - bool was_alldone; > >> + bool was_alldone, lazy; > > > > Please put "lazy" in alpha order. Except that... > > Ah sure. > > > > >> /* Misaligned rcu_head! */ > >> WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); > >> @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) > >> kasan_record_aux_stack_noalloc(head); > >> local_irq_save(flags); > >> rdp = this_cpu_ptr(&rcu_data); > >> + lazy = lazy_in && !rcu_gp_is_expedited(); > > > > Doesn't this completely disable laziness on Android? > > Good point, I am not sure but it could be. Maybe it is safer that I add > a new suspend-indicator then, with corresponding > suspend entry/exit calls like we do for expedited. > > That way anyone doing it this way will not disable > lazy fully. > > Thoughts? Makes sense to me! Just so you know, there is an overlapping patch series in flight here: https://lore.kernel.org/all/20221219202910.3063036-1-elliott@hpe.com/ Thanx, Paul
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 63545d79da51..93eb03f8ed99 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -2594,12 +2594,12 @@ static void check_cb_ovld(struct rcu_data *rdp) } static void -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in) { static atomic_t doublefrees; unsigned long flags; struct rcu_data *rdp; - bool was_alldone; + bool was_alldone, lazy; /* Misaligned rcu_head! */ WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy) kasan_record_aux_stack_noalloc(head); local_irq_save(flags); rdp = this_cpu_ptr(&rcu_data); + lazy = lazy_in && !rcu_gp_is_expedited(); /* Add the callback to our list. */ if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) {
During suspend, we see failures to suspend 1 in 300-500 suspends. Looking closer, it appears that we are queuing lazy callbacks even though rcu_gp_is_expedited(). These delays appear to not be very welcome by the suspend/resume code as evidenced by these occasional suspend failures. This commit therefore checks if rcu_gp_is_expedited() and ignores the lazy hint if so. Ignoring the lazy hint if rcu_gp_is_expedited() makes the 3000 suspend/resume cycles pass reliably on a 12th gen 12-core Intel CPU. Fixes: 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power") Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- Paul, could we take this for 6.2 -rc cycle? Thanks. kernel/rcu/tree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)