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)) ?
>
> > 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.
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);
