Re: [PATCH bpf-next 2/3] tools, perf: use smp_{rmb,mb} barriers instead of {rmb,mb}

[Daniel Borkmann]

On 10/17/2018 05:50 PM, Peter Zijlstra wrote:
> On Wed, Oct 17, 2018 at 04:41:55PM +0200, Daniel Borkmann wrote:
>> @@ -73,7 +73,8 @@ static inline u64 perf_mmap__read_head(struct perf_mmap 
>> *mm)
>>  {
>>      struct perf_event_mmap_page *pc = mm->base;
>>      u64 head = READ_ONCE(pc->data_head);
>> -    rmb();
>> +
>> +    smp_rmb();
>>      return head;
>>  }
>>  
>> @@ -84,7 +85,7 @@ static inline void perf_mmap__write_tail(struct perf_mmap 
>> *md, u64 tail)
>>      /*
>>       * ensure all reads are done before we write the tail out.
>>       */
>> -    mb();
>> +    smp_mb();
>>      pc->data_tail = tail;
> 
> Ideally that would be a WRITE_ONCE() to avoid store tearing.

Right, agree.

> Alternatively, I think we can use smp_store_release() here, all we care
> about is that the prior loads stay prior.
> 
> Similarly, I suppose, we could use smp_load_acquire() for the data_head
> load above.

Wouldn't this then also allow the kernel side to use smp_store_release()
when it updates the head? We'd be pretty much at the model as described
in Documentation/core-api/circular-buffers.rst.

Meaning, rough pseudo-code diff would look as:

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 5d3cf40..3d96275 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -84,8 +84,9 @@ static void perf_output_put_handle(struct perf_output_handle 
*handle)
         *
         * See perf_output_begin().
         */
-       smp_wmb(); /* B, matches C */
-       rb->user_page->data_head = head;
+
+       /* B, matches C */
+       smp_store_release(&rb->user_page->data_head, head);

        /*
         * Now check if we missed an update -- rely on previous implied

Plus, user space side of perf (assuming we have the barriers imported):

diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
index 05a6d47..66e1304 100644
--- a/tools/perf/util/mmap.h
+++ b/tools/perf/util/mmap.h
@@ -72,20 +72,15 @@ void perf_mmap__consume(struct perf_mmap *map);
 static inline u64 perf_mmap__read_head(struct perf_mmap *mm)
 {
        struct perf_event_mmap_page *pc = mm->base;
-       u64 head = READ_ONCE(pc->data_head);
-       rmb();
-       return head;
+
+       return smp_load_acquire(&pc->data_head);
 }

 static inline void perf_mmap__write_tail(struct perf_mmap *md, u64 tail)
 {
        struct perf_event_mmap_page *pc = md->base;

-       /*
-        * ensure all reads are done before we write the tail out.
-        */
-       mb();
-       pc->data_tail = tail;
+       smp_store_release(&pc->data_tail, tail);
 }

 union perf_event *perf_mmap__read_forward(struct perf_mmap *map);