On Mon, Oct 2, 2023 at 4:11 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>
> On Mon, Oct 02, 2023 at 07:47:55AM +0530, Neeraj upadhyay wrote:
> > On Mon, Oct 2, 2023 at 4:02 AM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> > >
> > > Le Sun, Oct 01, 2023 at 07:57:14AM +0530, Neeraj upadhyay a écrit :
> > > >
> > > > But "more" only checks for CBs in DONE tail. The callbacks which have been just
> > > > accelerated are not advanced to DONE tail.
> > > >
> > > > Having said above, I am still trying to figure out, which callbacks
> > > > are actually being pointed
> > > > by NEXT tail. Given that __call_srcu() already does a advance and
> > > > accelerate, all enqueued
> > > > callbacks would be in either WAIT tail (the callbacks for current
> > > > active GP) or NEXT_READY
> > > > tail (the callbacks for next GP after current one completes). So, they
> > > > should already have
> > > > GP num assigned and srcu_invoke_callbacks() won't accelerate them.
> > > > Only case I can
> > > > think of is, if current GP completes after we sample
> > > > rcu_seq_current(&ssp->srcu_gp_seq) for
> > > > rcu_segcblist_advance() (so, WAIT tail cbs are not moved to DONE tail)
> > > > and a new GP is started
> > > > before we take snapshot ('s') of next GP for
> > > > rcu_segcblist_accelerate(), then the gp num 's'
> > > > > gp num of NEXT_READY_TAIL and will be put in NEXT tail. Not sure
> > > > if my understanding is correct here. Thoughts?
> > > >
> > > > __call_srcu()
> > > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > > rcu_seq_current(&ssp->srcu_gp_seq));
> > > > s = rcu_seq_snap(&ssp->srcu_gp_seq);
> > > > (void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
> > >
> > > Good point! This looks plausible.
> > >
> > > Does the (buggy) acceleration in srcu_invoke_callbacks() exists solely
> > > to handle that case? Because then the acceleration could be moved before
> > > the advance on callbacks handling, something like:
> > >
> >
> > I think we might need to accelerate after advance, as the tail pointers
> > (WAIT, NEXT_READY) can be non-empty and we will not be able to
> > accelerate (and assign GP num) until we advance WAIT tail to DONE tail?
>
> Right indeed! How about the following patch then, assuming that in SRCU:
> 1 enqueue == 1 accelerate and therefore it only makes sense
> to accelerate on enqueue time and any other accelerate call is error prone.
>
Agree.
> I can't find a scenario where the second call below to accelerate (and thus also
> the second call to advance) would fail. Please tell me if I'm missing something.
Looks good to me. Few minor comments.
> 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.
> 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.
> 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)));
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);
