From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753385AbcE0LLX (ORCPT ); Fri, 27 May 2016 07:11:23 -0400 Received: from merlin.infradead.org ([205.233.59.134]:45550 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753163AbcE0LLV (ORCPT ); Fri, 27 May 2016 07:11:21 -0400 Date: Fri, 27 May 2016 13:11:17 +0200 From: Peter Zijlstra To: Alexey Dobriyan Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Paul McKenney Subject: Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Message-ID: <20160527111117.GL3192@twins.programming.kicks-ass.net> References: <20160521201448.GA7429@p183.telecom.by> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160521201448.GA7429@p183.telecom.by> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote: > lockless_dereference() is supposed to take pointer not integer. > > Signed-off-by: Alexey Dobriyan > --- > > include/linux/seqlock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- a/include/linux/seqlock.h > +++ b/include/linux/seqlock.h > @@ -277,7 +277,7 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s) > > static inline int raw_read_seqcount_latch(seqcount_t *s) > { > - return lockless_dereference(s->sequence); > + return lockless_dereference(s)->sequence; > } > > /** > @@ -331,7 +331,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s) > * unsigned seq, idx; > * > * do { > - * seq = lockless_dereference(latch->seq); > + * seq = lockless_dereference(latch)->seq; > * > * idx = seq & 0x01; > * entry = data_query(latch->data[idx], ...); So while the code was dubious; I it is now wrong, but my head hurts. I'll queue the below, TJs per-cpu change and the lockless_dereference() void * cast trick. --- Subject: seqcount: Re-fix raw_read_seqcount_latch() Commit 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") broke raw_read_seqcount_latch(). If you look at the comment that was modified; the thing that changes is the seq count, not the latch pointer. * void latch_modify(struct latch_struct *latch, ...) * { * smp_wmb(); <- Ensure that the last data[1] update is visible * latch->seq++; * smp_wmb(); <- Ensure that the seqcount update is visible * * modify(latch->data[0], ...); * * smp_wmb(); <- Ensure that the data[0] update is visible * latch->seq++; * smp_wmb(); <- Ensure that the seqcount update is visible * * modify(latch->data[1], ...); * } * * The query will have a form like: * * struct entry *latch_query(struct latch_struct *latch, ...) * { * struct entry *entry; * unsigned seq, idx; * * do { * seq = lockless_dereference(latch->seq); So here we have: seq = READ_ONCE(latch->seq); smp_read_barrier_depends(); Which is exactly what we want; the new code: seq = ({ p = READ_ONCE(latch); smp_read_barrier_depends(); p })->seq; is just wrong; because it looses the volatile read on seq, which can now be torn or worse 'optimized'. And the read_depend barrier is also placed wrong, we want it after the load of seq, to match the above data[] up-to-date wmb()s. Such that when we dereference latch->data[] below, we're guaranteed to observe the right data. * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...); * * smp_rmb(); * } while (seq != latch->seq); * * return entry; * } So yes, not passing a pointer is not pretty, but the code was correct, and isn't anymore now. Change to explicit READ_ONCE()+smp_read_barrier_depends() to avoid confusion and allow strict lockless_dereference() checking. Fixes: 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") Signed-off-by: Peter Zijlstra (Intel) --- include/linux/seqlock.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index 7973a821ac58..f3db247cebc8 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -277,7 +277,9 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s) static inline int raw_read_seqcount_latch(seqcount_t *s) { - return lockless_dereference(s)->sequence; + int seq = READ_ONCE(s->sequence); + smp_read_barrier_depends(); + return seq; } /** @@ -331,7 +333,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s) * unsigned seq, idx; * * do { - * seq = lockless_dereference(latch)->seq; + * seq = raw_read_seqcount_latch(&latch->seq); * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...);