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

On 10/17/2018 05:50 PM, Peter Zijlstra wrote:

On Wed, Oct 17, 2018 at 04:41:55PM +0200, Daniel Borkmann wrote:

quoted

@@ -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);