On Mon, 23 Jan 2017 11:12:07 -0800 "Paul E. McKenney" <paul...@linux.vnet.ibm.com> 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" <paul...@linux.vnet.ibm.com> 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
