Re: [PATCH] rcu: exp: Fix "must hold exp_mutex" comments for QS reporting functions

[Paul E. McKenney]

On Thu, Mar 08, 2018 at 04:30:06PM +0800, Boqun Feng wrote:
> On Thu, Mar 08, 2018 at 12:54:29PM +0800, Boqun Feng wrote:
> > 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 ;-)

The other limitation is that it occurs only on systems small enough
to have a single-node rcu_node tree.  But still...

> > > 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.

Very good, thank you!

> > > 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.
> 
> Hmm.. the other lockdep splat seems irrelevant with my patch, I could
> observe it on mainline using rcutorture with CONFIG_PROVE_LOCKING=y. I'd
> spend some more time on it, in the meanwhile, send a proper patch for
> this sync_rcu_preempt_exp_done().

I am not seeing that one, but am very interested in getting it fixed!  ;-)

							Thanx, Paul

> Regards,
> Boqun
> 
> > 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
> > > > 
> > > 
> > > 
> 
>