perf events ring buffer memory barrier on powerpc

kernel/events/ring_buffer.c +0 -0

index cd55144..95768c6 100644
@@ -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 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

kernel/events/ring_buffer.c +0 -0

index cd55144..95768c6 100644
@@ -87,10 +87,10 @@ again:
 		goto out;
 
 	/*

Quoted 37 lines

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

kernel/events/ring_buffer.c +0 -0

index cd55144..95768c6 100644
@@ -87,10 +87,10 @@ again:
              goto out;
 
      /*

kernel/events/ring_buffer.c again: · line 96

Quoted 10 lines

kernel/events/ring_buffer.c +3 -3

-      * 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

Quoted 9 lines · Michael Neuling

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 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?

Quoted 17 lines

kernel/events/ring_buffer.c +3 -3

index cd55144..95768c6 100644
@@ -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;
 

kernel/events/ring_buffer.c again: · line 96

Quoted 1 line

kernel/events/ring_buffer.c +0 -0

 	/*

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 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_head WRITE ->data_tail

Where A pairs with D, and B pairs with C.

I don't think A needs to be a full barrier because we won't in fact write data until we see the store from userspace. So we simply don't issue the data WRITE until we observe it.

OTOH, D needs to be a full barrier since it separates the data READ from the tail WRITE.

For B a WMB is sufficient since it separates two WRITEs, and for C an RMB is sufficient since it separates two READs.

---
 kernel/events/ring_buffer.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

kernel/events/ring_buffer.c +26 -3

index cd55144270b5..c91274ef4e23 100644
@@ -87,10 +87,31 @@ static void perf_output_put_handle(struct perf_output_handle *handle)
 		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.
+	 * Since the mmap() consumer (userspace) can run on a different CPU:
+	 *
+	 *   kernel				user
+	 *
+	 *   READ ->data_tail			READ ->data_head
+	 *   smp_rmb()	(A)			smp_rmb()	(C)
+	 *   WRITE $data			READ $data
+	 *   smp_wmb()	(B)			smp_mb()	(D)
+	 *   STORE ->data_head			WRITE ->data_tail
+	 * 
+	 * Where A pairs with D, and B pairs with C.
+	 * 
+	 * I don't think A needs to be a full barrier because we won't in fact
+	 * write data until we see the store from userspace. So we simply don't
+	 * issue the data WRITE until we observe it.
+	 * 
+	 * OTOH, D needs to be a full barrier since it separates the data READ
+	 * from the tail WRITE.
+	 * 
+	 * For B a WMB is sufficient since it separates two WRITEs, and for C
+	 * an RMB is sufficient since it separates two READs.
+	 *
+	 * See perf_output_begin().
 	 */
+	smp_wmb();
 	rb->user_page->data_head = head;
 
 	/*

@@ -154,6 +175,8 @@ int perf_output_begin(struct perf_output_handle *handle,
 		 * Userspace could choose to issue a mb() before updating the
 		 * tail pointer. So that all reads will be completed before the
 		 * write is issued.
+		 *
+		 * See perf_output_put_handle().
 		 */
 		tail = ACCESS_ONCE(rb->user_page->data_tail);
 		smp_rmb();

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_head WRITE ->data_tail

Where A pairs with D, and B pairs with C.

Peter Zijlstra <[email protected]> wrote on 10/25/2013 07:37:49 PM:

Quoted 8 lines

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_head WRITE ->data_tail

Quoted 1 line

Where A pairs with D, and B pairs with C.

1. I agree. My only concern is that architectures which do use atomic operations
with memory barriers, will issue two consecutive barriers now, which is sub-optimal.

2. I think the comment in "include/linux/perf_event.h" describing "data_head" and
"data_tail" for user space need an update as well. Current version -

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_head                     WRITE ->data_tail

Where A pairs with D, and B pairs with C.

I don't think A needs to be a full barrier because we won't in fact write data until we see the store from userspace. So we simply don't issue the data WRITE until we observe it.

OTOH, D needs to be a full barrier since it separates the data READ from the tail WRITE.

For B a WMB is sufficient since it separates two WRITEs, and for C an RMB is sufficient since it separates two READs.

From: Frederic Weisbecker <[email protected]>

2013/10/25 Peter Zijlstra <[email protected]>:

Quoted 16 lines · Frederic Weisbecker

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_head                     WRITE ->data_tail

Where A pairs with D, and B pairs with C.

I don't think A needs to be a full barrier because we won't in fact write data until we see the store from userspace. So we simply don't issue the data WRITE until we observe it.

OTOH, D needs to be a full barrier since it separates the data READ

Quoted 4 lines

the tail WRITE.

For B a WMB is sufficient since it separates two WRITEs, and for C an RMB is sufficient since it separates two READs.

Hmm, I need to defer on you for that, I'm not yet comfortable with picking specific barrier flavours when both write and read are involved in a same side :)

Quoted 21 lines

2013/10/25 Peter Zijlstra <[email protected]>:

Quoted 20 lines · Frederic Weisbecker

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_head                     WRITE ->data_tail

Where A pairs with D, and B pairs with C.

I don't think A needs to be a full barrier because we won't in fact write data until we see the store from userspace. So we simply don't issue the data WRITE until we observe it.

OTOH, D needs to be a full barrier since it separates the data READ from the tail WRITE.

For B a WMB is sufficient since it separates two WRITEs, and for C an RMB is sufficient since it separates two READs.

I think you have a point :) IMO, memory barrier (A) is superfluous. At producer side we need to ensure that "WRITE $data" is not committed to memory before "READ ->data_tail" had seen a new value and if the old one indicated that there is no enough space for a new entry. All this is already guaranteed by control flow dependancy on single CPU - writes will not be committed to the memory if read value of "data_tail" doesn't specify enough free space in the ring buffer.

Likewise, on consumer side, we can make use of natural data dependency and memory ordering guarantee for single CPU and try to replace "smp_mb" by a more light-weight "smp_rmb":

READ ->data_tail                      READ ->data_head
// ...                                smp_rmb()       (C)
WRITE $data                           READ $data
smp_wmb()     (B)                     smp_rmb()       (D)

READ $header_size
STORE ->data_head WRITE ->data_tail = $old_data_tail +
$header_size

We ensure that all $data is read before "data_tail" is written by doing "READ $header_size" after all other data is read and we rely on natural data dependancy between "data_tail" write and "header_size" read.

Quoted 21 lines · Victor Kaplansky

Quoted 21 lines

2013/10/25 Peter Zijlstra <[email protected]>:

Quoted 20 lines · Frederic Weisbecker

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_head                     WRITE ->data_tail

Where A pairs with D, and B pairs with C.

I don't think A needs to be a full barrier because we won't in fact write data until we see the store from userspace. So we simply don't issue the data WRITE until we observe it.

OTOH, D needs to be a full barrier since it separates the data READ from the tail WRITE.

For B a WMB is sufficient since it separates two WRITEs, and for C an RMB is sufficient since it separates two READs.

<snip>

Quoted 16 lines

I think you have a point :) IMO, memory barrier (A) is superfluous. At producer side we need to ensure that "WRITE $data" is not committed to memory before "READ ->data_tail" had seen a new value and if the old one indicated that there is no enough space for a new entry. All this is already guaranteed by control flow dependancy on single CPU - writes will not be committed to the memory if read value of "data_tail" doesn't specify enough free space in the ring buffer.

Likewise, on consumer side, we can make use of natural data dependency and memory ordering guarantee for single CPU and try to replace "smp_mb" by a more light-weight "smp_rmb":

READ ->data_tail                      READ ->data_head
// ...                                smp_rmb()       (C)
WRITE $data                           READ $data
smp_wmb()     (B)                     smp_rmb()       (D)

Quoted 8 lines

READ $header_size
STORE ->data_head WRITE ->data_tail = $old_data_tail +
$header_size

We ensure that all $data is read before "data_tail" is written by doing "READ $header_size" after all other data is read and we rely on natural data dependancy between "data_tail" write and "header_size" read.

