From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752666AbbGAWSP (ORCPT ); Wed, 1 Jul 2015 18:18:15 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:34747 "EHLO e35.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750935AbbGAWSL (ORCPT ); Wed, 1 Jul 2015 18:18:11 -0400 X-Helo: d03dlp03.boulder.ibm.com X-MailFrom: paulmck@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Wed, 1 Jul 2015 15:18:04 -0700 From: "Paul E. McKenney" To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, 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 RFC tip/core/rcu 05/14] rcu: Abstract sequence counting from synchronize_sched_expedited() Message-ID: <20150701221804.GW3717@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20150630222530.GA12044@linux.vnet.ibm.com> <1435703154-14659-1-git-send-email-paulmck@linux.vnet.ibm.com> <1435703154-14659-5-git-send-email-paulmck@linux.vnet.ibm.com> <20150701102717.GT19282@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150701102717.GT19282@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15070122-0013-0000-0000-000012F8A9BB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 01, 2015 at 12:27:17PM +0200, Peter Zijlstra wrote: > On Tue, Jun 30, 2015 at 03:25:45PM -0700, Paul E. McKenney wrote: > > From: "Paul E. McKenney" > > > > This commit creates rcu_exp_gp_seq_start() and rcu_exp_gp_seq_end() to > > bracket an expedited grace period, rcu_exp_gp_seq_snap() to snapshot the > > sequence counter, and rcu_exp_gp_seq_done() to check to see if a full > > expedited grace period has elapsed since the snapshot. These will be > > applied to synchronize_rcu_expedited(). These are defined in terms of > > underlying rcu_seq_start(), rcu_seq_end(), rcu_seq_snap(), rcu_seq_done(), > > which will be applied to _rcu_barrier(). > > It would be good to explain why you cannot use seqcount primitives. > They're >.< close. They are indeed! I gave it some thought, but it would inflict an unnecessary smp_mb() on seqlocks, as you note below. > > Signed-off-by: Paul E. McKenney > > --- > > kernel/rcu/tree.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 58 insertions(+), 10 deletions(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index c58fd27b4a22..f96500e462fd 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3307,6 +3307,60 @@ void cond_synchronize_sched(unsigned long oldstate) > > } > > EXPORT_SYMBOL_GPL(cond_synchronize_sched); > > > > +/* Adjust sequence number for start of update-side operation. */ > > +static void rcu_seq_start(unsigned long *sp) > > +{ > > + WRITE_ONCE(*sp, *sp + 1); > > + smp_mb(); /* Ensure update-side operation after counter increment. */ > > + WARN_ON_ONCE(!(*sp & 0x1)); > > +} > > That wants to be an ACQUIRE, right? I cannot put the acquire in the WARN_ON_ONCE() because there are configurations where WARN_ON_ONCE() is compiled out. I could conditionally compile, but given that this is nothing like a fastpath, I cannot really justify doing that. We could define an smp_store_acquire(), but that would require a full barrier against subsequent loads. The C++ committee hit this one when trying to implement seqeunce locking using the C/C++11 atomics. ;-) > > + > > +/* Adjust sequence number for end of update-side operation. */ > > +static void rcu_seq_end(unsigned long *sp) > > +{ > > + smp_mb(); /* Ensure update-side operation before counter increment. */ > > And that wants to be a RELEASE, right? > > > + WRITE_ONCE(*sp, *sp + 1); > > smp_store_release(); > > even if balanced against a full barrier, might be better here? I -think- it -might- be, and if it was in a fastpath, I might be more motivated to worry about it. I am not so sure that pairing an smp_store_release() with a full memory barrier is in any way an aid to readability, though. > > + WARN_ON_ONCE(*sp & 0x1); > > +} > > And the only difference between these and > raw_write_seqcount_{begin,end}() is the smp_wmb() vs your smp_mb(). > > Since seqcounts have a distinct read vs writer side, we really only care > about limiting the stores. I suspect you really do care about reads > between these 'sequence points'. A few words to that effect could > explain the existence of these primitives. Excellent point! I have updated the commit log accordingly. > > +/* Take a snapshot of the update side's sequence number. */ > > +static unsigned long rcu_seq_snap(unsigned long *sp) > > +{ > > + unsigned long s; > > + > > + smp_mb(); /* Caller's modifications seen first by other CPUs. */ > > + s = (READ_ONCE(*sp) + 3) & ~0x1; > > + smp_mb(); /* Above access must not bleed into critical section. */ > > smp_load_acquire() then? I have transitivity concerns. Which might well be baseless, but again, this is nowhere near a fastpath. > > + return s; > > +} > > + > > +/* > > + * Given a snapshot from rcu_seq_snap(), determine whether or not a > > + * full update-side operation has occurred. > > + */ > > +static bool rcu_seq_done(unsigned long *sp, unsigned long s) > > +{ > > + return ULONG_CMP_GE(READ_ONCE(*sp), s); > > I'm always amused you're not wanting to rely on 2s complement for > integer overflow. I _know_ its undefined behaviour in the C rule book, > but the entire rest of the kernel hard assumes it. I take it you have never seen the demonic glow in the eyes of a compiler implementer when thinking of all the code that can be broken^W^W^W^W^W optimizations that are enabled by relying on undefined behavior for signed integer overflow? ;-) > > +} > > + > > +/* Wrapper functions for expedited grace periods. */ > > +static void rcu_exp_gp_seq_start(struct rcu_state *rsp) > > +{ > > + rcu_seq_start(&rsp->expedited_sequence); > > +} > > +static void rcu_exp_gp_seq_end(struct rcu_state *rsp) > > +{ > > + rcu_seq_end(&rsp->expedited_sequence); > > +} > > +static unsigned long rcu_exp_gp_seq_snap(struct rcu_state *rsp) > > +{ > > + return rcu_seq_snap(&rsp->expedited_sequence); > > +} > > +static bool rcu_exp_gp_seq_done(struct rcu_state *rsp, unsigned long s) > > +{ > > + return rcu_seq_done(&rsp->expedited_sequence, s); > > +} > > This is wrappers for wrappers sake? Why? For _rcu_barrier(), as noted in the commit log. Thanx, Paul