* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.