From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> To: Victor Kaplansky <VICTORK@il.ibm.com> Cc: 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>, Oleg Nesterov <oleg@redhat.com>, Peter Zijlstra <peterz@infradead.org> Subject: Re: perf events ring buffer memory barrier on powerpc Date: Wed, 30 Oct 2013 21:32:58 -0700 [thread overview] Message-ID: <20131031043258.GQ4126@linux.vnet.ibm.com> (raw) In-Reply-To: <OFC6B7100E.31C6B190-ON42257C14.004841C6-42257C14.004A0EA6@il.ibm.com> On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote: > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote on 10/30/2013 > 11:27:25 AM: > > > If you were to back up that insistence with a description of the > orderings > > you are relying on, why other orderings are not important, and how the > > important orderings are enforced, I might be tempted to pay attention > > to your opinion. > > > > Thanx, Paul > > NP, though, I feel too embarrassed to explain things about memory barriers > when > one of the authors of Documentation/memory-barriers.txt is on cc: list ;-) > > Disclaimer: it is anyway impossible to prove lack of *any* problem. If you want to play the "omit memory barriers" game, then proving a negative is in fact the task before you. > Having said that, lets look into an example in > Documentation/circular-buffers.txt: And the correctness of this code has been called into question. :-( An embarrassingly long time ago -- I need to get this either proven or fixed. > > THE PRODUCER > > ------------ > > > > The producer will look something like this: > > > > spin_lock(&producer_lock); > > > > unsigned long head = buffer->head; > > unsigned long tail = ACCESS_ONCE(buffer->tail); > > > > if (CIRC_SPACE(head, tail, buffer->size) >= 1) { > > /* insert one item into the buffer */ > > struct item *item = buffer[head]; > > > > produce_item(item); > > > > smp_wmb(); /* commit the item before incrementing the head > */ > > > > buffer->head = (head + 1) & (buffer->size - 1); > > > > /* wake_up() will make sure that the head is committed > before > > * waking anyone up */ > > wake_up(consumer); > > } > > > > spin_unlock(&producer_lock); > > We can see that authors of the document didn't put any memory barrier > after "buffer->tail" read and before "produce_item(item)" and I think they > have > a good reason. > > Lets consider an imaginary smp_mb() right before "produce_item(item);". > Such a barrier will ensure that - > > - the memory read on "buffer->tail" is completed > before store to memory pointed by "item" is committed. > > However, the store to "buffer->tail" anyway cannot be completed before > conditional > branch implied by "if ()" is proven to execute body statement of the if(). > And the > latter cannot be proven before read of "buffer->tail" is completed. > > Lets see this other way. Lets imagine that somehow a store to the data > pointed by "item" > is completed before we read "buffer->tail". That would mean, that the store > was completed > speculatively. But speculative execution of conditional stores is > prohibited by C/C++ standard, > otherwise any conditional store at any random place of code could pollute > shared memory. Before C/C++11, the closest thing to such a prohibition is use of volatile, for example, ACCESS_ONCE(). Even in C/C++11, you have to use atomics to get anything resembing this prohibition. If you just use normal variables, the compiler is within its rights to transform something like the following: if (a) b = 1; else b = 42; Into: b = 42; if (a) b = 1; Many other similar transformations are permitted. Some are used to all vector instructions to be used -- the compiler can do a write with an overly wide vector instruction, then clean up the clobbered variables later, if it wishes. Again, if the variables are not marked volatile, or, in C/C++11, atomic. > On the other hand, if compiler or processor can prove that condition in > above if() is going > to be true (or if speculative store writes the same value as it was before > write), the > speculative store *is* allowed. In this case we should not be bothered by > the fact that > memory pointed by "item" is written before a read from "buffer->tail" is > completed. The compilers don't always know as much as they might about the underlying hardware's memory model. Of course, if this code is architecture specific, it can avoid DEC Alpha's fun and games, which could also violate your assumptions in the above paragraph: http://www.openvms.compaq.com/wizard/wiz_2637.html Anyway, proving or fixing the code in Documentation/circular-buffers.txt has been on my list for too long, so I will take a closer look at it. Thanx, Paul
WARNING: multiple messages have this Message-ID (diff)
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> To: Victor Kaplansky <VICTORK@il.ibm.com> Cc: Michael Neuling <mikey@neuling.org>, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>, Peter Zijlstra <peterz@infradead.org>, LKML <linux-kernel@vger.kernel.org>, Oleg Nesterov <oleg@redhat.com>, Linux PPC dev <linuxppc-dev@ozlabs.org>, Anton Blanchard <anton@samba.org>, Frederic Weisbecker <fweisbec@gmail.com> Subject: Re: perf events ring buffer memory barrier on powerpc Date: Wed, 30 Oct 2013 21:32:58 -0700 [thread overview] Message-ID: <20131031043258.GQ4126@linux.vnet.ibm.com> (raw) In-Reply-To: <OFC6B7100E.31C6B190-ON42257C14.004841C6-42257C14.004A0EA6@il.ibm.com> On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote: > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote on 10/30/2013 > 11:27:25 AM: > > > If you were to back up that insistence with a description of the > orderings > > you are relying on, why other orderings are not important, and how the > > important orderings are enforced, I might be tempted to pay attention > > to your opinion. > > > > Thanx, Paul > > NP, though, I feel too embarrassed to explain things about memory barriers > when > one of the authors of Documentation/memory-barriers.txt is on cc: list ;-) > > Disclaimer: it is anyway impossible to prove lack of *any* problem. If you want to play the "omit memory barriers" game, then proving a negative is in fact the task before you. > Having said that, lets look into an example in > Documentation/circular-buffers.txt: And the correctness of this code has been called into question. :-( An embarrassingly long time ago -- I need to get this either proven or fixed. > > THE PRODUCER > > ------------ > > > > The producer will look something like this: > > > > spin_lock(&producer_lock); > > > > unsigned long head = buffer->head; > > unsigned long tail = ACCESS_ONCE(buffer->tail); > > > > if (CIRC_SPACE(head, tail, buffer->size) >= 1) { > > /* insert one item into the buffer */ > > struct item *item = buffer[head]; > > > > produce_item(item); > > > > smp_wmb(); /* commit the item before incrementing the head > */ > > > > buffer->head = (head + 1) & (buffer->size - 1); > > > > /* wake_up() will make sure that the head is committed > before > > * waking anyone up */ > > wake_up(consumer); > > } > > > > spin_unlock(&producer_lock); > > We can see that authors of the document didn't put any memory barrier > after "buffer->tail" read and before "produce_item(item)" and I think they > have > a good reason. > > Lets consider an imaginary smp_mb() right before "produce_item(item);". > Such a barrier will ensure that - > > - the memory read on "buffer->tail" is completed > before store to memory pointed by "item" is committed. > > However, the store to "buffer->tail" anyway cannot be completed before > conditional > branch implied by "if ()" is proven to execute body statement of the if(). > And the > latter cannot be proven before read of "buffer->tail" is completed. > > Lets see this other way. Lets imagine that somehow a store to the data > pointed by "item" > is completed before we read "buffer->tail". That would mean, that the store > was completed > speculatively. But speculative execution of conditional stores is > prohibited by C/C++ standard, > otherwise any conditional store at any random place of code could pollute > shared memory. Before C/C++11, the closest thing to such a prohibition is use of volatile, for example, ACCESS_ONCE(). Even in C/C++11, you have to use atomics to get anything resembing this prohibition. If you just use normal variables, the compiler is within its rights to transform something like the following: if (a) b = 1; else b = 42; Into: b = 42; if (a) b = 1; Many other similar transformations are permitted. Some are used to all vector instructions to be used -- the compiler can do a write with an overly wide vector instruction, then clean up the clobbered variables later, if it wishes. Again, if the variables are not marked volatile, or, in C/C++11, atomic. > On the other hand, if compiler or processor can prove that condition in > above if() is going > to be true (or if speculative store writes the same value as it was before > write), the > speculative store *is* allowed. In this case we should not be bothered by > the fact that > memory pointed by "item" is written before a read from "buffer->tail" is > completed. The compilers don't always know as much as they might about the underlying hardware's memory model. Of course, if this code is architecture specific, it can avoid DEC Alpha's fun and games, which could also violate your assumptions in the above paragraph: http://www.openvms.compaq.com/wizard/wiz_2637.html Anyway, proving or fixing the code in Documentation/circular-buffers.txt has been on my list for too long, so I will take a closer look at it. Thanx, Paul
next prev parent reply other threads:[~2013-10-31 4:35 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 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 [this message] 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=20131031043258.GQ4126@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.