* [PATCH] seqlock: fix raw_read_seqcount_latch()
@ 2016-05-21 20:14 Alexey Dobriyan
2016-05-22 10:48 ` Peter Zijlstra
2016-05-27 11:11 ` [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra
0 siblings, 2 replies; 12+ messages in thread
From: Alexey Dobriyan @ 2016-05-21 20:14 UTC (permalink / raw)
To: akpm; +Cc: peterz, linux-kernel
lockless_dereference() is supposed to take pointer not integer.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
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], ...);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] seqlock: fix raw_read_seqcount_latch() 2016-05-21 20:14 [PATCH] seqlock: fix raw_read_seqcount_latch() Alexey Dobriyan @ 2016-05-22 10:48 ` Peter Zijlstra 2016-05-22 18:50 ` Alexey Dobriyan ` (2 more replies) 2016-05-27 11:11 ` [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra 1 sibling, 3 replies; 12+ messages in thread From: Peter Zijlstra @ 2016-05-22 10:48 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: akpm, linux-kernel, Paul McKenney 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); \ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] seqlock: fix raw_read_seqcount_latch() 2016-05-22 10:48 ` Peter Zijlstra @ 2016-05-22 18:50 ` Alexey Dobriyan 2016-05-23 9:36 ` Peter Zijlstra 2016-06-03 10:58 ` [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type tip-bot for Peter Zijlstra 2016-06-08 14:19 ` tip-bot for Peter Zijlstra 2 siblings, 1 reply; 12+ messages in thread From: Alexey Dobriyan @ 2016-05-22 18:50 UTC (permalink / raw) To: Peter Zijlstra; +Cc: akpm, linux-kernel, Paul McKenney 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); \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] seqlock: fix raw_read_seqcount_latch() 2016-05-22 18:50 ` Alexey Dobriyan @ 2016-05-23 9:36 ` Peter Zijlstra 2016-05-25 19:57 ` Tejun Heo 0 siblings, 1 reply; 12+ messages in thread From: Peter Zijlstra @ 2016-05-23 9:36 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: akpm, linux-kernel, Paul McKenney, Tejun Heo 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); \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] seqlock: fix raw_read_seqcount_latch() 2016-05-23 9:36 ` Peter Zijlstra @ 2016-05-25 19:57 ` Tejun Heo 2016-05-25 20:11 ` [PATCH] percpu: Revert ("percpu: Replace smp_read_barrier_depends() with lockless_dereference()") Tejun Heo 0 siblings, 1 reply; 12+ messages in thread From: Tejun Heo @ 2016-05-25 19:57 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Alexey Dobriyan, akpm, linux-kernel, Paul McKenney 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] percpu: Revert ("percpu: Replace smp_read_barrier_depends() with lockless_dereference()") 2016-05-25 19:57 ` Tejun Heo @ 2016-05-25 20:11 ` Tejun Heo 0 siblings, 0 replies; 12+ messages in thread From: Tejun Heo @ 2016-05-25 20:11 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Dobriyan, akpm, linux-kernel, Paul McKenney, Pranith Kumar, kernel-team lockless_dereference() is planned to grow a sanity check to ensure that the input parameter is a pointer. __ref_is_percpu() passes in an unsinged long value which is a combination of a pointer and a flag. While it can be casted to a pointer lvalue, the casting looks messy and it's a special case anyway. Let's revert back to open-coding READ_ONCE() and explicit barrier. This doesn't cause any functional changes. Signed-off-by: Tejun Heo <tj@kernel.org> Link: http://lkml.kernel.org/g/20160522185040.GA23664@p183.telecom.by Cc: Pranith Kumar <bobby.prani@gmail.com> Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> --- So, something like this. Please feel free to include in the series. Thanks. include/linux/percpu-refcount.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h index 84f542d..1c7eec0 100644 --- a/include/linux/percpu-refcount.h +++ b/include/linux/percpu-refcount.h @@ -136,14 +136,12 @@ static inline bool __ref_is_percpu(struct percpu_ref *ref, * used as a pointer. If the compiler generates a separate fetch * when using it as a pointer, __PERCPU_REF_ATOMIC may be set in * between contaminating the pointer value, meaning that - * ACCESS_ONCE() is required when fetching it. - * - * Also, we need a data dependency barrier to be paired with - * smp_store_release() in __percpu_ref_switch_to_percpu(). - * - * Use lockless deref which contains both. + * READ_ONCE() is required when fetching it. */ - percpu_ptr = lockless_dereference(ref->percpu_count_ptr); + percpu_ptr = READ_ONCE(ref->percpu_count_ptr); + + /* paired with smp_store_release() in __percpu_ref_switch_to_percpu() */ + smp_read_barrier_depends(); /* * Theoretically, the following could test just ATOMIC; however, ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type 2016-05-22 10:48 ` Peter Zijlstra 2016-05-22 18:50 ` Alexey Dobriyan @ 2016-06-03 10:58 ` tip-bot for Peter Zijlstra 2016-06-06 21:31 ` Mateusz Guzik 2016-06-08 14:19 ` tip-bot for Peter Zijlstra 2 siblings, 1 reply; 12+ messages in thread From: tip-bot for Peter Zijlstra @ 2016-06-03 10:58 UTC (permalink / raw) To: linux-tip-commits Cc: paulmck, akpm, torvalds, adobriyan, tglx, linux-kernel, mingo, hpa, peterz Commit-ID: 25841ee0e9d2a1d952828138416701f20ea831eb Gitweb: http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Sun, 22 May 2016 12:48:27 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 3 Jun 2016 12:06:11 +0200 locking/barriers: Validate lockless_dereference() is used on a pointer type Add a cast to void * to validate the argument @p is indeed a pointer. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/20160522104827.GP3193@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/compiler.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 793c082..e9c6417 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -545,9 +545,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s * Similar to rcu_dereference(), but for situations where the pointed-to * object's lifetime is managed by something other than RCU. That * "something other" might be reference counting or simple immortality. + * + * The seemingly unused void * variable is to validate @p is indeed a pointer + * type. All pointer types silently cast to void *. */ #define lockless_dereference(p) \ ({ \ + __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ (_________p1); \ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type 2016-06-03 10:58 ` [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type tip-bot for Peter Zijlstra @ 2016-06-06 21:31 ` Mateusz Guzik 2016-06-07 7:10 ` Peter Zijlstra 0 siblings, 1 reply; 12+ messages in thread From: Mateusz Guzik @ 2016-06-06 21:31 UTC (permalink / raw) To: Peter Zijlstra Cc: linux-tip-commits, paulmck, akpm, torvalds, adobriyan, tglx, linux-kernel, mingo, hpa, peterz On Fri, Jun 03, 2016 at 03:58:09AM -0700, tip-bot for Peter Zijlstra wrote: > Commit-ID: 25841ee0e9d2a1d952828138416701f20ea831eb > Gitweb: http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb > Author: Peter Zijlstra <peterz@infradead.org> > AuthorDate: Sun, 22 May 2016 12:48:27 +0200 > Committer: Ingo Molnar <mingo@kernel.org> > CommitDate: Fri, 3 Jun 2016 12:06:11 +0200 > > locking/barriers: Validate lockless_dereference() is used on a pointer type > > Add a cast to void * to validate the argument @p is indeed a pointer. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Alexey Dobriyan <adobriyan@gmail.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Cc: Paul McKenney <paulmck@linux.vnet.ibm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Link: http://lkml.kernel.org/r/20160522104827.GP3193@twins.programming.kicks-ass.net > Signed-off-by: Ingo Molnar <mingo@kernel.org> > --- > include/linux/compiler.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 793c082..e9c6417 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -545,9 +545,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s > * Similar to rcu_dereference(), but for situations where the pointed-to > * object's lifetime is managed by something other than RCU. That > * "something other" might be reference counting or simple immortality. > + * > + * The seemingly unused void * variable is to validate @p is indeed a pointer > + * type. All pointer types silently cast to void *. > */ > #define lockless_dereference(p) \ > ({ \ > + __maybe_unused const void * const _________p2 = p; \ > typeof(p) _________p1 = READ_ONCE(p); \ > smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ > (_________p1); \ This causes issues, e.g.: BUG: KASAN: user-memory-access on address 000060ff95001931^M Read of size 1 by task NetworkManager/897^M CPU: 1 PID: 897 Comm: NetworkManager Not tainted 4.7.0-rc1dupa+ #355^M Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011^M ffff88003a1f8040 0000000017d08b23 ffff880033aff4b0 ffffffff8187b2f8^M ffff88005b44f2c8 ffff880033aff548 ffff880033aff538 ffffffff813506a6^M ffffffff81167e5esystemd[1]: sshd.service: Forked /usr/sbin/sshd as 904^M ^M ffffed000675fe9d 0000000000000286 ffff880033aff588^M Call Trace:^M [<ffffffff8187b2f8>] dump_stack+0x85/0xcd^M [<ffffffff813506a6>] kasan_report_error+0x456/0x560^M [<ffffffff81167e5e>] ? vprintk_default+0x3e/0x60^M [<ffffffff812a19e3>] ? printk+0xa8/0xd8^M [<ffffffff812a193b>] ? power_down+0xa9/0xa9^M [<ffffffff81350d48>] kasan_report+0x58/0x60^M [<ffffffff81e84fe5>] ? leaf_walk_rcu+0x235/0x2d0^M [<ffffffff8134f447>] __asan_load1+0x47/0x50^M [<ffffffff81e84fe5>] leaf_walk_rcu+0x235/0x2d0^M [snip] The reason is that leaf_walk_rcu does get_child_rcu(pn, cindex++), which ends up in lockless_dereference as pn[cindex++], which is now evaluated twice. The simplest fix I see does the assignment later, that is: diff --git a/include/linux/compiler.h b/include/linux/compiler.h index e9c6417..06f27fd 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -551,8 +551,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s */ #define lockless_dereference(p) \ ({ \ - __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ + __maybe_unused const void * const _________p2 = _________p1; \ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ (_________p1); \ }) -- Mateusz Guzik ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type 2016-06-06 21:31 ` Mateusz Guzik @ 2016-06-07 7:10 ` Peter Zijlstra 0 siblings, 0 replies; 12+ messages in thread From: Peter Zijlstra @ 2016-06-07 7:10 UTC (permalink / raw) To: Mateusz Guzik Cc: linux-tip-commits, paulmck, akpm, torvalds, adobriyan, tglx, linux-kernel, mingo, hpa On Mon, Jun 06, 2016 at 11:31:39PM +0200, Mateusz Guzik wrote: > The reason is that leaf_walk_rcu does get_child_rcu(pn, cindex++), which > ends up in lockless_dereference as pn[cindex++], which is now evaluated > twice. Indeed, bad that; yes I think the below will work, will you send a proper patch so I can apply? > The simplest fix I see does the assignment later, that is: > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index e9c6417..06f27fd 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -551,8 +551,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s > */ > #define lockless_dereference(p) \ > ({ \ > - __maybe_unused const void * const _________p2 = p; \ > typeof(p) _________p1 = READ_ONCE(p); \ > + __maybe_unused const void * const _________p2 = _________p1; \ > smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ > (_________p1); \ > }) > > -- > Mateusz Guzik ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type 2016-05-22 10:48 ` Peter Zijlstra 2016-05-22 18:50 ` Alexey Dobriyan 2016-06-03 10:58 ` [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type tip-bot for Peter Zijlstra @ 2016-06-08 14:19 ` tip-bot for Peter Zijlstra 2 siblings, 0 replies; 12+ messages in thread From: tip-bot for Peter Zijlstra @ 2016-06-08 14:19 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, peterz, akpm, hpa, paulmck, adobriyan, mingo, torvalds, linux-kernel Commit-ID: 331b6d8c7afc2e5b900b9dcd850c265e1ba8d8e7 Gitweb: http://git.kernel.org/tip/331b6d8c7afc2e5b900b9dcd850c265e1ba8d8e7 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Sun, 22 May 2016 12:48:27 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Wed, 8 Jun 2016 14:22:47 +0200 locking/barriers: Validate lockless_dereference() is used on a pointer type Use the type to validate the argument @p is indeed a pointer type. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/20160522104827.GP3193@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/compiler.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 793c082..06f27fd 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -545,10 +545,14 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s * Similar to rcu_dereference(), but for situations where the pointed-to * object's lifetime is managed by something other than RCU. That * "something other" might be reference counting or simple immortality. + * + * The seemingly unused void * variable is to validate @p is indeed a pointer + * type. All pointer types silently cast to void *. */ #define lockless_dereference(p) \ ({ \ typeof(p) _________p1 = READ_ONCE(p); \ + __maybe_unused const void * const _________p2 = _________p1; \ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ (_________p1); \ }) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] seqlock: fix raw_read_seqcount_latch() 2016-05-21 20:14 [PATCH] seqlock: fix raw_read_seqcount_latch() Alexey Dobriyan 2016-05-22 10:48 ` Peter Zijlstra @ 2016-05-27 11:11 ` Peter Zijlstra 2016-06-03 10:46 ` [tip:locking/core] locking/seqcount: Re-fix raw_read_seqcount_latch() tip-bot for Peter Zijlstra 1 sibling, 1 reply; 12+ messages in thread From: Peter Zijlstra @ 2016-05-27 11:11 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: akpm, linux-kernel, Paul McKenney 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 <adobriyan@gmail.com> > --- > > 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) <peterz@infradead.org> --- 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], ...); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:locking/core] locking/seqcount: Re-fix raw_read_seqcount_latch() 2016-05-27 11:11 ` [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra @ 2016-06-03 10:46 ` tip-bot for Peter Zijlstra 0 siblings, 0 replies; 12+ messages in thread From: tip-bot for Peter Zijlstra @ 2016-06-03 10:46 UTC (permalink / raw) To: linux-tip-commits Cc: peterz, adobriyan, hpa, paulmck, linux-kernel, torvalds, tglx, akpm, mingo Commit-ID: 55eed755c6e30a89be3a791a6b0ad208aadd9bdc Gitweb: http://git.kernel.org/tip/55eed755c6e30a89be3a791a6b0ad208aadd9bdc Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Fri, 27 May 2016 13:11:17 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 3 Jun 2016 08:37:25 +0200 locking/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. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Fixes: 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") Link: http://lkml.kernel.org/r/20160527111117.GL3192@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/seqlock.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index 7973a82..ead9765 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -277,7 +277,10 @@ 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); + /* Pairs with the first smp_wmb() in raw_write_seqcount_latch() */ + smp_read_barrier_depends(); + return seq; } /** @@ -331,7 +334,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], ...); ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-06-08 14:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-21 20:14 [PATCH] seqlock: fix raw_read_seqcount_latch() Alexey Dobriyan
2016-05-22 10:48 ` Peter Zijlstra
2016-05-22 18:50 ` Alexey Dobriyan
2016-05-23 9:36 ` Peter Zijlstra
2016-05-25 19:57 ` Tejun Heo
2016-05-25 20:11 ` [PATCH] percpu: Revert ("percpu: Replace smp_read_barrier_depends() with lockless_dereference()") Tejun Heo
2016-06-03 10:58 ` [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type tip-bot for Peter Zijlstra
2016-06-06 21:31 ` Mateusz Guzik
2016-06-07 7:10 ` Peter Zijlstra
2016-06-08 14:19 ` tip-bot for Peter Zijlstra
2016-05-27 11:11 ` [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra
2016-06-03 10:46 ` [tip:locking/core] locking/seqcount: Re-fix raw_read_seqcount_latch() tip-bot for Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).