I'm not entirely sure I get the $header_size trickery; need to think more on that. But yes, I did consider the other one. However, I had trouble having no pairing barrier for (D).

ISTR something like Alpha being able to miss the update (for a long while) if you don't issue the RMB.

Lets add Paul and Oleg to the thread; this is getting far more 'fun' that it should be ;-)

For completeness; below the patch as I had queued it.
---
Subject: perf: Fix perf ring buffer memory ordering
From: Peter Zijlstra <[email protected]>
Date: Mon Oct 28 13:55:29 CET 2013

The PPC64 people noticed a missing memory barrier and crufty old comments in the perf ring buffer code. So update all the comments and add the missing barrier.

When the architecture implements local_t using atomic_long_t there will be double barriers issued; but short of introducing more conditional barrier primitives this is the best we can do.

Cc [email protected]

Cc [email protected]

Cc Mathieu Desnoyers <[email protected]>

Cc [email protected]

Cc Paul McKenney <[email protected]>

Cc Michael Neuling <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Reported-by Victor Kaplansky <[email protected]>

Tested-by Victor Kaplansky <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/uapi/linux/perf_event.h |   12 +++++++-----
 kernel/events/ring_buffer.c     |   29 ++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 8 deletions(-)

Index: linux-2.6/include/uapi/linux/perf_event.h
===================================================================

include/uapi/linux/perf_event.h +7 -5

