Re: [PATCH] seqlock: fix raw_read_seqcount_latch()
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], ...);
Re: [PATCH] seqlock: fix raw_read_seqcount_latch()
Hello, On Mon, May 23, 2016 at 11:36:18AM +0200, Peter Zijlstra wrote: > > include/linux/percpu-refcount.h:146:36: warning: initialization makes > > pointer from integer without a cast [-Wint-conversion] > > percpu_ptr = lockless_dereference(ref->percpu_count_ptr); > > TJ; would you prefer casting or not using lockless_dereference() here? Casting is nasty - *(unsigned long __percpu **)& - because the macro expects an lvalue. I think it'd be better to revert to opencoding READ_ONCE() and barrier there. It's a pretty special case anyway. Thanks. -- tejun
Re: [PATCH] seqlock: fix raw_read_seqcount_latch()
On Sun, May 22, 2016 at 09:50:40PM +0300, Alexey Dobriyan wrote: > On Sun, May 22, 2016 at 12:48:27PM +0200, Peter Zijlstra wrote: > > On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote: > > > lockless_dereference() is supposed to take pointer not integer. > > > > Urgh :/ > > > > Is there any way we can make lockless_dereference() issue a warning if > > we don't feed it a pointer? > > > > Would something like so work? All pointer types should silently cast to > > void * while integer (and others) should refuse to. > > This works (and spammy enough in case of seqlock, which is good) > but not for "unsigned long": > > include/linux/percpu-refcount.h:146:36: warning: initialization makes > pointer from integer without a cast [-Wint-conversion] > percpu_ptr = lockless_dereference(ref->percpu_count_ptr); TJ; would you prefer casting or not using lockless_dereference() here? > > > --- a/include/linux/compiler.h > > +++ b/include/linux/compiler.h > > @@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile > > void *p, void *res, int s > > */ > > #define lockless_dereference(p) \ > > ({ \ > > + __maybe_unused void * _p2 = p; \ > > typeof(p) _p1 = READ_ONCE(p); \ > > smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ > > (_p1); \
Re: [PATCH] seqlock: fix raw_read_seqcount_latch()
On Sun, May 22, 2016 at 12:48:27PM +0200, Peter Zijlstra wrote: > On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote: > > lockless_dereference() is supposed to take pointer not integer. > > Urgh :/ > > Is there any way we can make lockless_dereference() issue a warning if > we don't feed it a pointer? > > Would something like so work? All pointer types should silently cast to > void * while integer (and others) should refuse to. This works (and spammy enough in case of seqlock, which is good) but not for "unsigned long": include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion] percpu_ptr = lockless_dereference(ref->percpu_count_ptr); > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile > void *p, void *res, int s > */ > #define lockless_dereference(p) \ > ({ \ > + __maybe_unused void * _p2 = p; \ > typeof(p) _p1 = READ_ONCE(p); \ > smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ > (_p1); \
Re: [PATCH] seqlock: fix raw_read_seqcount_latch()
On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote: > lockless_dereference() is supposed to take pointer not integer. Urgh :/ Is there any way we can make lockless_dereference() issue a warning if we don't feed it a pointer? Would something like so work? All pointer types should silently cast to void * while integer (and others) should refuse to. diff --git a/include/linux/compiler.h b/include/linux/compiler.h index b5ff9881bef8..8886de704d33 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s */ #define lockless_dereference(p) \ ({ \ + __maybe_unused void * _p2 = p; \ typeof(p) _p1 = READ_ONCE(p); \ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ (_p1); \
[PATCH] seqlock: fix raw_read_seqcount_latch()
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], ...);