From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750797AbdAWIi3 (ORCPT ); Mon, 23 Jan 2017 03:38:29 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:34913 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750707AbdAWIi2 (ORCPT ); Mon, 23 Jan 2017 03:38:28 -0500 Date: Mon, 23 Jan 2017 00:38:29 -0800 From: Lance Roy To: "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, jiangshanlai@gmail.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com Subject: Re: [PATCH v2 tip/core/rcu 2/3] srcu: Force full grace-period ordering Message-ID: <20170123003829.3786251c@gmail.com> In-Reply-To: <1484520155-21017-2-git-send-email-paulmck@linux.vnet.ibm.com> References: <20170115224128.GA20492@linux.vnet.ibm.com> <1484520155-21017-2-git-send-email-paulmck@linux.vnet.ibm.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 15 Jan 2017 14:42:34 -0800 "Paul E. McKenney" wrote: > If a process invokes synchronize_srcu(), is delayed just the right amount > of time, and thus does not sleep when waiting for the grace period to > complete, there is no ordering between the end of the grace period and > the code following the synchronize_srcu(). Similarly, there can be a > lack of ordering between the end of the SRCU grace period and callback > invocation. > > This commit adds the necessary ordering. > > Reported-by: Lance Roy > Signed-off-by: Paul E. McKenney > --- > include/linux/rcupdate.h | 12 ++++++++++++ > kernel/rcu/srcu.c | 5 +++++ > kernel/rcu/tree.h | 12 ------------ > 3 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 01f71e1d2e94..6ade6a52d9d4 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -1161,5 +1161,17 @@ do { \ > ftrace_dump(oops_dump_mode); \ > } while (0) > > +/* > + * Place this after a lock-acquisition primitive to guarantee that > + * an UNLOCK+LOCK pair acts as a full barrier. This guarantee applies > + * if the UNLOCK and LOCK are executed by the same CPU or if the > + * UNLOCK and LOCK operate on the same lock variable. > + */ > +#ifdef CONFIG_PPC > +#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for > lock. */ +#else /* #ifdef CONFIG_PPC */ > +#define smp_mb__after_unlock_lock() do { } while (0) > +#endif /* #else #ifdef CONFIG_PPC */ > + > > #endif /* __LINUX_RCUPDATE_H */ > diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c > index ddabf5fbf562..f2abfbae258c 100644 > --- a/kernel/rcu/srcu.c > +++ b/kernel/rcu/srcu.c > @@ -359,6 +359,7 @@ void call_srcu(struct srcu_struct *sp, struct rcu_head > *head, head->next = NULL; > head->func = func; > spin_lock_irqsave(&sp->queue_lock, flags); > + smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */ > rcu_batch_queue(&sp->batch_queue, head); > if (!sp->running) { > sp->running = true; > @@ -392,6 +393,7 @@ static void __synchronize_srcu(struct srcu_struct *sp, > int trycount) head->next = NULL; > head->func = wakeme_after_rcu; > spin_lock_irq(&sp->queue_lock); > + smp_mb__after_unlock_lock(); /* Caller's prior accesses before GP. */ > if (!sp->running) { > /* steal the processing owner */ > sp->running = true; > @@ -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. > } > > /** > @@ -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? > for (i = 0; i < SRCU_CALLBACK_BATCH; i++) { > head = rcu_batch_dequeue(&sp->batch_done); > if (!head) > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h > index fe98dd24adf8..abcc25bdcb29 100644 > --- a/kernel/rcu/tree.h > +++ b/kernel/rcu/tree.h > @@ -688,18 +688,6 @@ static inline void rcu_nocb_q_lengths(struct rcu_data > *rdp, long *ql, long *qll) #endif /* #ifdef CONFIG_RCU_TRACE */ > > /* > - * Place this after a lock-acquisition primitive to guarantee that > - * an UNLOCK+LOCK pair act as a full barrier. This guarantee applies > - * if the UNLOCK and LOCK are executed by the same CPU or if the > - * UNLOCK and LOCK operate on the same lock variable. > - */ > -#ifdef CONFIG_PPC > -#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for > lock. */ -#else /* #ifdef CONFIG_PPC */ > -#define smp_mb__after_unlock_lock() do { } while (0) > -#endif /* #else #ifdef CONFIG_PPC */ > - > -/* > * Wrappers for the rcu_node::lock acquire and release. > * > * Because the rcu_nodes form a tree, the tree traversal locking will observe Thanks, Lance