@@ -479,13 +479,15 @@ struct perf_event_mmap_page {
 	/*
 	 * Control data for the mmap() data buffer.
 	 *
-	 * User-space reading the @data_head value should issue an rmb(), on
-	 * SMP capable platforms, after reading this value -- see
-	 * perf_event_wakeup().
+	 * User-space reading the @data_head value should issue an smp_rmb(),
+	 * after reading this value.
 	 *
 	 * When the mapping is PROT_WRITE the @data_tail value should be
-	 * written by userspace to reflect the last read data. In this case
-	 * the kernel will not over-write unread data.
+	 * written by userspace to reflect the last read data, after issueing
+	 * an smp_mb() to separate the data read from the ->data_tail store.
+	 * In this case the kernel will not over-write unread data.
+	 *
+	 * See perf_output_put_handle() for the data ordering.
 	 */
 	__u64   data_head;		/* head in the data section */
 	__u64	data_tail;		/* user-space written tail */

Index: linux-2.6/kernel/events/ring_buffer.c
===================================================================

kernel/events/ring_buffer.c +0 -0

@@ -87,10 +87,31 @@ static void perf_output_put_handle(struc
 		goto out;
 

kernel/events/ring_buffer.c +20 -0

+	 *   WRITE $data			READ $data
+	 *   smp_wmb()	(B)			smp_mb()	(D)
+	 *   STORE ->data_head			WRITE ->data_tail
+	 *
+	 * Where A pairs with D, and B pairs with C.
+	 *
+	 * I don't think A needs to be a full barrier because we won't in fact
+	 * write data until we see the store from userspace. So we simply don't
+	 * issue the data WRITE until we observe it.
+	 *
+	 * OTOH, D needs to be a full barrier since it separates the data READ
+	 * from the tail WRITE.
+	 *
+	 * For B a WMB is sufficient since it separates two WRITEs, and for C
+	 * an RMB is sufficient since it separates two READs.
+	 *
+	 * See perf_output_begin().
 	 */
+	smp_wmb();
 	rb->user_page->data_head = head;
 
 	/*

@@ -154,6 +175,8 @@ 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
 		 * write is issued.
+		 *
+		 * See perf_output_put_handle().
 		 */
 		tail = ACCESS_ONCE(rb->user_page->data_tail);
 		smp_rmb();

Quoted 5 lines · Peter Zijlstra

kernel/events/ring_buffer.c +0 -0

@@ -87,10 +87,31 @@ static void perf_output_put_handle(struc
 		goto out;
 

kernel/events/ring_buffer.c static void perf_output_put_handle(struc · line 95

Quoted 10 lines

kernel/events/ring_buffer.c +6 -3

 	/*
-	 * Publish the known good head. Rely on the full barrier implied
-	 * by atomic_dec_and_test() order the rb->head read and this
-	 * write.
+	 * Since the mmap() consumer (userspace) can run on a different CPU:
+	 *
+	 *   kernel				user
+	 *
+	 *   READ ->data_tail			READ ->data_head
+	 *   smp_rmb()	(A)			smp_rmb()	(C)

Given that both of the kernel's subsequent operations are stores/writes, doesn't (A) need to be smp_mb()?

      mb();   // XXXXXXXX: do we really need it? I think yes.

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: 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.

Quoted 6 lines · Victor Kaplansky

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: 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.

I'll make it a full mb for now and too am curious to see the end of this discussion explaining things ;-)

kernel/events/ring_buffer.c int perf_output_begin(struct perf_output · line 185

Quoted 13 lines · Peter Zijlstra

kernel/events/ring_buffer.c +3 -1

@@ -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
 		 * write is issued.
+		 *
+		 * See perf_output_put_handle().
 		 */
 		tail = ACCESS_ONCE(rb->user_page->data_tail);
-		smp_rmb();
+		smp_mb();
 		offset = head = local_read(&rb->head);
 		head += size;
 		if (unlikely(!perf_output_space(rb, tail, offset, head)))

That said; it would be very nice to be able to remove this barrier. This is in every event write path :/

include/uapi/linux/perf_event.h struct perf_event_mmap_page { · line 483

Quoted 12 lines · Peter Zijlstra

On Tue, Oct 29, 2013 at 11:21:31AM +0100, Peter Zijlstra wrote:

include/uapi/linux/perf_event.h +2 -3

@@ -479,13 +479,15 @@ struct perf_event_mmap_page {
 	/*
 	 * Control data for the mmap() data buffer.
 	 *
-	 * User-space reading the @data_head value should issue an rmb(), on
-	 * SMP capable platforms, after reading this value -- see
-	 * perf_event_wakeup().
+	 * User-space reading the @data_head value should issue an smp_rmb(),
+	 * after reading this value.

so where's the patch fixing perf to use the new recommendations?

Is this purely a performance thing or a correctness change?

A change like this a bit of a pain, especially as userspace doesn't really have nice access to smb_mb() defines so a lot of cut-and-pasting will ensue for everyone who's trying to parse the mmap buffer.

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 arch/*/include/asm/barrier.h and with the exception of metag I'm fairly sure they're correct.

Hi Peter,

Quoted 8 lines · Peter Zijlstra

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 arch/*/include/asm/barrier.h and with the exception of metag I'm fairly sure they're correct.

Yeh...

Short answer:
For Meta you're probably best off assuming CONFIG_METAG_SMP_WRITE_REORDERING=n and just using compiler barriers.

Long answer:
The issue with write reordering between Meta's hardware threads beyond the cache is only with a particular SoC, and SMP is not used in production on it.
It is possible to make the LINSYSEVENT_WR_COMBINE_FLUSH register writable to userspace (it's in a non-mappable region already) but even then the write to that register needs odd placement to be effective (before the shmem write rather than after - which isn't a place any existing barriers are guaranteed to be placed). I'm fairly confident we get away with it in the kernel, and userland normally just uses linked load/store instructions for atomicity which works fine.

Commit-ID:  a94d342b9cb09edfe888ea972af0883b6a8d992b
Gitweb:     http://git.kernel.org/tip/a94d342b9cb09edfe888ea972af0883b6a8d992b
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 30 Oct 2013 11:42:46 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 6 Nov 2013 12:34:26 +0100

tools/perf: Add required memory barriers

To match patch bf378d341e48 ("perf: Fix perf ring buffer memory ordering") change userspace to also adhere to the ordering outlined.

tools/perf/util/evlist.h int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s · line 182

Quoted 9 lines · tip-bot for Peter Zijlstra

+++ b/tools/perf/util/evlist.h

tools/perf/util/evlist.h +1 -1

@@ -177,7 +177,7 @@ int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s
 static inline unsigned int perf_mmap__read_head(struct perf_mmap *mm)
 {
 	struct perf_event_mmap_page *pc = mm->base;
-	int head = pc->data_head;
+	int head = ACCESS_ONCE(pc->data_head);
 	rmb();
 	return head;

so is this ACCESS_ONCE required now for proper access to the mmap buffer?

remember that there are users trying to use this outside of the kernel where we don't necessarily have access to internal kernl macros. Some of these users aren't necessarily GPLv2 compatible either (PAPI for example is more or less BSD licensed) so just cutting and pasting chunks of internal kernel macros isn't always the best route either.

Quoted 13 lines · Vince Weaver

tools/perf/util/evlist.h int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s · line 182

Quoted 9 lines · tip-bot for Peter Zijlstra

+++ b/tools/perf/util/evlist.h

tools/perf/util/evlist.h +1 -1

@@ -177,7 +177,7 @@ int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s
 static inline unsigned int perf_mmap__read_head(struct perf_mmap *mm)
 {
 	struct perf_event_mmap_page *pc = mm->base;
-	int head = pc->data_head;
+	int head = ACCESS_ONCE(pc->data_head);
 	rmb();
 	return head;

so is this ACCESS_ONCE required now for proper access to the mmap buffer?

Pretty much; otherwise your C compiler is allowed to mess it up.

Quoted 5 lines

remember that there are users trying to use this outside of the kernel where we don't necessarily have access to internal kernl macros. Some of these users aren't necessarily GPLv2 compatible either (PAPI for example is more or less BSD licensed) so just cutting and pasting chunks of internal kernel macros isn't always the best route either.

Other license stuff is not my problem; that said I doubt there's much copyright to claim on a volatile cast.

Quoted 10 lines · Peter Zijlstra

Quoted 5 lines · Vince Weaver

remember that there are users trying to use this outside of the kernel where we don't necessarily have access to internal kernl macros. Some of these users aren't necessarily GPLv2 compatible either (PAPI for example is more or less BSD licensed) so just cutting and pasting chunks of internal kernel macros isn't always the best route either.

Other license stuff is not my problem; that said I doubt there's much copyright to claim on a volatile cast.

Also, does PAPI actually use the buffer then? I thought that was strictly self monitoring.

Having perf in the kernel tree really makes it hard for you guys to keep a clean API/ABI it seems.

Quoted 2 lines · Vince Weaver

Having perf in the kernel tree really makes it hard for you guys to keep a clean API/ABI it seems.

Lock free buffers are 'fun'.. The ABI can be described as:

read pc->data_head

// ensure no other reads get before this point and ->data_head // doesn't get re-read hereafter.

Quoted 13 lines · Vince Weaver

tools/perf/util/evlist.h int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s · line 182

Quoted 9 lines · tip-bot for Peter Zijlstra

+++ b/tools/perf/util/evlist.h

tools/perf/util/evlist.h +1 -1

@@ -177,7 +177,7 @@ int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, s
 static inline unsigned int perf_mmap__read_head(struct perf_mmap *mm)
 {
 	struct perf_event_mmap_page *pc = mm->base;
-	int head = pc->data_head;
+	int head = ACCESS_ONCE(pc->data_head);
 	rmb();
 	return head;

so is this ACCESS_ONCE required now for proper access to the mmap buffer?

Pretty much; otherwise your C compiler is allowed to mess it up.

long head = ((__atomic long)pc->data_head).load(memory_order_acquire);

coupled with:

((__atomic long)pc->data_tail).store(tail, memory_order_release);

might be the 'right' and proper C11 incantations to avoid having to touch kernel macros; but would obviously require a recent compiler.

Barring that, I think we're stuck with:

long head = ACCESS_ONCE(pc->data_head);
smp_rmb();

...

smp_mb();
pc->data_tail = tail;

And using the right asm goo for the barriers. That said, all these asm barriers should include a compiler barriers (memory clobber) which _should_ avoid the worst compiler trickery -- although I don't think it completely obviates the need for ACCESS_ONCE() -- uncertain there.

Quoted 19 lines · Peter Zijlstra

long head = ((__atomic long)pc->data_head).load(memory_order_acquire);

coupled with:

((__atomic long)pc->data_tail).store(tail, memory_order_release);

might be the 'right' and proper C11 incantations to avoid having to touch kernel macros; but would obviously require a recent compiler.

Barring that, I think we're stuck with:

long head = ACCESS_ONCE(pc->data_head);
smp_rmb();

...

smp_mb();
pc->data_tail = tail;

Quoted 4 lines

And using the right asm goo for the barriers. That said, all these asm barriers should include a compiler barriers (memory clobber) which _should_ avoid the worst compiler trickery -- although I don't think it completely obviates the need for ACCESS_ONCE() -- uncertain there.

http://software.intel.com/en-us/articles/single-producer-single-consumer-queue/

There's one for icc on x86.

Quoted 25 lines · Peter Zijlstra

Quoted 23 lines · Peter Zijlstra

long head = ((__atomic long)pc->data_head).load(memory_order_acquire);

coupled with:

((__atomic long)pc->data_tail).store(tail, memory_order_release);

might be the 'right' and proper C11 incantations to avoid having to touch kernel macros; but would obviously require a recent compiler.

Barring that, I think we're stuck with:

long head = ACCESS_ONCE(pc->data_head);
smp_rmb();

...

smp_mb();
pc->data_tail = tail;

And using the right asm goo for the barriers. That said, all these asm barriers should include a compiler barriers (memory clobber) which _should_ avoid the worst compiler trickery -- although I don't think it completely obviates the need for ACCESS_ONCE() -- uncertain there.

Quoted 4 lines

http://software.intel.com/en-us/articles/single-producer-single-consumer-queue/

There's one for icc on x86.

I think the problem here is this really isn't a good interface.

Most users just want the most recent batch of samples. Something like

    char buffer[4096];
    int count;

    do {
       count=perf_read_sample_buffer(buffer,4096);
       process_samples(buffer);
    } while(count);

where perf_read_sample_buffer() is a syscall that just copies the current valid samples to userspace.

Yes, this is inefficient (requires an extra copy of the values) but the kernel then could handle all the SMP/multithread/barrier/locking issues.

How much overhead is really introduced by making a copy?

Requiring the user of a kernel interface to have a deep knowledge of optimizing compilers, barriers, and CPU memory models is just asking for trouble.

Especially as this all needs to get documented in the manpage and I'm not sure that's possible in a sane fashion.

Quoted 3 lines

Requiring the user of a kernel interface to have a deep knowledge of optimizing compilers, barriers, and CPU memory models is just asking for trouble.

It shouldn't be all that hard to put this in a (lgpl) library others can link to -- that way you can build it once (using GCC).

I don't want a library that is external and under-tested: for example quite a few of the PAPI breakages were found very late, after a new kernel has been released - that's the big disadvantage of librarization and decentralization. The decentralized library model might work if all you want to create is a second-class also-ran GUI, but it just doesn't work very well for actively developed kernel code.

Quoted 6 lines · Ingo Molnar

I don't want a library that is external and under-tested: for example quite a few of the PAPI breakages were found very late, after a new kernel has been released - that's the big disadvantage of librarization and decentralization. The decentralized library model might work if all you want to create is a second-class also-ran GUI, but it just doesn't work very well for actively developed kernel code.

I would argue that PAPI's problem was because it was trying to use perf_event_open() which is a complex, poorly documented kernel interface with a lot of code churn. Usually it's the job of the kernel not to break user-space, not the other way around.

It's too late on the decentralized library issue. PAPI has to support kernels going back to 2.6.32 so it's going to have its own copy of the mmap parsing code even if a new syscall gets introduced.

There are a lot of tools out there now that open-code a perf_event interface. I don't think it's really possible to say "anyone using the syscall without using our kernel library is unsupported".

Quoted 4 lines · Peter Zijlstra

Quoted 3 lines

Requiring the user of a kernel interface to have a deep knowledge of optimizing compilers, barriers, and CPU memory models is just asking for trouble.

Quoted 2 lines

It shouldn't be all that hard to put this in a (lgpl) library others can link to -- that way you can build it once (using GCC).

I'd suggest to expose it via a new perf syscall, using vsyscall methods to not have to enter the kernel for the pure user-space bits. It should also have a real usecase in tools/perf/ so that it's constantly tested, with matching 'perf test' entries, etc.

Quoted 7 lines · Ingo Molnar

Quoted 4 lines · Peter Zijlstra

Quoted 3 lines

Requiring the user of a kernel interface to have a deep knowledge of optimizing compilers, barriers, and CPU memory models is just asking for trouble.

Quoted 7 lines

Quoted 2 lines

It shouldn't be all that hard to put this in a (lgpl) library others can link to -- that way you can build it once (using GCC).

I'd suggest to expose it via a new perf syscall, using vsyscall methods to not have to enter the kernel for the pure user-space bits. It should also have a real usecase in tools/perf/ so that it's constantly tested, with matching 'perf test' entries, etc.

Oh man, I've never poked at the entire vsyscall stuff before; let alone done it for ARM, ARM64, PPC64 etc..

Keeping it in userspace like we have is so much easier.

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines · Victor Kaplansky

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: do we really need it? I think yes.

Quoted 5 lines

Quoted 2 lines

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.

I'll make it a full mb for now and too am curious to see the end of this discussion explaining things ;-)

That is, I've now got this queued:

---
Subject: perf: Fix perf ring buffer memory ordering
From: Peter Zijlstra <[email protected]>
Date: Mon Oct 28 13:55:29 CET 2013

The PPC64 people noticed a missing memory barrier and crufty old comments in the perf ring buffer code. So update all the comments and add the missing barrier.

When the architecture implements local_t using atomic_long_t there will be double barriers issued; but short of introducing more conditional barrier primitives this is the best we can do.

Cc Mathieu Desnoyers <[email protected]>

Cc [email protected]

Cc Paul McKenney <[email protected]>

Cc Michael Neuling <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc [email protected]

Cc [email protected]

Reported-by Victor Kaplansky <[email protected]>

Tested-by Victor Kaplansky <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 include/uapi/linux/perf_event.h |   12 +++++++-----
 kernel/events/ring_buffer.c     |   31 +++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 9 deletions(-)

Index: linux-2.6/include/uapi/linux/perf_event.h
===================================================================

include/uapi/linux/perf_event.h +7 -5

@@ -479,13 +479,15 @@ struct perf_event_mmap_page {
 	/*
 	 * Control data for the mmap() data buffer.
 	 *
-	 * User-space reading the @data_head value should issue an rmb(), on
-	 * SMP capable platforms, after reading this value -- see
-	 * perf_event_wakeup().
+	 * User-space reading the @data_head value should issue an smp_rmb(),
+	 * after reading this value.
 	 *
 	 * When the mapping is PROT_WRITE the @data_tail value should be
-	 * written by userspace to reflect the last read data. In this case
-	 * the kernel will not over-write unread data.
+	 * written by userspace to reflect the last read data, after issueing
+	 * an smp_mb() to separate the data read from the ->data_tail store.
+	 * In this case the kernel will not over-write unread data.
+	 *
+	 * See perf_output_put_handle() for the data ordering.
 	 */
 	__u64   data_head;		/* head in the data section */
 	__u64	data_tail;		/* user-space written tail */

Index: linux-2.6/kernel/events/ring_buffer.c
===================================================================

kernel/events/ring_buffer.c +27 -4

@@ -87,10 +87,31 @@ static void perf_output_put_handle(struc
 		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.
+	 * Since the mmap() consumer (userspace) can run on a different CPU:
+	 *
+	 *   kernel				user
+	 *
+	 *   READ ->data_tail			READ ->data_head
+	 *   smp_mb()	(A)			smp_rmb()	(C)
+	 *   WRITE $data			READ $data
+	 *   smp_wmb()	(B)			smp_mb()	(D)
+	 *   STORE ->data_head			WRITE ->data_tail
+	 *
+	 * Where A pairs with D, and B pairs with C.
+	 *
+	 * I don't think A needs to be a full barrier because we won't in fact
+	 * write data until we see the store from userspace. So we simply don't
+	 * issue the data WRITE until we observe it. Be conservative for now.
+	 *
+	 * OTOH, D needs to be a full barrier since it separates the data READ
+	 * from the tail WRITE.
+	 *
+	 * For B a WMB is sufficient since it separates two WRITEs, and for C
+	 * an RMB is sufficient since it separates two READs.
+	 *
+	 * See perf_output_begin().
 	 */
+	smp_wmb();
 	rb->user_page->data_head = head;
 
 	/*

@@ -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
 		 * write is issued.
+		 *
+		 * See perf_output_put_handle().
 		 */
 		tail = ACCESS_ONCE(rb->user_page->data_tail);
-		smp_rmb();
+		smp_mb();
 		offset = head = local_read(&rb->head);
 		head += size;
 		if (unlikely(!perf_output_space(rb, tail, offset, head)))

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: 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 example.

Quoted 4 lines · Victor Kaplansky

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: do we really need it? I think yes.

Quoted 3 lines

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 example.

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.

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.

Peter Zijlstra <[email protected]> wrote on 10/30/2013 01:25:26 PM:

Quoted 4 lines

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, this one we cannot omit, because our problem on consumer side is not with @tail, which is written exclusively by consumer, but with @head.

BTW, it is why you also don't need ACCESS_ONCE() around @tail, but only around
@head read.

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.

But since it is still a dependent load in that we do that @tail vs @head comparison before doing other loads, wouldn't a read_barrier_depends() be sufficient? Or do we still need a complete rmb?

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 + tail)" to be hoisted beyond "ubuf->head" load you would need something like this:

void
ubuf_read(void)

{
        u64 head, tail;

        tail = ubuf->tail;
        head = ACCESS_ONCE(ubuf->head);

        /*
         * Ensure we read the buffer boundaries before the actual buffer
         * data...
         */

        while (tail != head) {
		    smp_read_barrier_depends();         /* for Alpha */
                obj = *(ubuf->data + head - 128);
                /* process obj */
                tail += obj->size;
                tail %= ubuf->size;
        }

        /*
         * Ensure all data reads are complete before we issue the
         * ubuf->tail update; once that update hits, kbuf_write() can
         * observe and overwrite data.
         */
        smp_mb();               /* D, matches with A */

        ubuf->tail = tail;
}

(note that "head" is part of address calculation of obj load now).

But, even in this demo example some "smp_read_barrier_depends()" before "obj = *(ubuf->data + head - 100);" is required for architectures like Alpha. Though, on more sane architectures "smp_read_barrier_depends()" will be translated to nothing.

Peter Zijlstra <[email protected]> wrote on 10/30/2013 01:25:26 PM:

Quoted 4 lines

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, this one we cannot omit, because our problem on consumer side is not with @tail, which is written exclusively by consumer, but with @head.

BTW, it is why you also don't need ACCESS_ONCE() around @tail, but only around
@head read.

Quoted 3 lines

BTW, it is why you also don't need ACCESS_ONCE() around @tail, but only around
@head read.

If you omit the ACCESS_ONCE() calls around @tail, the compiler is within its rights to combine adjacent operations and also to invent loads and stores, for example, in cases of register pressure.

It is also within
its rights to do piece-at-a-time loads and stores, which might sound unlikely, but which can actually has happened when the compiler figures out exactly what is to be stored at compile time, especially on hardware that only allows small immediate values.

"Paul E. McKenney" <[email protected]> wrote on 10/31/2013 08:16:02 AM:

Quoted 3 lines

Quoted 3 lines

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 from Documentation/circular-buffers.txt for CONSUMER:

        unsigned long head = ACCESS_ONCE(buffer->head);
        unsigned long tail = buffer->tail;      /* AAA */

        if (CIRC_CNT(head, tail, buffer->size) >= 1) {
                /* read index before reading contents at that index */
                smp_read_barrier_depends();

                /* extract one item from the buffer */
                struct item *item = buffer[tail];

                consume_item(item);

                smp_mb(); /* finish reading descriptor before incrementing

tail */

                buffer->tail = (tail + 1) & (buffer->size - 1); /* BBB */
        }

Quoted 3 lines

If you omit the ACCESS_ONCE() calls around @tail, the compiler is within its rights to combine adjacent operations and also to invent loads and stores, for example, in cases of register pressure.

Right. And I was completely aware about these possible transformations when said that ACCESS_ONCE() around @tail in point AAA is redundant. Moved, or even
completely dismissed reads of @tail in consumer code, are not a problem at all,
since @tail is written exclusively by CONSUMER side.

Quoted 5 lines

It is also within
its rights to do piece-at-a-time loads and stores, which might sound unlikely, but which can actually has happened when the compiler figures out exactly what is to be stored at compile time, especially on hardware that only allows small immediate values.

As for writes to @tail, the ACCESS_ONCE around @tail at point AAA, doesn't prevent in any way an imaginary super-optimizing compiler from moving around the store to @tail (which appears in the code at point BBB).

It is why ACCESS_ONCE at point AAA is completely redundant.

Quoted 25 lines · Victor Kaplansky

"Paul E. McKenney" <[email protected]> wrote on 10/31/2013 08:16:02 AM:

Quoted 3 lines

Quoted 3 lines

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 from Documentation/circular-buffers.txt for CONSUMER:

        unsigned long head = ACCESS_ONCE(buffer->head);
        unsigned long tail = buffer->tail;      /* AAA */

        if (CIRC_CNT(head, tail, buffer->size) >= 1) {
                /* read index before reading contents at that index */
                smp_read_barrier_depends();

                /* extract one item from the buffer */
                struct item *item = buffer[tail];

                consume_item(item);

                smp_mb(); /* finish reading descriptor before incrementing

Quoted 4 lines

tail */

                buffer->tail = (tail + 1) & (buffer->size - 1); /* BBB */
        }

Hmmm... I believe that we need to go back to the original code in Documentation/circular-buffers.txt. I do so at the bottom of this email.

Quoted 10 lines

Quoted 3 lines

If you omit the ACCESS_ONCE() calls around @tail, the compiler is within its rights to combine adjacent operations and also to invent loads and stores, for example, in cases of register pressure.

Right. And I was completely aware about these possible transformations when said that ACCESS_ONCE() around @tail in point AAA is redundant. Moved, or even
completely dismissed reads of @tail in consumer code, are not a problem at all,
since @tail is written exclusively by CONSUMER side.

I believe that the lack of ACCESS_ONCE() around the consumer's store to buffer->tail is at least a documentation problem. In the original consumer code, it is trapped between an smp_mb() and a spin_unlock(), but it is updating something that is read without synchronization by some other thread.

Quoted 6 lines

Quoted 5 lines

It is also within
its rights to do piece-at-a-time loads and stores, which might sound unlikely, but which can actually has happened when the compiler figures out exactly what is to be stored at compile time, especially on hardware that only allows small immediate values.

Quoted 6 lines

As for writes to @tail, the ACCESS_ONCE around @tail at point AAA, doesn't prevent in any way an imaginary super-optimizing compiler from moving around the store to @tail (which appears in the code at point BBB).

It is why ACCESS_ONCE at point AAA is completely redundant.

Agreed, it is under the lock that guards modifications, so AAA does not need ACCESS_ONCE().

OK, here is the producer from Documentation/circular-buffers.txt, with some comments added:

	spin_lock(&producer_lock);

	unsigned long head = buffer->head;

	unsigned long tail = ACCESS_ONCE(buffer->tail); /* PT */

	if (CIRC_SPACE(head, tail, buffer->size) >= 1) {
		/* insert one item into the buffer */
		struct item *item = buffer[head];

		produce_item(item); /* PD */

		smp_wmb(); /* commit the item before incrementing the head */

		buffer->head = (head + 1) & (buffer->size - 1);  /* PH */

		/* wake_up() will make sure that the head is committed before
		 * waking anyone up */
		wake_up(consumer);
	}

	spin_unlock(&producer_lock);

And here is the consumer, also from Documentation/circular-buffers.txt:

	spin_lock(&consumer_lock);

	unsigned long head = ACCESS_ONCE(buffer->head); /* CH */
	unsigned long tail = buffer->tail;

	if (CIRC_CNT(head, tail, buffer->size) >= 1) {
		/* read index before reading contents at that index */
		smp_read_barrier_depends();

		/* extract one item from the buffer */
		struct item *item = buffer[tail]; /* CD */

		consume_item(item);

		smp_mb(); /* finish reading descriptor before incrementing tail */

		buffer->tail = (tail + 1) & (buffer->size - 1); /* CT */

	}

	spin_unlock(&consumer_lock);

Here are the ordering requirements as I see them:

1. The producer is not allowed to clobber a location that the
consumer is in the process of reading from.

2. The consumer is not allowed to read from a location that the
producer has not yet completed writing to.

#1 is helped out by the fact that there is always an empty element in the array, so that the producer will need to produce twice in a row to catch up to where the consumer is currently consuming. #2 has no such benefit: The consumer can consume an item that has just now been produced.

#1 requires that CD is ordered before CT in a way that pairs with the ordering of PT and PD. There is of course no effective ordering between PT and PD within a given call to the producer, but we only need the ordering between the read from PT for one call to the producer and the PD of the -next- call to the producer, courtesy of the fact that there is always one empty cell in the array. Therefore, the required ordering between PT of one call and PD of the next is provided by the unlock-lock pair. The ordering of CD and CT is of course provided by the smp_mb(). (And yes, I was missing the unlock-lock pair earlier. In my defense, you did leave this unlock-lock pair out of your example.)

So ordering requirement #1 is handled by the original, but only if you leave the locking in place. The producer's smp_wmb() does not necessarily order prior loads against subsequent stores, and the wake_up() only guarantees ordering if something was actually awakened. As noted earlier, the "if" does not necessarily provide ordering.

On to ordering requirement #2.

This requires that CH and CD is ordered in a way that pairs with ordering between PD and PH. PD and PH are both writes, so the smp_wmb() does the trick there. The consumer side is a bit strange. On DEC Alpha, smp_read_barrier_dependes() turns into smp_mb(), so that case is covered (though by accident). On other architectures, smp_read_barrier_depends() generates no code, and there is no data dependency between the CH and CD. The dependency is instead between the read from ->tail and the write,

and as you noted, ->tail is written by the consumer, not the producer.

But my battery is dying, so more later, including ACCESS_ONCE().

Quoted 5 lines · Paul E. McKenney

Quoted 4 lines · Victor Kaplansky

Oleg Nesterov <[email protected]> wrote on 10/28/2013 10:17:35 PM:

Quoted 1 line

      mb();   // XXXXXXXX: do we really need it? I think yes.

Quoted 8 lines

Quoted 3 lines

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 example.

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.

OK, so let me try.. a slightly less convoluted version of the code in kernel/events/ring_buffer.c coupled with a userspace consumer would look something like the below.

One important detail is that the kbuf part and the kbuf_writer() are strictly per cpu and we can thus rely on implicit ordering for those.

Only the userspace consumer can possibly run on another cpu, and thus we need to ensure data consistency for those.

struct buffer {
	u64 size;
	u64 tail;
	u64 head;
	void *data;
};

struct buffer *kbuf, *ubuf;

/*
* Determine there's space in the buffer to store data at @offset to
* @head without overwriting data at @tail.
*/
bool space(u64 tail, u64 offset, u64 head)

{
	offset = (offset - tail) % kbuf->size;
	head   = (head   - tail) % kbuf->size;

	return (s64)(head - offset) >= 0;
}

/*
* If there's space in the buffer; store the data @buf; otherwise
* discard it.
*/
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;

	if (!space(tail, offset, head)) {
		/* discard @buf */
		return;
	}

	/*
	 * Ensure that if we see the userspace tail (ubuf->tail) such
	 * that there is space to write @buf without overwriting data
	 * userspace hasn't seen yet, we won't in fact store data before
	 * that read completes.
	 */

	smp_mb(); /* A, matches with D */

	write(kbuf->data + offset, buf, sz);
	kbuf->head = head % kbuf->size;

	/*
	 * Ensure that we write all the @buf data before we update the
	 * userspace visible ubuf->head pointer.
	 */
	smp_wmb(); /* B, matches with C */

	ubuf->head = kbuf->head;
}

/*
* Consume the buffer data and update the tail pointer to indicate to
* kernel space there's 'free' space.
*/
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...
	 */
	smp_rmb(); /* C, matches with B */

	while (tail != head) {
		obj = ubuf->data + tail;
		/* process obj */
		tail += obj->size;
		tail %= ubuf->size;
	}

	/*
	 * Ensure all data reads are complete before we issue the
	 * ubuf->tail update; once that update hits, kbuf_write() can
	 * observe and overwrite data.
	 */
	smp_mb(); /* D, matches with A */

	ubuf->tail = tail;
}

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.

If the read shows no available space, we simply will not issue those writes -- therefore we could argue we can avoid the memory barrier.

However, that leaves D unpaired and me confused. We must have D because otherwise the CPU could reorder that write into the reads previous and the kernel could start overwriting data we're still reading.. which seems like a bad deal.

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.

Could we replace A and C with an smp_read_barrier_depends()?

Quoted 27 lines

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...
    */
   smp_rmb(); /* C, matches with B */

   while (tail != head) {
      obj = ubuf->data + tail;
      /* process obj */
      tail += obj->size;
      tail %= ubuf->size;
   }

   /*
    * Ensure all data reads are complete before we issue the
    * ubuf->tail update; once that update hits, kbuf_write() can
    * observe and overwrite data.
    */
   smp_mb(); /* D, matches with A */

Quoted 4 lines

   ubuf->tail = tail;
}

Could we replace A and C with an smp_read_barrier_depends()?

C, yes, given that you have ACCESS_ONCE() on the fetch from ->tail and that the value fetch from ->tail feeds into the address used for the "obj =" assignment.

"Paul E. McKenney" <[email protected]> wrote on 10/31/2013 08:40:15 AM:

Quoted 32 lines

Quoted 31 lines

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...
    */
   smp_rmb(); /* C, matches with B */

   while (tail != head) {
      obj = ubuf->data + tail;
      /* process obj */
      tail += obj->size;
      tail %= ubuf->size;
   }

   /*
    * Ensure all data reads are complete before we issue the
    * ubuf->tail update; once that update hits, kbuf_write() can
    * observe and overwrite data.
    */
   smp_mb(); /* D, matches with A */

   ubuf->tail = tail;
}

Could we replace A and C with an smp_read_barrier_depends()?

Quoted 3 lines

C, yes, given that you have ACCESS_ONCE() on the fetch from ->tail and that the value fetch from ->tail feeds into the address used for the "obj =" assignment.

No! You must to have a full smp_rmb() at C. The race on the reader side is not between fetch of @tail and read from address pointed by @tail. The real race here is between a fetch of @head and read of obj from memory pointed by @tail.

Quoted 3 lines

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? 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 ARM, anyway) and Power could use a weaker barrier, and so on -- but smp_mb() emits a full barrier.

Perhaps a new smp_tmb() for TSO semantics, where reads are ordered before reads, writes before writes, and reads before writes, but not writes before reads? Another approach would be to define a per-arch barrier for this particular case.

Quoted 2 lines

If the read shows no available space, we simply will not issue those writes -- therefore we could argue we can avoid the memory barrier.

Proving that means iterating through the permitted combinations of compilers and architectures... There is always hand-coded assembly language, I suppose.

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines

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? 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 ARM, anyway) and Power could use a weaker barrier, and so on -- but smp_mb() emits a full barrier.

