Re: [PATCH v2 tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Lance Roy]

On Mon, 23 Jan 2017 11:12:07 -0800
"Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> wrote:

> On Mon, Jan 23, 2017 at 12:38:29AM -0800, Lance Roy wrote:
> > On Sun, 15 Jan 2017 14:42:34 -0800
> > "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > @@ -413,6 +415,8 @@ static void __synchronize_srcu(struct srcu_struct *sp,
> > > int trycount) 
> > >  	if (!done)
> > >  		wait_for_completion(&rcu.completion);
> > > +
> > > +	smp_mb(); /* Caller's later accesses after GP. */  
> > 
> > I think that this memory barrier is only necessary when done == false, as
> > otherwise srcu_advance_batches() should provide sufficient memory
> > ordering.  
> 
> Let me make sure that I understand your rationale here.
> 
> The idea is that although srcu_readers_active_idx_check() did a full
> memory barrier, this might have happened on some other CPU, which
> would not have provided ordering to the current CPU in the race case
> where current CPU didn't actually sleep.  (This can happen where the
> current task is preempted, and then is resumed just as the grace period
> completes.)
> 
> Or are you concerned about some other sequence of events?
Yes, the problem only occurs when the only full memory barrier is executed on a
different CPU.

> (I have moved the smp_mb() inside the "if (!done)" in the meantime.)
Thanks.

> > > @@ -587,6 +591,7 @@ static void srcu_invoke_callbacks(struct srcu_struct
> > > *sp) int i;
> > >  	struct rcu_head *head;
> > >  
> > > +	smp_mb(); /* Callback accesses after GP. */  
> > 
> > Shouldn't srcu_advance_batches() have already run all necessary memory
> > barriers?  
> 
> It does look that way:
> 
> o	process_srcu() is the only thing that invokes
> srcu_invoke_callbacks().
> 
> o	process_srcu() invokes srcu_advance_batches() immediately before
> 	srcu_invoke_callbacks(), so any memory barriers invoked from
> 	srcu_advance_batches() affect process_srcu() (unlike the earlier
> 	example where srcu_advance_batches() might be executed in the
> 	context of some other task).
> 
> o	srcu_advance_batches() unconditionally invokes try_check_zero(),
> 	which in turn unconditionally invokes srcu_readers_active_idx_check(),
> 	which in turn invokes smp_mb().
> 
> 	This smp_mb() precedes a successful check that all pre-existing
> 	readers are done, otherwise srcu_advance_batches() won't have
> 	returned (or won't have advanced the callbacks, which in turn
> 	will prevent them from being invoked).
> 
> I have removed this memory barrier and added a comment.
> 
> And thank you for your review and comments!!!

Thanks,
Lance