Re: SRCU: kworker hung in synchronize_srcu

[Frederic Weisbecker <frederic@kernel.org>]

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@kernel.org> 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);