Quoted 4 lines

Perhaps a new smp_tmb() for TSO semantics, where reads are ordered before reads, writes before writes, and reads before writes, but not writes before reads? Another approach would be to define a per-arch barrier for this particular case.

I suppose we can only introduce new barrier primitives if there's more than 1 use-case.

Quoted 6 lines

Quoted 2 lines

If the read shows no available space, we simply will not issue those writes -- therefore we could argue we can avoid the memory barrier.

Proving that means iterating through the permitted combinations of compilers and architectures... There is always hand-coded assembly language, I suppose.

I'm starting to think that while the C/C++ language spec says they can wreck the world by doing these silly optimization, real world users will push back for breaking their existing code.

I'm fairly sure the GCC people _will_ get shouted at _loudly_ when they break the kernel by doing crazy shit like that.

Given its near impossible to write a correct program in C/C++ and tagging the entire kernel with __atomic is equally not going to happen, I think we must find a practical solution.

Either that, or we really need to consider forking the language and compiler :-(

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Paul E. McKenney

Quoted 3 lines

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? 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 ARM, anyway) and Power could use a weaker barrier, and so on -- but smp_mb() emits a full barrier.

Perhaps a new smp_tmb() for TSO semantics, where reads are ordered before reads, writes before writes, and reads before writes, but not writes before reads? Another approach would be to define a per-arch barrier for this particular case.

