On Wed, Mar 07, 2018 at 08:30:17PM -0800, Paul E. McKenney wrote:
[...]
> >  
> > +/*
> > + * Like sync_rcu_preempt_exp_done(), but this function assumes the caller
> > + * doesn't hold the rcu_node's ->lock, and will acquire and release the 
> > lock
> > + * itself
> > + */
> > +static bool sync_rcu_preempt_exp_done_unlocked(struct rcu_node *rnp)
> > +{
> > +   unsigned long flags;
> > +   bool ret;
> > +
> > +   raw_spin_lock_irqsave_rcu_node(rnp, flags);
> > +   ret = sync_rcu_preempt_exp_done(rnp);
> 
> Let's see...  The sync_rcu_preempt_exp_done() function checks the
> ->exp_tasks pointer and the ->expmask bitmask.  The number of bits in the
> mask can only decrease, and the ->exp_tasks pointer can only transition
> from NULL to non-NULL when there is at least one bit set.  However,
> there is no ordering in sync_rcu_preempt_exp_done(), so it is possible
> that it could be fooled without the lock:
> 
> o     CPU 0 in sync_rcu_preempt_exp_done() reads ->exp_tasks and
>       sees that it is NULL.
> 
> o     CPU 1 blocks within an RCU read-side critical section, so
>       it enqueues the task and points ->exp_tasks at it and
>       clears CPU 1's bit in ->expmask.
> 
> o     All other CPUs clear their bits in ->expmask.
> 
> o     CPU 0 reads ->expmask, sees that it is zero, so incorrectly
>       concludes that all quiescent states have completed, despite
>       the fact that ->exp_tasks is non-NULL.
> 
> So it seems to me that the lock is needed.  Good catch!!!  The problem
> would occur only if the task running on CPU 0 received a spurious
> wakeup, but that could potentially happen.
> 

Thanks for the analysis ;-)

> If lock contention becomes a problem, memory-ordering tricks could be
> applied, but the lock is of course simpler.
> 

Agreed.

> I am guessing that this is a prototype patch, and that you are planning

Yes, this is a prototype. And I'm preparing a proper patch to send
later.

> to add lockdep annotations in more places, but either way please let
> me know.
> 

Give it's a bug as per your analysis, I'd like to defer other lockdep
annotations and send this first. However, I'm currently getting other
lockdep splats after applying this, so I need to get that sorted first.

Regards,
Boqun

>                                               Thanx, Paul
> 
> > +   raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> > +
> > +   return ret;
> > +}
> > +
> > +
> >  /*
> >   * Report the exit from RCU read-side critical section for the last task
> >   * that queued itself during or before the current expedited 
> > preemptible-RCU
> > @@ -490,6 +512,7 @@ static void synchronize_sched_expedited_wait(struct 
> > rcu_state *rsp)
> >     struct rcu_node *rnp;
> >     struct rcu_node *rnp_root = rcu_get_root(rsp);
> >     int ret;
> > +   unsigned long flags;
> >  
> >     trace_rcu_exp_grace_period(rsp->name, rcu_exp_gp_seq_endval(rsp), 
> > TPS("startwait"));
> >     jiffies_stall = rcu_jiffies_till_stall_check();
> > @@ -498,9 +521,9 @@ static void synchronize_sched_expedited_wait(struct 
> > rcu_state *rsp)
> >     for (;;) {
> >             ret = swait_event_timeout(
> >                             rsp->expedited_wq,
> > -                           sync_rcu_preempt_exp_done(rnp_root),
> > +                           sync_rcu_preempt_exp_done_unlocked(rnp_root),
> >                             jiffies_stall);
> > -           if (ret > 0 || sync_rcu_preempt_exp_done(rnp_root))
> > +           if (ret > 0 || sync_rcu_preempt_exp_done_unlocked(rnp_root))
> >                     return;
> >             WARN_ON(ret < 0);  /* workqueues should not be signaled. */
> >             if (rcu_cpu_stall_suppress)
> > @@ -533,8 +556,14 @@ static void synchronize_sched_expedited_wait(struct 
> > rcu_state *rsp)
> >                     rcu_for_each_node_breadth_first(rsp, rnp) {
> >                             if (rnp == rnp_root)
> >                                     continue; /* printed unconditionally */
> > -                           if (sync_rcu_preempt_exp_done(rnp))
> > +
> > +                           raw_spin_lock_irqsave_rcu_node(rnp, flags);
> > +                           if (sync_rcu_preempt_exp_done(rnp)) {
> > +                                   
> > raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> >                                     continue;
> > +                           }
> > +                           raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> > +
> >                             pr_cont(" l=%u:%d-%d:%#lx/%c",
> >                                     rnp->level, rnp->grplo, rnp->grphi,
> >                                     rnp->expmask,
> > -- 
> > 2.16.2
> > 
> 
> 

Attachment: signature.asc
Description: PGP signature

Reply via email to