On Mon, Nov 04, 2013 at 10:07:44AM +0100, Peter Zijlstra wrote:
> On Sat, Nov 02, 2013 at 08:20:48AM -0700, Paul E. McKenney wrote:
> > On Fri, Nov 01, 2013 at 11:30:17AM +0100, Peter Zijlstra wrote:
> > > Furthermore there's a gazillion parallel userspace programs.
> >
> > Most of which have very
On Sat, Nov 02, 2013 at 08:20:48AM -0700, Paul E. McKenney wrote:
> On Fri, Nov 01, 2013 at 11:30:17AM +0100, Peter Zijlstra wrote:
> > Furthermore there's a gazillion parallel userspace programs.
>
> Most of which have very unaggressive concurrency designs.
pthread_mutex_t A, B;
char data_A[x];
On Fri, 2013-11-01 at 18:30 +0200, Victor Kaplansky wrote:
> "David Laight" wrote on 11/01/2013 06:25:29 PM:
> > gcc will do unexpected memory accesses for bit fields that are
> > adjacent to volatile data.
> > In particular it may generate 64bit sized (and aligned) RMW cycles
> > when accessing b
On Sat, Nov 02, 2013 at 10:32:39AM -0700, Paul E. McKenney wrote:
> On Fri, Nov 01, 2013 at 03:56:34PM +0100, Peter Zijlstra wrote:
> > On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote:
> > > > Now the whole crux of the question is if we need barrier A at all, since
> > > > the STOR
On Fri, Nov 01, 2013 at 03:56:34PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote:
> > > 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.
>
On Fri, Nov 01, 2013 at 05:18:19PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote:
> > The dependency you are talking about is via the "if" statement?
> > Even C/C++11 is not required to respect control dependencies.
> >
> > This one is a bit annoyi
[ Adding David Howells, Lech Fomicki, and Mark Batty on CC for their
thoughts given previous discussions. ]
On Sat, Nov 02, 2013 at 09:36:18AM -0700, Paul E. McKenney wrote:
> On Fri, Nov 01, 2013 at 03:12:58PM +0200, Victor Kaplansky wrote:
> > "Paul E. McKenney" wrote on 10/31/2013
> > 08:16:
On Fri, Nov 01, 2013 at 06:06:58PM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" wrote on 10/31/2013
> 05:25:43 PM:
>
> > I really don't care about "fair" -- I care instead about the kernel
> > working reliably.
>
> Though I don't see how putting a memory barrier without deep understanding
On Fri, Nov 01, 2013 at 11:30:17AM +0100, Peter Zijlstra wrote:
> On Fri, Nov 01, 2013 at 02:28:14AM -0700, Paul E. McKenney wrote:
> > > This is a completely untenable position.
> >
> > Indeed it is!
> >
> > C/C++ never was intended to be used for parallel programming,
>
> And yet pretty much
On Fri, Nov 01, 2013 at 03:12:58PM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" wrote on 10/31/2013
> 08:16:02 AM:
>
> > > BTW, it is why you also don't need ACCESS_ONCE() around @tail, but only
> > > around
> > > @head read.
>
> Just to be sure, that we are talking about the same code -
On Fri, Nov 01, 2013 at 04:25:42PM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" wrote on 10/31/2013
> 08:40:15 AM:
>
> > > void ubuf_read(void)
> > > {
> > >u64 head, tail;
> > >
> > >tail = ACCESS_ONCE(ubuf->tail);
> > >head = ACCESS_ONCE(ubuf->head);
> > >
> > >/*
> > >
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
"David Laight" wrote on 11/01/2013 06:25:29 PM:
> gcc will do unexpected memory accesses for bit fields that are
> adjacent to volatile data.
> In particular it may generate 64bit sized (and aligned) RMW cycles
> when accessing bit fields.
> And yes, this has caused real problems.
Thanks, I am aw
> But "broken" compiler is much wider issue to be deeply discussed in this
> thread. I'm pretty sure that kernel have tons of very subtle
> code that actually creates locks and memory ordering. Such code
> usually just use the "barrier()" approach to tell gcc not to combine
> or move memory access
On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote:
> The dependency you are talking about is via the "if" statement?
> Even C/C++11 is not required to respect control dependencies.
>
> This one is a bit annoying. The x86 TSO means that you really only
> need barrier(), ARM (recent
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;
>
"Paul E. McKenney" wrote on 10/31/2013
05:25:43 PM:
> I really don't care about "fair" -- I care instead about the kernel
> working reliably.
Though I don't see how putting a memory barrier without deep understanding
why it is needed helps kernel reliability, I do agree that reliability
is more
On Wed, Oct 30, 2013 at 11:40:15PM -0700, Paul E. McKenney wrote:
> > 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?
> Ev
"Paul E. McKenney" wrote on 10/31/2013
08:40:15 AM:
> > void ubuf_read(void)
> > {
> >u64 head, tail;
> >
> >tail = ACCESS_ONCE(ubuf->tail);
> >head = ACCESS_ONCE(ubuf->head);
> >
> >/*
> > * Ensure we read the buffer boundaries before the actual buffer
> > * data...
> >
"Paul E. McKenney" wrote on 10/31/2013
08:16:02 AM:
> > BTW, it is why you also don't need ACCESS_ONCE() around @tail, but only
> > around
> > @head read.
Just to be sure, that we are talking about the same code - I was
considering
ACCESS_ONCE() around @tail in point AAA in the following example
On Wed, Oct 30, 2013 at 04:52:05PM +0200, Victor Kaplansky wrote:
> Peter Zijlstra wrote on 10/30/2013 01:25:26 PM:
>
> > Also, I'm not entirely sure on C, that too seems like a dependency, we
> > simply cannot read the buffer @tail before we've read the tail itself,
> > now can we? Similarly we
On Wed, Oct 30, 2013 at 12:25:26PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 02:27:25AM -0700, Paul E. McKenney wrote:
> > On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> > > Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
> > >
> > > > mb(); // : d
On Thu, Oct 31, 2013 at 11:59:21AM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" wrote on 10/31/2013
> 06:32:58 AM:
>
> > If you want to play the "omit memory barriers" game, then proving a
> > negative is in fact the task before you.
>
> Generally it is not fair. Otherwise, anyone could p
On Fri, Nov 01, 2013 at 02:28:14AM -0700, Paul E. McKenney wrote:
> > This is a completely untenable position.
>
> Indeed it is!
>
> C/C++ never was intended to be used for parallel programming,
And yet pretty much all kernels ever written for SMP systems are written
in it; what drugs are those
On Thu, Oct 31, 2013 at 04:19:55PM +0100, Peter Zijlstra wrote:
> On Thu, Oct 31, 2013 at 08:07:56AM -0700, Paul E. McKenney wrote:
> > On Thu, Oct 31, 2013 at 10:04:57AM +0100, Peter Zijlstra wrote:
> > > On Wed, Oct 30, 2013 at 09:32:58PM -0700, Paul E. McKenney wrote:
> > > > Before C/C++11, the
On Thu, Oct 31, 2013 at 08:07:56AM -0700, Paul E. McKenney wrote:
> On Thu, Oct 31, 2013 at 10:04:57AM +0100, Peter Zijlstra wrote:
> > On Wed, Oct 30, 2013 at 09:32:58PM -0700, Paul E. McKenney wrote:
> > > Before C/C++11, the closest thing to such a prohibition is use of
> > > volatile, for examp
On Thu, Oct 31, 2013 at 10:04:57AM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 09:32:58PM -0700, Paul E. McKenney wrote:
> > 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 an
"David Laight" wrote on 10/31/2013 02:28:56 PM:
> So even though the wmb() in the writer ensures the writes are correctly
> ordered, the reader can read the old value from the second location from
> its local cache.
In case of circular buffer, the only thing that producer reads is @tail,
and not
> "For instance, your producer must issue a "memory barrier" instruction
> after writing the data to shared memory and before inserting it on
> the queue; likewise, your consumer must issue a memory barrier
> instruction after removing an item from the queue and before reading
> from its me
"Paul E. McKenney" wrote on 10/31/2013
06:32:58 AM:
> If you want to play the "omit memory barriers" game, then proving a
> negative is in fact the task before you.
Generally it is not fair. Otherwise, anyone could put an smp_mb() at a
random place, and expect others to "prove" that it is not ne
On Wed, Oct 30, 2013 at 09:32:58PM -0700, Paul E. McKenney wrote:
> 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
On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" 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 ord
On Wed, Oct 30, 2013 at 04:51:16PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote:
> > one of the authors of Documentation/memory-barriers.txt is on cc: list ;-)
> >
> > Disclaimer: it is anyway impossible to prove lack of *any* problem.
> >
> > Ha
On Wed, Oct 30, 2013 at 07:29:30PM +0100, Peter Zijlstra wrote:
> + page_shift = PAGE_SHIFT + page_order(rb);
> +
> + handle->page = (offset >> page_shift) & (rb->nr_pages - 1);
> +
> + offset &= page_shift - 1;
offset &= (1UL << page_shift) - 1;
Weird that it even appeared to work..
On Wed, Oct 30, 2013 at 04:51:16PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote:
> > one of the authors of Documentation/memory-barriers.txt is on cc: list ;-)
> >
> > Disclaimer: it is anyway impossible to prove lack of *any* problem.
> >
> > Ha
On Wed, Oct 30, 2013 at 07:14:29PM +0200, Victor Kaplansky wrote:
> We need a complete rmb() here IMO. I think there is a fundamental
> difference between load and stores in this aspect. Load are allowed to
> be hoisted by compiler or executed speculatively by HW. To prevent
> load "*(ubuf->data +
Peter Zijlstra wrote on 10/30/2013 05:39:31 PM:
> Although I suppose speculative reads are allowed -- they don't have the
> destructive behaviour speculative writes have -- and thus we could in
> fact get reorder issues.
I agree.
>
> But since it is still a dependent load in that we do that @ta
On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote:
> one of the authors of Documentation/memory-barriers.txt is on cc: list ;-)
>
> Disclaimer: it is anyway impossible to prove lack of *any* problem.
>
> Having said that, lets look into an example in
> Documentation/circular-buffer
On Wed, Oct 30, 2013 at 04:52:05PM +0200, Victor Kaplansky wrote:
> Peter Zijlstra wrote on 10/30/2013 01:25:26 PM:
>
> > Also, I'm not entirely sure on C, that too seems like a dependency, we
> > simply cannot read the buffer @tail before we've read the tail itself,
> > now can we? Similarly we
Peter Zijlstra wrote on 10/30/2013 01:25:26 PM:
> Also, I'm not entirely sure on C, that too seems like a dependency, we
> simply cannot read the buffer @tail before we've read the tail itself,
> now can we? Similarly we cannot compare tail to head without having the
> head read completed.
No, t
"Paul E. McKenney" 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.
>
>
On Wed, Oct 30, 2013 at 11:48:44AM +, James Hogan wrote:
> Hi Peter,
>
> On 30/10/13 10:42, Peter Zijlstra wrote:
> > Subject: perf, tool: Add required memory barriers
> >
> > To match patch bf378d341e48 ("perf: Fix perf ring buffer memory
> > ordering") change userspace to also adhere to the
Hi Peter,
On 30/10/13 10:42, Peter Zijlstra wrote:
> Subject: perf, tool: Add required memory barriers
>
> To match patch bf378d341e48 ("perf: Fix perf ring buffer memory
> ordering") change userspace to also adhere to the ordering outlined.
>
> Most barrier implementations were gleaned from
> a
On Wed, Oct 30, 2013 at 02:27:25AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> > Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
> >
> > > mb(); // : do we really need it? I think yes.
> >
> > Oh, it is hard to argue with fe
On Tue, Oct 29, 2013 at 03:27:05PM -0400, Vince Weaver wrote:
> On Tue, 29 Oct 2013, Peter Zijlstra wrote:
>
> > On Tue, Oct 29, 2013 at 11:21:31AM +0100, Peter Zijlstra wrote:
> > --- linux-2.6.orig/include/uapi/linux/perf_event.h
> > +++ linux-2.6/include/uapi/linux/perf_event.h
> > @@ -479,13 +
On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
>
> > mb(); // : do we really need it? I think yes.
>
> Oh, it is hard to argue with feelings. Also, it is easy to be on
> conservative side and put the barrier here
Peter Zijlstra wrote:
> On Tue, Oct 29, 2013 at 11:21:31AM +0100, Peter Zijlstra wrote:
> > On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> > > Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
> > >
> > > > mb(); // : do we really need it? I think yes.
> > >
>
On 10/29, Peter Zijlstra wrote:
>
> On Tue, Oct 29, 2013 at 11:30:57AM +0100, Peter Zijlstra wrote:
> > @@ -154,9 +175,11 @@ int perf_output_begin(struct perf_output
> > * Userspace could choose to issue a mb() before updating the
> > * tail pointer. So that all reads will
On Tue, 29 Oct 2013, Peter Zijlstra wrote:
> On Tue, Oct 29, 2013 at 11:21:31AM +0100, Peter Zijlstra wrote:
> --- linux-2.6.orig/include/uapi/linux/perf_event.h
> +++ linux-2.6/include/uapi/linux/perf_event.h
> @@ -479,13 +479,15 @@ struct perf_event_mmap_page {
> /*
>* Control data
On Tue, Oct 29, 2013 at 11:30:57AM +0100, Peter Zijlstra wrote:
> @@ -154,9 +175,11 @@ int perf_output_begin(struct perf_output
>* Userspace could choose to issue a mb() before updating the
>* tail pointer. So that all reads will be completed before the
>
On Tue, Oct 29, 2013 at 11:21:31AM +0100, Peter Zijlstra wrote:
> On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> > Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
> >
> > > mb(); // : do we really need it? I think yes.
> >
> > Oh, it is hard to argue with feel
On Mon, Oct 28, 2013 at 10:58:58PM +0200, Victor Kaplansky wrote:
> Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
>
> > mb(); // : do we really need it? I think yes.
>
> Oh, it is hard to argue with feelings. Also, it is easy to be on
> conservative side and put the barrier here
Oleg Nesterov wrote on 10/28/2013 10:17:35 PM:
> mb(); // : do we really need it? I think yes.
Oh, it is hard to argue with feelings. Also, it is easy to be on
conservative side and put the barrier here just in case.
But I still insist that the barrier is redundant in your exampl
On 10/28, Paul E. McKenney wrote:
>
> On Mon, Oct 28, 2013 at 02:26:34PM +0100, Peter Zijlstra wrote:
> > --- linux-2.6.orig/kernel/events/ring_buffer.c
> > +++ linux-2.6/kernel/events/ring_buffer.c
> > @@ -87,10 +87,31 @@ static void perf_output_put_handle(struc
> > goto out;
> >
> >
On 10/28, Peter Zijlstra wrote:
>
> Lets add Paul and Oleg to the thread; this is getting far more 'fun'
> that it should be ;-)
Heh. All I can say is that I would like to see the authoritative answer,
you know who can shed a light ;)
But to avoid the confusion, wmb() added by this patch looks "o
On Mon, Oct 28, 2013 at 02:26:34PM +0100, Peter Zijlstra wrote:
> On Mon, Oct 28, 2013 at 02:38:29PM +0200, Victor Kaplansky wrote:
> > > 2013/10/25 Peter Zijlstra :
> > > > On Wed, Oct 23, 2013 at 03:19:51PM +0100, Frederic Weisbecker wrote:
> > > > I would argue for
> > > >
> > > > READ ->data_
On Mon, Oct 28, 2013 at 02:38:29PM +0200, Victor Kaplansky wrote:
> > 2013/10/25 Peter Zijlstra :
> > > On Wed, Oct 23, 2013 at 03:19:51PM +0100, Frederic Weisbecker wrote:
> > > I would argue for
> > >
> > > READ ->data_tail READ ->data_head
> > > smp_rmb() (A)
> From: Frederic Weisbecker
>
> 2013/10/25 Peter Zijlstra :
> > On Wed, Oct 23, 2013 at 03:19:51PM +0100, Frederic Weisbecker wrote:
> > I would argue for
> >
> > READ ->data_tail READ ->data_head
> > smp_rmb() (A) smp_rmb() (C)
> > WRITE $d
2013/10/25 Peter Zijlstra :
> On Wed, Oct 23, 2013 at 03:19:51PM +0100, Frederic Weisbecker wrote:
> I would argue for:
>
> READ ->data_tail READ ->data_head
> smp_rmb() (A) smp_rmb() (C)
> WRITE $data READ $data
>
On Sun, Oct 27, 2013 at 11:00:33AM +0200, Victor Kaplansky wrote:
> Peter Zijlstra wrote on 10/25/2013 07:37:49 PM:
>
> > I would argue for:
> >
> > READ ->data_tail READ ->data_head
> > smp_rmb() (A) smp_rmb() (C)
> > WRITE $data READ $data
> > smp_w
Peter Zijlstra wrote on 10/25/2013 07:37:49 PM:
> I would argue for:
>
> READ ->data_tail READ ->data_head
> smp_rmb() (A) smp_rmb() (C)
> WRITE $data READ $data
> smp_wmb() (B) smp_mb() (D)
> STORE ->data_headWRITE ->data_tail
> I would argue for:
>
> READ ->data_tailREAD ->data_head
> smp_rmb() (A) smp_rmb() (C)
> WRITE $data READ $data
> smp_wmb() (B) smp_mb()(D)
> STORE ->data_head WRITE -
On Wed, Oct 23, 2013 at 03:19:51PM +0100, Frederic Weisbecker wrote:
> On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
> > Frederic,
> >
> > The comment says atomic_dec_and_test() but the code is
> > local_dec_and_test().
> >
> > On powerpc, local_dec_and_test() doesn't have a me
2013/10/23 Frederic Weisbecker :
> On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
>> Frederic,
>>
>> In the perf ring buffer code we have this in perf_output_get_handle():
>>
>> if (!local_dec_and_test(&rb->nest))
>> goto out;
>>
>> /*
>>* Publish
On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
> Frederic,
>
> In the perf ring buffer code we have this in perf_output_get_handle():
>
> if (!local_dec_and_test(&rb->nest))
> goto out;
>
> /*
>* Publish the known good head. Rely on the full ba
See below.
Michael Neuling wrote on 10/23/2013 02:54:54 AM:
>
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index cd55144..95768c6 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -87,10 +87,10 @@ again:
>goto out;
>
> /
Frederic,
In the perf ring buffer code we have this in perf_output_get_handle():
if (!local_dec_and_test(&rb->nest))
goto out;
/*
* Publish the known good head. Rely on the full barrier implied
* by atomic_dec_and_test() order the rb->head read a
67 matches
Mail list logo