Quoted 2 lines

I suppose we can only introduce new barrier primitives if there's more than 1 use-case.

There probably are others.

Quoted 14 lines

Quoted 6 lines

Quoted 2 lines

If the read shows no available space, we simply will not issue those writes -- therefore we could argue we can avoid the memory barrier.

Proving that means iterating through the permitted combinations of compilers and architectures... There is always hand-coded assembly language, I suppose.

I'm starting to think that while the C/C++ language spec says they can wreck the world by doing these silly optimization, real world users will push back for breaking their existing code.

I'm fairly sure the GCC people _will_ get shouted at _loudly_ when they break the kernel by doing crazy shit like that.

Quoted 6 lines

Given its near impossible to write a correct program in C/C++ and tagging the entire kernel with __atomic is equally not going to happen, I think we must find a practical solution.

Either that, or we really need to consider forking the language and compiler :-(

Depends on how much benefit the optimizations provide. If they provide little or no benefit, I am with you, otherwise we will need to bit some bullet or another. Keep in mind that there is a lot of code in the kernel that runs sequentially (e.g., due to being fully protected by locks), and aggressive optimizations for that sort of code are harmless.

Can't say I know the answer at the moment, though.

Thanx, Paul

If there was an smp_tmb(), I would likely use it in rcu_assign_pointer().

Quoted 1 line · Paul E. McKenney

If there was an smp_tmb(), I would likely use it in rcu_assign_pointer().

Well, I'm obviously all for introducing this new barrier, for it will reduce a full mfence on x86 to a compiler barrier. And ppc can use lwsync as opposed to sync afaict. Not sure ARM can do better.

---
Subject: arch: Introduce new TSO memory barrier smp_tmb()

Quoted 9 lines · Peter Zijlstra

Quoted 1 line · Paul E. McKenney

If there was an smp_tmb(), I would likely use it in rcu_assign_pointer().

Well, I'm obviously all for introducing this new barrier, for it will reduce a full mfence on x86 to a compiler barrier. And ppc can use lwsync as opposed to sync afaict. Not sure ARM can do better.

---
Subject: arch: Introduce new TSO memory barrier smp_tmb()

This is specialized enough that I would *really* like the name to be more descriptive. Compare to the special "smp_read_barrier_depends()"
maco: it's unusual, and it has very specific semantics, so it gets a long and descriptive name.

Memory ordering is subtle enough without then using names that are subtle in themselves. mb/rmb/wmb are conceptually pretty simple operations, and very basic when talking about memory ordering. "acquire" and "release" are less simple, but have descriptive names and have very specific uses in locking.

In contrast "smp_tmb()" is a *horrible* name, because TSO is a description of the memory ordering, not of a particular barrier. It's also not even clear that you can have a "tso barrier", since the ordering (like acquire/release) presumably is really about one particular *store*, not about some kind of barrier between different operations.

So please describe exactly what the semantics that barrier has, and then name the barrier that way.

I assume that in this particular case, the semantics RCU wants is "write barrier, and no preceding reads can move past this point".

Calling that "smp_tmb()" is f*cking insane, imnsho.

Quoted 29 lines · Linus Torvalds

Quoted 9 lines · Peter Zijlstra

Quoted 1 line · Paul E. McKenney

If there was an smp_tmb(), I would likely use it in rcu_assign_pointer().

Well, I'm obviously all for introducing this new barrier, for it will reduce a full mfence on x86 to a compiler barrier. And ppc can use lwsync as opposed to sync afaict. Not sure ARM can do better.

---
Subject: arch: Introduce new TSO memory barrier smp_tmb()

This is specialized enough that I would *really* like the name to be more descriptive. Compare to the special "smp_read_barrier_depends()"
maco: it's unusual, and it has very specific semantics, so it gets a long and descriptive name.

Memory ordering is subtle enough without then using names that are subtle in themselves. mb/rmb/wmb are conceptually pretty simple operations, and very basic when talking about memory ordering. "acquire" and "release" are less simple, but have descriptive names and have very specific uses in locking.

In contrast "smp_tmb()" is a *horrible* name, because TSO is a description of the memory ordering, not of a particular barrier. It's also not even clear that you can have a "tso barrier", since the ordering (like acquire/release) presumably is really about one particular *store*, not about some kind of barrier between different operations.

Quoted 5 lines

So please describe exactly what the semantics that barrier has, and then name the barrier that way.

I assume that in this particular case, the semantics RCU wants is "write barrier, and no preceding reads can move past this point".

Quoted 1 line

Calling that "smp_tmb()" is f*cking insane, imnsho.

Fair enough; from what I could gather the proposed semantics are RELEASE+WMB, such that neither reads not writes can cross over, writes can't cross back, but reads could.

Since both RELEASE and WMB are trivial under TSO the entire thing collapses.

Now I'm currently completely confused as to what C/C++ wrecks vs actual proper memory order issues; let alone fully comprehend the case that started all this.

smp_storebuffer_mb() -- A barrier that enforces those orderings
that do not invalidate the hardware store-buffer optimization.

smp_not_w_r_mb() -- A barrier that orders everything except prior
writes against subsequent reads.

smp_acqrel_mb() -- A barrier that combines C/C++ acquire and release
semantics. (C/C++ "acquire" orders a specific load against subsequent loads and stores, while C/C++ "release" orders a specific store against prior loads and stores.)

Quoted 3 lines · Paul E. McKenney

smp_storebuffer_mb() -- A barrier that enforces those orderings
that do not invalidate the hardware store-buffer optimization.

Ugh. Maybe. Can you guarantee that those are the correct semantics? And why talk about the hardware semantics, when you really want specific semantics for the *software*.

Quoted 2 lines

smp_not_w_r_mb() -- A barrier that orders everything except prior
writes against subsequent reads.

Ok, that sounds more along the lines of "these are the semantics we want", but I have to say, it also doesn't make me go "ahh, ok".

Quoted 4 lines

smp_acqrel_mb() -- A barrier that combines C/C++ acquire and release
semantics. (C/C++ "acquire" orders a specific load against subsequent loads and stores, while C/C++ "release" orders a specific store against prior loads and stores.)

I don't think this is true. acquire+release is much stronger than what you're looking for - it doesn't allow subsequent reads to move past the write (because that would violate the acquire part). On x86, for example, you'd need to have a locked cycle for smp_acqrel_mb().

So again, what are the guarantees you actually want? Describe those. And then make a name.

I _think_ the guarantees you want is:
- SMP write barrier
- *local* read barrier for reads preceding the write.

but the problem is that the "preceding reads" part is really specifically about the write that you had. The barrier should really be attached to the *particular* write operation, it cannot be a standalone barrier.

So it would *kind* of act like a "smp_wmb() + smp_rmb()", but the problem is that a "smp_rmb()" doesn't really "attach" to the preceding write.

This is analogous to a "acquire" operation: you cannot make an "acquire" barrier, because it's not a barrier *between* two ops, it's associated with one particular op.

So what I *think* you actually really really want is a "store with release consistency, followed by a write barrier".

In TSO, afaik all stores have release consistency, and all writes are ordered, which is why this is a no-op in TSO. And x86 also has that "all stores have release consistency, and all writes are ordered" model, even if TSO doesn't really describe the x86 model.

But on ARM64, for example, I think you'd really want the store itself to be done with "stlr" (store with release), and then follow up with a "dsb st" after that.

And notice how that requires you to mark the store itself. There is no actual barrier *after* the store that does the optimized model.

Of course, it's entirely possible that it's not worth worrying about this on ARM64, and that just doing it as a "normal store followed by a full memory barrier" is good enough. But at least in *theory* a microarchitecture might make it much cheaper to do a "store with release consistency" followed by "write barrier".

Anyway, having talked exhaustively about exactly what semantics you are after, I *think* the best model would be to just have a

#define smp_store_with_release_semantics(x, y) ...

and use that *and* a "smp_wmb()" for this (possibly a special "smp_wmb_after_release()" if that allows people to avoid double barriers). On x86 (and TSO systems), the smp_store_with_release_semantics() would be just a regular store, and the smp_wmb() is obviously a no-op. Other platforms would end up doing other things.

Hmm?

OK, something like this for the definitions (though PowerPC might want to locally abstract the lwsync expansion):

	#define smp_store_with_release_semantics(p, v) /* x86, s390, etc. */ \
	do { \
		barrier(); \
		ACCESS_ONCE(p) = (v); \
	} while (0)

	#define smp_store_with_release_semantics(p, v) /* PowerPC. */ \
	do { \
		__asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory"); \
		ACCESS_ONCE(p) = (v); \
	} while (0)

	#define smp_load_with_acquire_semantics(p) /* x86, s390, etc. */ \
	({ \
		typeof(*p) *_________p1 = ACCESS_ONCE(p); \
		barrier(); \
		_________p1; \
	})

	#define smp_load_with_acquire_semantics(p) /* PowerPC. */ \
	({ \
		typeof(*p) *_________p1 = ACCESS_ONCE(p); \
		__asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory"); \
		_________p1; \
	})

For ARM, smp_load_with_acquire_semantics() is a wrapper around the ARM "ldar" instruction and smp_store_with_release_semantics() is a wrapper around the ARM "stlr" instruction.

Quoted 29 lines · Paul E. McKenney

OK, something like this for the definitions (though PowerPC might want to locally abstract the lwsync expansion):

	#define smp_store_with_release_semantics(p, v) /* x86, s390, etc. */ \
	do { \
		barrier(); \
		ACCESS_ONCE(p) = (v); \
	} while (0)

	#define smp_store_with_release_semantics(p, v) /* PowerPC. */ \
	do { \
		__asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory"); \
		ACCESS_ONCE(p) = (v); \
	} while (0)

	#define smp_load_with_acquire_semantics(p) /* x86, s390, etc. */ \
	({ \
		typeof(*p) *_________p1 = ACCESS_ONCE(p); \
		barrier(); \
		_________p1; \
	})

	#define smp_load_with_acquire_semantics(p) /* PowerPC. */ \
	({ \
		typeof(*p) *_________p1 = ACCESS_ONCE(p); \
		__asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory"); \
		_________p1; \
	})

Quoted 3 lines

For ARM, smp_load_with_acquire_semantics() is a wrapper around the ARM "ldar" instruction and smp_store_with_release_semantics() is a wrapper around the ARM "stlr" instruction.

This still leaves me confused as to what to do with my case :/

Slightly modified since last time -- as the simplified version was maybe simplified too far.

To recap, I'd like to get rid of barrier A where possible, since that's now a full barrier for every event written.

However, there's no immediate store I can attach it to; the obvious one would be the kbuf->head store, but that's complicated by the local_cmpxchg() thing.

And we need that cmpxchg loop because a hardware NMI event can interleave with a software event.

And to be honest, I'm still totally confused about memory barriers vs control flow vs C/C++. The only way we're ever getting to that memcpy is if we've already observed ubuf->tail, so that LOAD has to be fully processes and completed.

I'm really not seeing how a STORE from the memcpy() could possibly go wrong; and if C/C++ can hoist the memcpy() over a compiler barrier() then I suppose we should all just go home.

/me who wants A to be a barrier() but is terminally confused.

---

/*
* One important detail is that the kbuf part and the kbuf_writer() are
* strictly per cpu and we can thus rely on program order for those. *
* Only the userspace consumer can possibly run on another cpu, and thus we
* need to ensure data consistency for those.
*/

struct buffer {
        u64 size;
        u64 tail;
        u64 head;
        void *data;
};

struct buffer *kbuf, *ubuf;

/*
* If there's space in the buffer; store the data @buf; otherwise
* discard it.
*/
void kbuf_write(int sz, void *buf)

{
	u64 tail, head, offset;

	do {
		tail = ACCESS_ONCE(ubuf->tail);

		offset = head = kbuf->head;
		if (CIRC_SPACE(head, tail, kbuf->size) < sz) {
			/* discard @buf */
			return;
		}
		head += sz;
	} while (local_cmpxchg(&kbuf->head, offset, head) != offset)

/*
* Ensure that if we see the userspace tail (ubuf->tail) such
* that there is space to write @buf without overwriting data
* userspace hasn't seen yet, we won't in fact store data before
* that read completes.
*/

smp_mb(); /* A, matches with D */

        memcpy(kbuf->data + offset, buf, sz);

        /*
         * Ensure that we write all the @buf data before we update the
         * userspace visible ubuf->head pointer.
         */
        smp_wmb(); /* B, matches with C */

        ubuf->head = kbuf->head;

}

/*
* Consume the buffer data and update the tail pointer to indicate to
* kernel space there's 'free' space.
*/
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...
         */
        smp_rmb(); /* C, matches with B */

        while (tail != head) {
                obj = ubuf->data + tail;
                /* process obj */
                tail += obj->size;
                tail %= ubuf->size;
        }

        /*
         * Ensure all data reads are complete before we issue the
         * ubuf->tail update; once that update hits, kbuf_write() can
         * observe and overwrite data.
         */
        smp_mb(); /* D, matches with A */

        ubuf->tail = tail;

}

Quoted 27 lines

/*
* One important detail is that the kbuf part and the kbuf_writer() are
* strictly per cpu and we can thus rely on program order for those. *
* Only the userspace consumer can possibly run on another cpu, and thus we
* need to ensure data consistency for those.
*/

struct buffer {
        u64 size;
        u64 tail;
        u64 head;
        void *data;
};

struct buffer *kbuf, *ubuf;

/*
* If there's space in the buffer; store the data @buf; otherwise
* discard it.
*/
void kbuf_write(int sz, void *buf)

{
	u64 tail, head, offset;

Quoted 2 lines

	do {
		tail = ACCESS_ONCE(ubuf->tail);

So the above load is the key load. It determines whether or not we have space in the buffer. This of course assumes that only this CPU writes to ->head.

If so, then:

		tail = smp_load_with_acquire_semantics(ubuf->tail); /* A -> D */

Quoted 7 lines

		offset = head = kbuf->head;
		if (CIRC_SPACE(head, tail, kbuf->size) < sz) {
			/* discard @buf */
			return;
		}
		head += sz;
	} while (local_cmpxchg(&kbuf->head, offset, head) != offset)

If there is an issue with kbuf->head, presumably local_cmpxchg() fails and we retry.

But sheesh, do you think we could have buried the definitions of local_cmpxchg() under a few more layers of macro expansion just to keep things more obscure? Anyway, griping aside...

o __cmpxchg_local_generic() in include/asm-generic/cmpxchg-local.h
doesn't seem to exclude NMIs, so is not safe for this usage.

o __cmpxchg_local() in ARM handles NMI as long as the
argument is 32 bits, otherwise, it uses the aforementionted __cmpxchg_local_generic(), which does not handle NMI. Given your u64, this does not look good...

And some ARM subarches (e.g., metag) seem to fail to handle NMI even in the 32-bit case.

o FRV and M32r seem to act similar to ARM.

Or maybe these architectures don't do NMIs? If they do, local_cmpxchg() does not seem to be safe against NMIs in general. :-/

That said, powerpc, 64-bit s390, sparc, and x86 seem to handle it.

Of course, x86's local_cmpxchg() has full memory barriers implicitly.

Quoted 8 lines

/*
* Ensure that if we see the userspace tail (ubuf->tail) such
* that there is space to write @buf without overwriting data
* userspace hasn't seen yet, we won't in fact store data before
* that read completes.
*/

Quoted 1 line

        smp_mb(); /* A, matches with D */

Given a change to smp_load_with_acquire_semantics() above, you should not need this smp_mb().

Quoted 9 lines

        memcpy(kbuf->data + offset, buf, sz);

        /*
         * Ensure that we write all the @buf data before we update the
         * userspace visible ubuf->head pointer.
         */
        smp_wmb(); /* B, matches with C */

Quoted 1 line

        ubuf->head = kbuf->head;

Replace the smp_wmb() and the assignment with:

	smp_store_with_release_semantics(ubuf->head, kbuf->head); /* B -> C */

Quoted 10 lines

}

/*
* Consume the buffer data and update the tail pointer to indicate to
* kernel space there's 'free' space.
*/
void ubuf_read(void)

{
        u64 head, tail;

Quoted 1 line

        tail = ACCESS_ONCE(ubuf->tail);

Does anyone else write tail? Or is this defense against NMIs?

If no one else writes to tail and if NMIs cannot muck things up, then the above ACCESS_ONCE() is not needed, though I would not object to its staying.

Quoted 1 line

        head = ACCESS_ONCE(ubuf->head);

Make the above be:

	head = smp_load_with_acquire_semantics(ubuf->head);  /* C -> B */

Quoted 5 lines

        /*
         * Ensure we read the buffer boundaries before the actual buffer
         * data...
         */
        smp_rmb(); /* C, matches with B */

And drop the above memory barrier.

Quoted 14 lines

        while (tail != head) {
                obj = ubuf->data + tail;
                /* process obj */
                tail += obj->size;
                tail %= ubuf->size;
        }

        /*
         * Ensure all data reads are complete before we issue the
         * ubuf->tail update; once that update hits, kbuf_write() can
         * observe and overwrite data.
         */
        smp_mb(); /* D, matches with A */

Quoted 1 line

        ubuf->tail = tail;

Replace the above barrier and the assignment with:

	smp_store_with_release_semantics(ubuf->tail, tail); /* D -> B. */

Quoted 1 line

}

All this is leading me to suggest the following shortenings of names:

	smp_load_with_acquire_semantics() -> smp_load_acquire()

	smp_store_with_release_semantics() -> smp_store_release()

But names aside, the above gets rid of explicit barriers on TSO architectures, allows ARM to avoid full DMB, and allows PowerPC to use lwsync instead of the heavier-weight sync.

All this is leading me to suggest the following shortenings of names:

	smp_load_with_acquire_semantics() -> smp_load_acquire()

	smp_store_with_release_semantics() -> smp_store_release()

But names aside, the above gets rid of explicit barriers on TSO architectures, allows ARM to avoid full DMB, and allows PowerPC to use lwsync instead of the heavier-weight sync.