Messages in this thread | | | Date | Thu, 8 Mar 2018 12:54:29 +0800 | From | Boqun Feng <> | Subject | Re: [PATCH] rcu: exp: Fix "must hold exp_mutex" comments for QS reporting functions |
| |
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 > > > > [unhandled content-type:application/pgp-signature] | |