Re: perf events ring buffer memory barrier on powerpc
From: Frederic Weisbecker <hidden>
Date: 2013-10-23 14:19:55
Also in:
lkml
On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
quoted hunk ↗ jump to hunk
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 and this * write. */ rb->user_page->data_head = head; 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 memory barrier but atomic_dec_and_test() does. Is the comment wrong, or is local_dec_and_test() suppose to imply a memory barrier too and we have it wrongly implemented in powerpc? My guess is that local_dec_and_test() is correct but we to add an explicit memory barrier like below: (Kudos to Victor Kaplansky for finding this) Mikeydiff --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; /* - * Publish the known good head. Rely on the full barrier implied - * by atomic_dec_and_test() order the rb->head read and this - * write. + * Publish the known good head. We need a memory barrier to order the + * order the rb->head read and this write. */ + smp_mb (); rb->user_page->data_head = head; /*
I'm adding Peter in Cc since he wrote that code.
I agree that local_dec_and_test() doesn't need to imply an smp barrier.
All it has to provide as a guarantee is the atomicity against local concurrent
operations (interrupts, preemption, ...).
Now I'm a bit confused about this barrier.
I think we want this ordering:
Kernel User
READ rb->user_page->data_tail READ rb->user_page->data_head
smp_mb() smp_mb()
WRITE rb data READ rb data
smp_mb() smp_mb()
rb->user_page->data_head WRITE rb->user_page->data_tail
So yeah we want a berrier between the data published and the user data_head.
But this ordering concerns wider layout than just rb->head and rb->user_page->data_head
And BTW I can see an smp_rmb() after we read rb->user_page->data_tail. This is probably the
first kernel barrier in my above example. (not sure if rmb() alone is enough though).