lkml.org 
[lkml]   [2013]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: perf events ring buffer memory barrier on powerpc
From
2013/10/23 Frederic Weisbecker <fweisbec@gmail.com>:
> 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 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)
>>
>> Mikey
>>
>> 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;
>>
>> /*
>> - * 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
^^ I meant a write above for data_head.


\
 
 \ /
  Last update: 2013-10-23 16:41    [W:1.055 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site