On Wed, Sep 07, 2022 at 11:40:14AM +0200, Frederic Weisbecker wrote:
> On Wed, Sep 07, 2022 at 12:06:26AM +0000, Joel Fernandes wrote:
> > > > @@ -326,13 +372,20 @@ static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
> > > > * Note that this function always returns true if rhp is NULL.
> > > > */
> > > > static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
> > > > - unsigned long j)
> > > > + unsigned long j, unsigned long flush_flags)
> > > > {
> > > > + bool ret;
> > > > +
> > > > if (!rcu_rdp_is_offloaded(rdp))
> > > > return true;
> > > > rcu_lockdep_assert_cblist_protected(rdp);
> > > > rcu_nocb_bypass_lock(rdp);
> > > > - return rcu_nocb_do_flush_bypass(rdp, rhp, j);
> > > > + ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
> > > > +
> > > > + if (flush_flags & FLUSH_BP_WAKE)
> > > > + wake_nocb_gp(rdp, true);
> > >
> > > Why the true above?
> > >
> > > Also should we check if the wake up is really necessary (otherwise it means we
> > > force a wake up for all rdp's from rcu_barrier())?
> > >
> > > was_alldone = rcu_segcblist_pend_cbs(&rdp->cblist);
> > > ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
> > > if (was_alldone && rcu_segcblist_pend_cbs(&rdp->cblist))
> > > wake_nocb_gp(rdp, false);
> >
> > You mean something like the following right? Though I'm thinking if its
> > better to call wake_nocb_gp() from tree.c in entrain() and let that handle
> > the wake. That way, we can get rid of the extra FLUSH_BP flags as well and
> > let the flush callers deal with the wakeups..
>
> Ah yes that could make sense if only one caller cares.
>
> >
> > Anyway, for testing this should be good...
> >
> > ---8<-----------------------
> >
> > diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> > index bd8f39ee2cd0..e3344c262672 100644
> > --- a/kernel/rcu/tree_nocb.h
> > +++ b/kernel/rcu/tree_nocb.h
> > @@ -382,15 +382,19 @@ static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
> > unsigned long j, unsigned long flush_flags)
> > {
> > bool ret;
> > + bool was_alldone;
> >
> > if (!rcu_rdp_is_offloaded(rdp))
> > return true;
> > rcu_lockdep_assert_cblist_protected(rdp);
> > rcu_nocb_bypass_lock(rdp);
> > + if (flush_flags & FLUSH_BP_WAKE)
> > + was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
> > +
>
> You can check that outside bypass lock (but you still need nocb_lock).
>
> > ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
> >
> > - if (flush_flags & FLUSH_BP_WAKE)
> > - wake_nocb_gp(rdp, true);
> > + if (flush_flags & FLUSH_BP_WAKE && was_alldone)
> > + wake_nocb_gp(rdp, false);
>
> That doesn't check if the bypass list was empty.
I am ending up with something like the below for v6, after discussing with
Paul on IRC he pointed out we only need to do the rcu_barrier() related
wakeup when all the CBs are lazy in the bypass list. Otherwise timer goes
off. I think Frederic mentioned something similar above in different words.
I prefer to keep this logic in tree_nocb.h since rcu_barrier_entrain()
shouldn't have to deal with nocb internals (in theory anyway).
Looks Ok?
thanks,
- Joel
---8<-----------------------
From: "Joel Fernandes (Google)" <joel@xxxxxxxxxxxxxxxxx>
Subject: [PATCH for v6] fixup! rcu: Introduce call_rcu_lazy() API implementation
Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
---
kernel/rcu/tree_nocb.h | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
index c197534d0c99..fd056358f041 100644
--- a/kernel/rcu/tree_nocb.h
+++ b/kernel/rcu/tree_nocb.h
@@ -375,18 +375,26 @@ static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
unsigned long j, unsigned long flush_flags)
{
bool ret;
- bool was_alldone;
+ bool was_alldone = false;
+ bool bypass_all_lazy = false;
if (!rcu_rdp_is_offloaded(rdp))
return true;
rcu_lockdep_assert_cblist_protected(rdp);
rcu_nocb_bypass_lock(rdp);
- if (flush_flags & FLUSH_BP_WAKE)
+
+ if (flush_flags & FLUSH_BP_WAKE) {
was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
+ bypass_all_lazy =
+ (rcu_cblist_n_cbs(&rdp->nocb_bypass) == rdp->lazy_len);
+ }
ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
- if (flush_flags & FLUSH_BP_WAKE && was_alldone)
+ // Wake up the nocb GP thread if needed. GP thread could be sleeping
+ // while waiting for lazy timer to expire (otherwise rcu_barrier may
+ // end up waiting for the duration of the lazy timer).
+ if (flush_flags & FLUSH_BP_WAKE && was_alldone && bypass_all_lazy)
wake_nocb_gp(rdp, false);
return ret;
--
2.37.3.998.g577e59143f-goog
