On Tue, 3 Oct, 2023, 2:39 am Frederic Weisbecker, <frederic@xxxxxxxxxx> wrote:
>
> Le Mon, Oct 02, 2023 at 06:52:27PM +0530, Neeraj upadhyay a écrit :
> > On Mon, Oct 2, 2023 at 4:11 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> > > Also the role of the remaining advance in srcu_gp_start() is unclear to me...
> > >
> >
> > As far as I understand, the advance call before accelerate is to make
> > space in one of WAIT
> > or NEXT_READY tail for acceleration. So, it can be removed.
>
> Sounds good. Will remove that.
>
> >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 20d7a238d675..5ac81ca10ec8 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -782,8 +782,6 @@ static void srcu_gp_start(struct srcu_struct *ssp)
> > > spin_lock_rcu_node(sdp); /* Interrupts already disabled. */
> > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > - (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> > > - rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
> >
> > Deletion is ok; alternatively, we could have used
> > WARN_ON_ONCE(rcu_segcblist_accelerate(...))
> > in all places other than enqueue time for few cycles to be on safer side.
>
> How about WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL)) ?
>
Sounds good. My initial thought to put
WARN_ON_ONCE(rcu_segcblist_accelerate(...)) was to
catch any unexpected accelerations for WAIT and NEXT_READY cbs.
However, as all rcu_seq_snap()
snapshots, used for acceleration, are ordered, I think we do not need
to check for those tails.
> >
> > > spin_unlock_rcu_node(sdp); /* Interrupts remain disabled. */
> > > WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
> > > WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
> > > @@ -1245,7 +1243,18 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > s = rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq);
> > > - (void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> > > + /*
> > > + * Acceleration might fail if the preceding call to
> > > + * rcu_segcblist_advance() also failed due to a prior grace
> > > + * period seen incomplete before rcu_seq_snap(). If so then a new
> > > + * call to advance will see the completed grace period and fix
> > > + * the situation.
> > > + */
> > > + if (!rcu_segcblist_accelerate(&sdp->srcu_cblist, s)) {
> >
> > We can add below also? Here old and new are rcu_seq_current() values used in
> > the 2 calls to rcu_segcblist_advance().
> >
> > WARN_ON_ONCE(!(rcu_seq_completed_gp(old, new) && rcu_seq_new_gp(old, new)));
>
> Very good point! "new" should be exactly one and a half grace period away from
> "old", will add that.
>
> Cooking proper patches now.
>
Cool!
Thanks
Neeraj
> Thanks.
>
> >
> >
> > Thanks
> > Neeraj
> >
> > > + rcu_segcblist_advance(&sdp->srcu_cblist,
> > > + rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > + WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s));
> > > + }
> > > if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
> > > sdp->srcu_gp_seq_needed = s;
> > > needgp = true;
> > > @@ -1692,6 +1701,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > > ssp = sdp->ssp;
> > > rcu_cblist_init(&ready_cbs);
> > > spin_lock_irq_rcu_node(sdp);
> > > + WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > if (sdp->srcu_cblist_invoking ||
> > > @@ -1720,8 +1730,6 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > > */
> > > spin_lock_irq_rcu_node(sdp);
> > > rcu_segcblist_add_len(&sdp->srcu_cblist, -len);
> > > - (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> > > - rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
> > > sdp->srcu_cblist_invoking = false;
> > > more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
> > > spin_unlock_irq_rcu_node(sdp);
