From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Victor Kaplansky <VICTORK@il.ibm.com>, Oleg Nesterov <oleg@redhat.com>, Anton Blanchard <anton@samba.org>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Frederic Weisbecker <fweisbec@gmail.com>, LKML <linux-kernel@vger.kernel.org>, Linux PPC dev <linuxppc-dev@ozlabs.org>, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>, Michael Ellerman <michael@ellerman.id.au>, Michael Neuling <mikey@neuling.org> Subject: Re: perf events ring buffer memory barrier on powerpc Date: Sat, 2 Nov 2013 10:46:45 -0700 [thread overview] Message-ID: <20131102174645.GC3947@linux.vnet.ibm.com> (raw) In-Reply-To: <20131101161129.GU16117@laptop.programming.kicks-ass.net> On Fri, Nov 01, 2013 at 05:11:29PM +0100, Peter Zijlstra wrote: > On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote: > > > void kbuf_write(int sz, void *buf) > > > { > > > u64 tail = ACCESS_ONCE(ubuf->tail); /* last location userspace read */ > > > u64 offset = kbuf->head; /* we already know where we last wrote */ > > > u64 head = offset + sz; > > > > > > if (!space(tail, offset, head)) { > > > /* discard @buf */ > > > return; > > > } > > > > > > /* > > > * Ensure that if we see the userspace tail (ubuf->tail) such > > > * that there is space to write @buf without overwriting data > > > * userspace hasn't seen yet, we won't in fact store data before > > > * that read completes. > > > */ > > > > > > smp_mb(); /* A, matches with D */ > > > > > > write(kbuf->data + offset, buf, sz); > > > kbuf->head = head % kbuf->size; > > > > > > /* > > > * Ensure that we write all the @buf data before we update the > > > * userspace visible ubuf->head pointer. > > > */ > > > smp_wmb(); /* B, matches with C */ > > > > > > ubuf->head = kbuf->head; > > > } > > > > Now the whole crux of the question is if we need barrier A at all, since > > > the STORES issued by the @buf writes are dependent on the ubuf->tail > > > read. > > > > The dependency you are talking about is via the "if" statement? > > Even C/C++11 is not required to respect control dependencies. > > But surely we must be able to make it so; otherwise you'd never be able > to write: > > void *ptr = obj1; > > void foo(void) > { > > /* create obj2, obj3 */ > > smp_wmb(); /* ensure the objs are complete */ > > /* expose either obj2 or obj3 */ > if (x) > ptr = obj2; > else > ptr = obj3; OK, the smp_wmb() orders the creation and the exposing. But the compiler can do this: ptr = obj3; if (x) ptr = obj2; And that could momentarily expose obj3 to readers, and these readers might be fatally disappointed by the free() below. If you instead said: if (x) ACCESS_ONCE(ptr) = obj2; else ACCESS_ONCE(ptr) = obj3; then the general consensus appears to be that the compiler would not be permitted to carry out the above optimization. Since you have the smp_wmb(), readers that are properly ordered (e.g., smp_rmb() or rcu_dereference()) would be prevented from seeing pre-initialization state. > /* free the unused one */ > if (x) > free(obj3); > else > free(obj2); > } > > Earlier you said that 'volatile' or '__atomic' avoids speculative > writes; so would: > > volatile void *ptr = obj1; > > Make the compiler respect control dependencies again? If so, could we > somehow mark that !space() condition volatile? The compiler should, but the CPU is still free to ignore the control dependencies in the general case. We might be able to rely on weakly ordered hardware refraining from speculating stores, but not sure that this applies across all architectures of interest. We definitely can -not- rely on weakly ordered hardware refraining from speculating loads. > Currently the above would be considered a valid pattern. But you're > saying its not because the compiler is free to expose both obj2 and obj3 > (for however short a time) and thus the free of the 'unused' object is > incorrect and can cause use-after-free. Yes, it is definitely unsafe and invalid in absence of ACCESS_ONCE(). > In fact; how can we be sure that: > > void *ptr = NULL; > > void bar(void) > { > void *obj = malloc(...); > > /* fill obj */ > > if (!err) > rcu_assign_pointer(ptr, obj); > else > free(obj); > } > > Does not get 'optimized' into: > > void bar(void) > { > void *obj = malloc(...); > void *old_ptr = ptr; > > /* fill obj */ > > rcu_assign_pointer(ptr, obj); > if (err) { /* because runtime profile data says this is unlikely */ > ptr = old_ptr; > free(obj); > } > } In this particular case, the barrier() implied by the smp_wmb() in rcu_assign_pointer() will prevent this "optimization". However, other "optimizations" are the reason why I am working to introduce ACCESS_ONCE() into rcu_assign_pointer. > We _MUST_ be able to rely on control flow, otherwise me might as well > all go back to writing kernels in asm. It isn't -that- bad! ;-) Thanx, Paul
WARNING: multiple messages have this Message-ID (diff)
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Michael Neuling <mikey@neuling.org>, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>, Oleg Nesterov <oleg@redhat.com>, LKML <linux-kernel@vger.kernel.org>, Linux PPC dev <linuxppc-dev@ozlabs.org>, Anton Blanchard <anton@samba.org>, Frederic Weisbecker <fweisbec@gmail.com>, Victor Kaplansky <VICTORK@il.ibm.com> Subject: Re: perf events ring buffer memory barrier on powerpc Date: Sat, 2 Nov 2013 10:46:45 -0700 [thread overview] Message-ID: <20131102174645.GC3947@linux.vnet.ibm.com> (raw) In-Reply-To: <20131101161129.GU16117@laptop.programming.kicks-ass.net> On Fri, Nov 01, 2013 at 05:11:29PM +0100, Peter Zijlstra wrote: > On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote: > > > void kbuf_write(int sz, void *buf) > > > { > > > u64 tail = ACCESS_ONCE(ubuf->tail); /* last location userspace read */ > > > u64 offset = kbuf->head; /* we already know where we last wrote */ > > > u64 head = offset + sz; > > > > > > if (!space(tail, offset, head)) { > > > /* discard @buf */ > > > return; > > > } > > > > > > /* > > > * Ensure that if we see the userspace tail (ubuf->tail) such > > > * that there is space to write @buf without overwriting data > > > * userspace hasn't seen yet, we won't in fact store data before > > > * that read completes. > > > */ > > > > > > smp_mb(); /* A, matches with D */ > > > > > > write(kbuf->data + offset, buf, sz); > > > kbuf->head = head % kbuf->size; > > > > > > /* > > > * Ensure that we write all the @buf data before we update the > > > * userspace visible ubuf->head pointer. > > > */ > > > smp_wmb(); /* B, matches with C */ > > > > > > ubuf->head = kbuf->head; > > > } > > > > Now the whole crux of the question is if we need barrier A at all, since > > > the STORES issued by the @buf writes are dependent on the ubuf->tail > > > read. > > > > The dependency you are talking about is via the "if" statement? > > Even C/C++11 is not required to respect control dependencies. > > But surely we must be able to make it so; otherwise you'd never be able > to write: > > void *ptr = obj1; > > void foo(void) > { > > /* create obj2, obj3 */ > > smp_wmb(); /* ensure the objs are complete */ > > /* expose either obj2 or obj3 */ > if (x) > ptr = obj2; > else > ptr = obj3; OK, the smp_wmb() orders the creation and the exposing. But the compiler can do this: ptr = obj3; if (x) ptr = obj2; And that could momentarily expose obj3 to readers, and these readers might be fatally disappointed by the free() below. If you instead said: if (x) ACCESS_ONCE(ptr) = obj2; else ACCESS_ONCE(ptr) = obj3; then the general consensus appears to be that the compiler would not be permitted to carry out the above optimization. Since you have the smp_wmb(), readers that are properly ordered (e.g., smp_rmb() or rcu_dereference()) would be prevented from seeing pre-initialization state. > /* free the unused one */ > if (x) > free(obj3); > else > free(obj2); > } > > Earlier you said that 'volatile' or '__atomic' avoids speculative > writes; so would: > > volatile void *ptr = obj1; > > Make the compiler respect control dependencies again? If so, could we > somehow mark that !space() condition volatile? The compiler should, but the CPU is still free to ignore the control dependencies in the general case. We might be able to rely on weakly ordered hardware refraining from speculating stores, but not sure that this applies across all architectures of interest. We definitely can -not- rely on weakly ordered hardware refraining from speculating loads. > Currently the above would be considered a valid pattern. But you're > saying its not because the compiler is free to expose both obj2 and obj3 > (for however short a time) and thus the free of the 'unused' object is > incorrect and can cause use-after-free. Yes, it is definitely unsafe and invalid in absence of ACCESS_ONCE(). > In fact; how can we be sure that: > > void *ptr = NULL; > > void bar(void) > { > void *obj = malloc(...); > > /* fill obj */ > > if (!err) > rcu_assign_pointer(ptr, obj); > else > free(obj); > } > > Does not get 'optimized' into: > > void bar(void) > { > void *obj = malloc(...); > void *old_ptr = ptr; > > /* fill obj */ > > rcu_assign_pointer(ptr, obj); > if (err) { /* because runtime profile data says this is unlikely */ > ptr = old_ptr; > free(obj); > } > } In this particular case, the barrier() implied by the smp_wmb() in rcu_assign_pointer() will prevent this "optimization". However, other "optimizations" are the reason why I am working to introduce ACCESS_ONCE() into rcu_assign_pointer. > We _MUST_ be able to rely on control flow, otherwise me might as well > all go back to writing kernels in asm. It isn't -that- bad! ;-) Thanx, Paul
next prev parent reply other threads:[~2013-11-03 4:04 UTC|newest] Thread overview: 215+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-10-22 23:54 perf events ring buffer memory barrier on powerpc Michael Neuling 2013-10-23 7:39 ` Victor Kaplansky 2013-10-23 7:39 ` Victor Kaplansky 2013-10-23 14:19 ` Frederic Weisbecker 2013-10-23 14:19 ` Frederic Weisbecker 2013-10-23 14:25 ` Frederic Weisbecker 2013-10-23 14:25 ` Frederic Weisbecker 2013-10-25 17:37 ` Peter Zijlstra 2013-10-25 17:37 ` Peter Zijlstra 2013-10-25 20:31 ` Michael Neuling 2013-10-25 20:31 ` Michael Neuling 2013-10-27 9:00 ` Victor Kaplansky 2013-10-27 9:00 ` Victor Kaplansky 2013-10-28 9:22 ` Peter Zijlstra 2013-10-28 9:22 ` Peter Zijlstra 2013-10-28 10:02 ` Frederic Weisbecker 2013-10-28 10:02 ` Frederic Weisbecker 2013-10-28 12:38 ` Victor Kaplansky 2013-10-28 12:38 ` Victor Kaplansky 2013-10-28 13:26 ` Peter Zijlstra 2013-10-28 13:26 ` Peter Zijlstra 2013-10-28 16:34 ` Paul E. McKenney 2013-10-28 16:34 ` Paul E. McKenney 2013-10-28 20:17 ` Oleg Nesterov 2013-10-28 20:17 ` Oleg Nesterov 2013-10-28 20:58 ` Victor Kaplansky 2013-10-28 20:58 ` Victor Kaplansky 2013-10-29 10:21 ` Peter Zijlstra 2013-10-29 10:21 ` Peter Zijlstra 2013-10-29 10:30 ` Peter Zijlstra 2013-10-29 10:30 ` Peter Zijlstra 2013-10-29 10:35 ` Peter Zijlstra 2013-10-29 10:35 ` Peter Zijlstra 2013-10-29 20:15 ` Oleg Nesterov 2013-10-29 20:15 ` Oleg Nesterov 2013-10-29 19:27 ` Vince Weaver 2013-10-29 19:27 ` Vince Weaver 2013-10-30 10:42 ` Peter Zijlstra 2013-10-30 10:42 ` Peter Zijlstra 2013-10-30 11:48 ` James Hogan 2013-10-30 11:48 ` James Hogan 2013-10-30 12:48 ` Peter Zijlstra 2013-10-30 12:48 ` Peter Zijlstra 2013-11-06 13:19 ` [tip:perf/core] tools/perf: Add required memory barriers tip-bot for Peter Zijlstra 2013-11-06 13:50 ` Vince Weaver 2013-11-06 14:00 ` Peter Zijlstra 2013-11-06 14:28 ` Peter Zijlstra 2013-11-06 14:55 ` Vince Weaver 2013-11-06 15:10 ` Peter Zijlstra 2013-11-06 15:23 ` Peter Zijlstra 2013-11-06 14:44 ` Peter Zijlstra 2013-11-06 16:07 ` Peter Zijlstra 2013-11-06 17:31 ` Vince Weaver 2013-11-06 18:24 ` Peter Zijlstra 2013-11-07 8:21 ` Ingo Molnar 2013-11-07 14:27 ` Vince Weaver 2013-11-07 15:55 ` Ingo Molnar 2013-11-11 16:24 ` Peter Zijlstra 2013-11-11 21:10 ` Ingo Molnar 2013-10-29 21:23 ` perf events ring buffer memory barrier on powerpc Michael Neuling 2013-10-29 21:23 ` Michael Neuling 2013-10-30 9:27 ` Paul E. McKenney 2013-10-30 9:27 ` Paul E. McKenney 2013-10-30 11:25 ` Peter Zijlstra 2013-10-30 11:25 ` Peter Zijlstra 2013-10-30 14:52 ` Victor Kaplansky 2013-10-30 14:52 ` Victor Kaplansky 2013-10-30 15:39 ` Peter Zijlstra 2013-10-30 15:39 ` Peter Zijlstra 2013-10-30 17:14 ` Victor Kaplansky 2013-10-30 17:14 ` Victor Kaplansky 2013-10-30 17:44 ` Peter Zijlstra 2013-10-30 17:44 ` Peter Zijlstra 2013-10-31 6:16 ` Paul E. McKenney 2013-10-31 6:16 ` Paul E. McKenney 2013-11-01 13:12 ` Victor Kaplansky 2013-11-01 13:12 ` Victor Kaplansky 2013-11-02 16:36 ` Paul E. McKenney 2013-11-02 16:36 ` Paul E. McKenney 2013-11-02 17:26 ` Paul E. McKenney 2013-11-02 17:26 ` Paul E. McKenney 2013-10-31 6:40 ` Paul E. McKenney 2013-10-31 6:40 ` Paul E. McKenney 2013-11-01 14:25 ` Victor Kaplansky 2013-11-01 14:25 ` Victor Kaplansky 2013-11-02 17:28 ` Paul E. McKenney 2013-11-02 17:28 ` Paul E. McKenney 2013-11-01 14:56 ` Peter Zijlstra 2013-11-01 14:56 ` Peter Zijlstra 2013-11-02 17:32 ` Paul E. McKenney 2013-11-02 17:32 ` Paul E. McKenney 2013-11-03 14:40 ` Paul E. McKenney 2013-11-03 14:40 ` Paul E. McKenney 2013-11-03 15:17 ` [RFC] arch: Introduce new TSO memory barrier smp_tmb() Peter Zijlstra 2013-11-03 15:17 ` Peter Zijlstra 2013-11-03 18:08 ` Linus Torvalds 2013-11-03 18:08 ` Linus Torvalds 2013-11-03 20:01 ` Peter Zijlstra 2013-11-03 20:01 ` Peter Zijlstra 2013-11-03 22:42 ` Paul E. McKenney 2013-11-03 22:42 ` Paul E. McKenney 2013-11-03 23:34 ` Linus Torvalds 2013-11-03 23:34 ` Linus Torvalds 2013-11-04 10:51 ` Paul E. McKenney 2013-11-04 10:51 ` Paul E. McKenney 2013-11-04 11:22 ` Peter Zijlstra 2013-11-04 11:22 ` Peter Zijlstra 2013-11-04 16:27 ` Paul E. McKenney 2013-11-04 16:27 ` Paul E. McKenney 2013-11-04 16:48 ` Peter Zijlstra 2013-11-04 16:48 ` Peter Zijlstra 2013-11-04 19:11 ` Peter Zijlstra 2013-11-04 19:11 ` Peter Zijlstra 2013-11-04 19:18 ` Peter Zijlstra 2013-11-04 19:18 ` Peter Zijlstra 2013-11-04 20:54 ` Paul E. McKenney 2013-11-04 20:54 ` Paul E. McKenney 2013-11-04 20:53 ` Paul E. McKenney 2013-11-04 20:53 ` Paul E. McKenney 2013-11-05 14:05 ` Will Deacon 2013-11-05 14:05 ` Will Deacon 2013-11-05 14:49 ` Paul E. McKenney 2013-11-05 14:49 ` Paul E. McKenney 2013-11-05 18:49 ` Peter Zijlstra 2013-11-05 18:49 ` Peter Zijlstra 2013-11-06 11:00 ` Will Deacon 2013-11-06 11:00 ` Will Deacon 2013-11-06 12:39 ` Peter Zijlstra 2013-11-06 12:39 ` Peter Zijlstra 2013-11-06 12:51 ` Geert Uytterhoeven 2013-11-06 12:51 ` Geert Uytterhoeven 2013-11-06 13:57 ` Peter Zijlstra 2013-11-06 13:57 ` Peter Zijlstra 2013-11-06 18:48 ` Paul E. McKenney 2013-11-06 18:48 ` Paul E. McKenney 2013-11-06 19:42 ` Peter Zijlstra 2013-11-06 19:42 ` Peter Zijlstra 2013-11-07 11:17 ` Will Deacon 2013-11-07 11:17 ` Will Deacon 2013-11-07 13:36 ` Peter Zijlstra 2013-11-07 13:36 ` Peter Zijlstra 2013-11-07 23:50 ` Mathieu Desnoyers 2013-11-07 23:50 ` Mathieu Desnoyers 2013-11-04 11:05 ` Will Deacon 2013-11-04 11:05 ` Will Deacon 2013-11-04 16:34 ` Paul E. McKenney 2013-11-04 16:34 ` Paul E. McKenney 2013-11-03 20:59 ` Benjamin Herrenschmidt 2013-11-03 20:59 ` Benjamin Herrenschmidt 2013-11-03 22:43 ` Paul E. McKenney 2013-11-03 22:43 ` Paul E. McKenney 2013-11-03 17:07 ` perf events ring buffer memory barrier on powerpc Will Deacon 2013-11-03 22:47 ` Paul E. McKenney 2013-11-04 9:57 ` Will Deacon 2013-11-04 10:52 ` Paul E. McKenney 2013-11-01 16:11 ` Peter Zijlstra 2013-11-01 16:11 ` Peter Zijlstra 2013-11-02 17:46 ` Paul E. McKenney [this message] 2013-11-02 17:46 ` Paul E. McKenney 2013-11-01 16:18 ` Peter Zijlstra 2013-11-01 16:18 ` Peter Zijlstra 2013-11-02 17:49 ` Paul E. McKenney 2013-11-02 17:49 ` Paul E. McKenney 2013-10-30 13:28 ` Victor Kaplansky 2013-10-30 13:28 ` Victor Kaplansky 2013-10-30 15:51 ` Peter Zijlstra 2013-10-30 15:51 ` Peter Zijlstra 2013-10-30 18:29 ` Peter Zijlstra 2013-10-30 18:29 ` Peter Zijlstra 2013-10-30 19:11 ` Peter Zijlstra 2013-10-30 19:11 ` Peter Zijlstra 2013-10-31 4:33 ` Paul E. McKenney 2013-10-31 4:33 ` Paul E. McKenney 2013-10-31 4:32 ` Paul E. McKenney 2013-10-31 4:32 ` Paul E. McKenney 2013-10-31 9:04 ` Peter Zijlstra 2013-10-31 9:04 ` Peter Zijlstra 2013-10-31 15:07 ` Paul E. McKenney 2013-10-31 15:07 ` Paul E. McKenney 2013-10-31 15:19 ` Peter Zijlstra 2013-10-31 15:19 ` Peter Zijlstra 2013-11-01 9:28 ` Paul E. McKenney 2013-11-01 9:28 ` Paul E. McKenney 2013-11-01 10:30 ` Peter Zijlstra 2013-11-01 10:30 ` Peter Zijlstra 2013-11-02 15:20 ` Paul E. McKenney 2013-11-02 15:20 ` Paul E. McKenney 2013-11-04 9:07 ` Peter Zijlstra 2013-11-04 9:07 ` Peter Zijlstra 2013-11-04 10:00 ` Paul E. McKenney 2013-11-04 10:00 ` Paul E. McKenney 2013-10-31 9:59 ` Victor Kaplansky 2013-10-31 9:59 ` Victor Kaplansky 2013-10-31 12:28 ` David Laight 2013-10-31 12:28 ` David Laight 2013-10-31 12:55 ` Victor Kaplansky 2013-10-31 12:55 ` Victor Kaplansky 2013-10-31 15:25 ` Paul E. McKenney 2013-10-31 15:25 ` Paul E. McKenney 2013-11-01 16:06 ` Victor Kaplansky 2013-11-01 16:06 ` Victor Kaplansky 2013-11-01 16:25 ` David Laight 2013-11-01 16:25 ` David Laight 2013-11-01 16:30 ` Victor Kaplansky 2013-11-01 16:30 ` Victor Kaplansky 2013-11-03 20:57 ` Benjamin Herrenschmidt 2013-11-03 20:57 ` Benjamin Herrenschmidt 2013-11-02 15:46 ` Paul E. McKenney 2013-11-02 15:46 ` Paul E. McKenney 2013-10-28 19:09 ` Oleg Nesterov 2013-10-28 19:09 ` Oleg Nesterov 2013-10-29 14:06 ` [tip:perf/urgent] perf: Fix perf ring buffer memory ordering tip-bot for Peter Zijlstra 2014-05-08 20:46 perf events ring buffer memory barrier on powerpc Mikulas Patocka [not found] ` <OF667059AA.7F151BCC-ONC2257CD3.0036CFEB-C2257CD3.003BBF01@il.ibm.com> 2014-05-09 12:20 ` Mikulas Patocka 2014-05-09 13:47 ` Paul E. McKenney
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20131102174645.GC3947@linux.vnet.ibm.com \ --to=paulmck@linux.vnet.ibm.com \ --cc=VICTORK@il.ibm.com \ --cc=anton@samba.org \ --cc=benh@kernel.crashing.org \ --cc=fweisbec@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@ozlabs.org \ --cc=mathieu.desnoyers@polymtl.ca \ --cc=michael@ellerman.id.au \ --cc=mikey@neuling.org \ --cc=oleg@redhat.com \ --cc=peterz@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.