From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753579AbbFKVqG (ORCPT ); Thu, 11 Jun 2015 17:46:06 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:54908 "EHLO e39.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750905AbbFKVqC (ORCPT ); Thu, 11 Jun 2015 17:46:02 -0400 X-Helo: d03dlp03.boulder.ibm.com X-MailFrom: paulmck@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Thu, 11 Jun 2015 14:45:57 -0700 From: "Paul E. McKenney" To: Peter Zijlstra Cc: umgwanakikbuti@gmail.com, mingo@elte.hu, ktkhai@parallels.com, rostedt@goodmis.org, tglx@linutronix.de, juri.lelli@gmail.com, pang.xunlei@linaro.org, oleg@redhat.com, wanpeng.li@linux.intel.com, linux-kernel@vger.kernel.org, Al Viro , Linus Torvalds Subject: Re: [PATCH 11/18] seqcount: Introduce raw_write_seqcount_barrier() Message-ID: <20150611214557.GA4249@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20150611124636.448700267@infradead.org> <20150611124743.374180021@infradead.org> <20150611153341.GK3913@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150611153341.GK3913@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15061121-0033-0000-0000-000004D1B260 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 11, 2015 at 08:33:41AM -0700, Paul E. McKenney wrote: > On Thu, Jun 11, 2015 at 02:46:47PM +0200, Peter Zijlstra wrote: > > Introduce raw_write_seqcount_barrier(), a new construct that can be > > used to provide write barrier semantics in seqcount read loops instead > > of the usual consistency guarantee. > > > > Cc: Al Viro > > Cc: Linus Torvalds > > Cc: Paul McKenney > > Suggested-by: Oleg Nesterov > > Signed-off-by: Peter Zijlstra (Intel) > > --- > > include/linux/seqlock.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > --- a/include/linux/seqlock.h > > +++ b/include/linux/seqlock.h > > @@ -233,6 +233,48 @@ static inline void raw_write_seqcount_en > > s->sequence++; > > } > > > > +/** > > + * raw_write_seqcount_barrier - do a seq write barrier > > + * @s: pointer to seqcount_t > > + * > > + * This can be used to provide an ordering guarantee instead of the > > + * usual consistency guarantee. It is one wmb cheaper, because we can > > + * collapse the two back-to-back wmb()s. > > + * > > + * seqcount_t seq; > > + * bool X = true, Y = false; > > + * > > + * void read(void) > > + * { > > + * bool x, y; > > + * > > + * do { > > + * int s = read_seqcount_begin(&seq); > > + * > > + * x = X; y = Y; > > + * > > + * } while (read_seqcount_retry(&seq, s)); > > + * > > + * BUG_ON(!x && !y); > > + * } > > + * > > + * void write(void) > > + * { > > + * Y = true; > > + * > > + * write_seqcount_begin(seq); > > + * write_seqcount_end(seq); > > + * > > + * X = false; > > + * } > > So when using this, write() would instead look like this? > > void write(void) > { > Y = true; > raw_write_seqcount_barrier(seq); > X = false; > } > > I suggest calling this out explicitly. Agreed, it should be obvious, > but some poor sot is going to be reading this at 3AM local time after > a couple days of no sleep, in which case obvious might not be so obvious. > > I also would suggest READ_ONCE() and WRITE_ONCE() to keep the compiler > trickiness down to a dull roar. Understood, it is hard to make anything > bad happen in this case, but small changes could result in badness. > > > + */ > > +static inline void raw_write_seqcount_barrier(seqcount_t *s) > > +{ > > + s->sequence++; > > + smp_wmb(); > > + s->sequence++; > > +} > > + > > /* > > * raw_write_seqcount_latch - redirect readers to even/odd copy > > * @s: pointer to seqcount_t > > Looks good otherwise. > > Reviewed-by: Paul E. McKenney Color me slow and stupid. Maybe due to reviewing a patch too early in the morning, who knows? There is nothing above that prevents the compiler and the CPU from reordering the assignments to X and Y with the increment of s->sequence++. One fix would be as follows: static inline void raw_write_seqcount_barrier(seqcount_t *s) { smp_wmb(); s->sequence++; smp_wmb(); s->sequence++; smp_wmb(); } Of course, this assumes that the accesses surrounding the call to raw_write_seqcount_barrier() are writes. If they can be a reads, the two added smp_wmb() calls need to be full barriers. Thanx, Paul