Documentation: atomic_t.txt: Explain ordering provided by smp_mb__{before,after}_atomic()

The description of smp_mb__before_atomic() and smp_mb__after_atomic() in Documentation/atomic_t.txt is slightly terse and misleading. It does not clearly state that these barriers only affect the ordering of other instructions with respect to the atomic operation.

This improves the text to make the actual ordering implications clear, and also to explain how these barriers differ from a RELEASE or ACQUIRE ordering.

Signed-off-by Alan Stern <[email protected]>

---

 Documentation/atomic_t.txt |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: usb-devel/Documentation/atomic_t.txt
===================================================================

Documentation/atomic_t.txt +8 -3

@@ -171,7 +171,10 @@ The barriers:
   smp_mb__{before,after}_atomic()
 
 only apply to the RMW ops and can be used to augment/upgrade the ordering
-inherent to the used atomic op. These barriers provide a full smp_mb().
+inherent to the used atomic op. Unlike normal smp_mb() barriers, they order
+only the RMW op itself against the instructions preceding the
+smp_mb__before_atomic() or following the smp_mb__after_atomic(); they do
+not order instructions on the other side of the RMW op at all.
 
 These helper barriers exist because architectures have varying implicit
 ordering on their SMP atomic primitives. For example our TSO architectures

@@ -195,7 +198,8 @@ Further, while something like:
   atomic_dec(&X);
 
 is a 'typical' RELEASE pattern, the barrier is strictly stronger than
-a RELEASE. Similarly for something like:
+a RELEASE because it orders preceding instructions against both the read
+and write parts of the atomic_dec(). Similarly, something like:
 
   atomic_inc(&X);
   smp_mb__after_atomic();

@@ -227,7 +231,8 @@ strictly stronger than ACQUIRE. As illus
 
 This should not happen; but a hypothetical atomic_inc_acquire() --
 (void)atomic_fetch_inc_acquire() for instance -- would allow the outcome,
-since then:
+because it would not order the W part of the RMW against the following
+WRITE_ONCE.  Thus:
 
   P1			P2
 

Documentation/atomic_t.txt The barriers: · line 177

Quoted 13 lines · Alan Stern

Index: usb-devel/Documentation/atomic_t.txt
===================================================================

Documentation/atomic_t.txt +4 -1

@@ -171,7 +171,10 @@ The barriers:
   smp_mb__{before,after}_atomic()
 
 only apply to the RMW ops and can be used to augment/upgrade the ordering
-inherent to the used atomic op. These barriers provide a full smp_mb().
+inherent to the used atomic op. Unlike normal smp_mb() barriers, they order
+only the RMW op itself against the instructions preceding the
+smp_mb__before_atomic() or following the smp_mb__after_atomic(); they do
+not order instructions on the other side of the RMW op at all.

Now it is I who is confused; what?

	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);
	y = 1;

the stores to both x and y will be ordered as if an smp_mb() where there. There is no order between a and y otoh.

Quoted 17 lines · Alan Stern

Documentation/atomic_t.txt The barriers: · line 177

Quoted 13 lines

Index: usb-devel/Documentation/atomic_t.txt
===================================================================

Documentation/atomic_t.txt +4 -1

@@ -171,7 +171,10 @@ The barriers:
   smp_mb__{before,after}_atomic()
 
 only apply to the RMW ops and can be used to augment/upgrade the ordering
-inherent to the used atomic op. These barriers provide a full smp_mb().
+inherent to the used atomic op. Unlike normal smp_mb() barriers, they order
+only the RMW op itself against the instructions preceding the
+smp_mb__before_atomic() or following the smp_mb__after_atomic(); they do
+not order instructions on the other side of the RMW op at all.

Now it is I who is confused; what?

Quoted 7 lines

	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);
	y = 1;

the stores to both x and y will be ordered as if an smp_mb() where there. There is no order between a and y otoh.

Let's look at x86. And a slightly different example:

	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);
	r1 = y;

The atomic_add() asm does not have the "memory" constraint, which is completely legitimate because atomic_add() does not return a value, and thus guarantees no ordering. The compiler is therefore within its rights to transform the code into the following:

	x = 1;
	smp_mb__before_atomic();
	r1 = y;
	atomic_add(1, &a);

But x86's smp_mb__before_atomic() is just a compiler barrier, and x86 is further allowed to reorder prior stores with later loads. The CPU can therefore execute this code as follows:

	r1 = y;
	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);

So in general, the ordering is guaranteed only to the atomic itself, not to accesses on the other side of the atomic.

Paul E. McKenney's on April 20, 2019 4:26 am:

Quoted 47 lines · Peter Zijlstra

Quoted 17 lines · Alan Stern

Documentation/atomic_t.txt The barriers: · line 177

Quoted 13 lines

Index: usb-devel/Documentation/atomic_t.txt
===================================================================

Documentation/atomic_t.txt +4 -1

@@ -171,7 +171,10 @@ The barriers:
   smp_mb__{before,after}_atomic()
 
 only apply to the RMW ops and can be used to augment/upgrade the ordering
-inherent to the used atomic op. These barriers provide a full smp_mb().
+inherent to the used atomic op. Unlike normal smp_mb() barriers, they order
+only the RMW op itself against the instructions preceding the
+smp_mb__before_atomic() or following the smp_mb__after_atomic(); they do
+not order instructions on the other side of the RMW op at all.

Now it is I who is confused; what?

Quoted 7 lines

	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);
	y = 1;

the stores to both x and y will be ordered as if an smp_mb() where there. There is no order between a and y otoh.

Let's look at x86. And a slightly different example:

	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);
	r1 = y;

The atomic_add() asm does not have the "memory" constraint, which is completely legitimate because atomic_add() does not return a value, and thus guarantees no ordering. The compiler is therefore within its rights to transform the code into the following:

	x = 1;
	smp_mb__before_atomic();
	r1 = y;
	atomic_add(1, &a);

But x86's smp_mb__before_atomic() is just a compiler barrier, and x86 is further allowed to reorder prior stores with later loads. The CPU can therefore execute this code as follows:

Quoted 7 lines

	r1 = y;
	x = 1;
	smp_mb__before_atomic();
	atomic_add(1, &a);

So in general, the ordering is guaranteed only to the atomic itself, not to accesses on the other side of the atomic.

That's interesting. I don't think that's what all our code expects. I had the same idea as Peter.

IIRC the primitive was originally introduced exactly so x86 would not need to have the unnecessary hardware barrier with sequences like

  smp_mb();
  ...
  atomic_inc(&v);

The "new" semantics are a bit subtle. One option might be just to replace it entirely with atomic_xxx_mb() ?

3. Make non-value-returning atomics provide full ordering.
This would of course need some benchmarking, but would be a simple change to make and would eliminate a large class of potential bugs. My guess is that the loss in performance would be non-negligible, but who knows?

Quoted 5 lines · Paul E. McKenney

3. Make non-value-returning atomics provide full ordering.
This would of course need some benchmarking, but would be a simple change to make and would eliminate a large class of potential bugs. My guess is that the loss in performance would be non-negligible, but who knows?

Well, only for the architectures that have smp_mb__{before,after}_atomic() as barrier(), which are: ia64, mips, s390, sparc, x86 and xtense.

$ ./compare.sh defconfig-build defconfig-build1 vmlinux
do_profile_hits                                           275        278   +3,+0
freezer_apply_state                                        86         98   +12,+0
perf_event_alloc                                         2232       2261   +29,+0
_free_event                                               631        660   +29,+0
shmem_add_to_page_cache                                   712        722   +10,+0
_enable_swap_info                                         333        337   +4,+0
do_mmu_notifier_register                                  303        311   +8,+0
__nfs_commit_inode                                        356        359   +3,+0
tcp_try_coalesce                                          246        250   +4,+0
i915_gem_free_object                                       90         97   +7,+0
mce_intel_hcpu_update                                      39         47   +8,+0
__ia32_sys_swapoff                                       1177       1181   +4,+0
pci_enable_ats                                            124        131   +7,+0
__x64_sys_swapoff                                        1178       1182   +4,+0
i915_gem_madvise_ioctl                                    447        443   -4,+0
calc_global_load_tick                                      75         82   +7,+0
i915_gem_object_set_tiling                                712        708   -4,+0
					     total   11374236   11374367   +131,+0
Which doesn't look too bad.

---

arch/x86/include/asm/atomic.h +4 -4

index ea3d95275b43..115127c7ad28 100644
@@ -54,7 +54,7 @@ static __always_inline void arch_atomic_add(int i, atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "addl %1,%0"
 		     : "+m" (v->counter)
-		     : "ir" (i));
+		     : "ir" (i) : "memory");
 }
 
 /**

@@ -68,7 +68,7 @@ static __always_inline void arch_atomic_sub(int i, atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "subl %1,%0"
 		     : "+m" (v->counter)
-		     : "ir" (i));
+		     : "ir" (i) : "memory");
 }
 
 /**

@@ -95,7 +95,7 @@ static __always_inline bool arch_atomic_sub_and_test(int i, atomic_t *v)
 static __always_inline void arch_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
-		     : "+m" (v->counter));
+		     : "+m" (v->counter) :: "memory");
 }
 #define arch_atomic_inc arch_atomic_inc
 

@@ -108,7 +108,7 @@ static __always_inline void arch_atomic_inc(atomic_t *v)
 static __always_inline void arch_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
-		     : "+m" (v->counter));
+		     : "+m" (v->counter) :: "memory");
 }
 #define arch_atomic_dec arch_atomic_dec
 

arch/x86/include/asm/atomic64_64.h +4 -4

index dadc20adba21..5e86c0d68ac1 100644
@@ -45,7 +45,7 @@ static __always_inline void arch_atomic64_add(long i, atomic64_t *v)
 {
 	asm volatile(LOCK_PREFIX "addq %1,%0"
 		     : "=m" (v->counter)
-		     : "er" (i), "m" (v->counter));
+		     : "er" (i), "m" (v->counter) : "memory");
 }
 
 /**

@@ -59,7 +59,7 @@ static inline void arch_atomic64_sub(long i, atomic64_t *v)
 {
 	asm volatile(LOCK_PREFIX "subq %1,%0"
 		     : "=m" (v->counter)
-		     : "er" (i), "m" (v->counter));
+		     : "er" (i), "m" (v->counter) : "memory");
 }
 
 /**

@@ -87,7 +87,7 @@ static __always_inline void arch_atomic64_inc(atomic64_t *v)
 {
 	asm volatile(LOCK_PREFIX "incq %0"
 		     : "=m" (v->counter)
-		     : "m" (v->counter));
+		     : "m" (v->counter) : "memory");
 }
 #define arch_atomic64_inc arch_atomic64_inc
 

@@ -101,7 +101,7 @@ static __always_inline void arch_atomic64_dec(atomic64_t *v)
 {
 	asm volatile(LOCK_PREFIX "decq %0"
 		     : "=m" (v->counter)
-		     : "m" (v->counter));
+		     : "m" (v->counter) : "memory");
 }
 #define arch_atomic64_dec arch_atomic64_dec
 

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

3. Make non-value-returning atomics provide full ordering.
This would of course need some benchmarking, but would be a simple change to make and would eliminate a large class of potential bugs. My guess is that the loss in performance would be non-negligible, but who knows?

Quoted 3 lines

Well, only for the architectures that have smp_mb__{before,after}_atomic() as barrier(), which are: ia64, mips, s390, sparc, x86 and xtense.

The weakly ordered architectures would need to add the equivalent of smp_mb() before and after, right? This might result in a more noticeable loss of performance.

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

3. Make non-value-returning atomics provide full ordering.
This would of course need some benchmarking, but would be a simple change to make and would eliminate a large class of potential bugs. My guess is that the loss in performance would be non-negligible, but who knows?

Well, only for the architectures that have smp_mb__{before,after}_atomic() as barrier(), which are: ia64, mips, s390, sparc, x86 and xtense.

Quoted 3 lines

The weakly ordered architectures would need to add the equivalent of smp_mb() before and after, right? This might result in a more noticeable loss of performance.

The weak archs already have: smp_mb__{before,after}_atomic() := smp_mb().

Agreed, but I thought that one of the ideas going forward was to get rid of smp_mb__{before,after}_atomic().

Quoted 2 lines · Paul E. McKenney

Agreed, but I thought that one of the ideas going forward was to get rid of smp_mb__{before,after}_atomic().

It's not one I had considered.. I just wanted to get rid of this 'surprise' behaviour.

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Paul E. McKenney

Agreed, but I thought that one of the ideas going forward was to get rid of smp_mb__{before,after}_atomic().

Quoted 2 lines

It's not one I had considered.. I just wanted to get rid of this 'surprise' behaviour.

Ah, good point, your patch is in fact a midpoint between those two positions. Just to make sure I understand:

1. Without your patch, smp_mb__{before,after}_atomic() orders
only against the atomic itself.

2. With your patch, smp_mb__{before,after}_atomic() orders against
the atomic itself and the accesses on the other side of that atomic. However, it does not order the atomic against the accesses on the other side of that atomic.

Putting things between the smp_mb__{before,after}_atomic() and the atomic is in my opinion a bad idea, but in this case they are not necessarily ordered.

3. Dispensing with smp_mb__{before,after}_atomic() would have
void RMW atomics fully ordered, but I suspect that it results in ugly performance regressions.

Or am I still missing something?

And atomic_set(): set_preempt_state(). This fails
on x86, s390, and TSO friends, does it not? Or is
this ARM-only? Still, why not just smp_mb() before and
after? Same issue in __kernfs_new_node(), bio_cnt_set(),
sbitmap_queue_update_wake_batch(),

Ditto for atomic64_set() in __ceph_dir_set_complete().

Ditto for atomic_read() in rvt_qp_is_avail(). This function has a couple of other oddly placed smp_mb__before_atomic().

And atomic_cmpxchg(): msc_buffer_alloc(). This instance of smp_mb__before_atomic() can be removed unless I am missing something subtle. Ditto for kvm_vcpu_exiting_guest_mode(), pv_kick_node(), __sbq_wake_up(),

And lock acquisition??? acm_read_bulk_callback().

In nfnl_acct_fill_info(), a smp_mb__before_atomic() after a atomic64_xchg()??? Also before a clear_bit(), but the clear_bit() is inside an "if".

	There are a few cases that would see added overhead.  For example,
	svc_get_next_xprt() has the following:

		smp_mb__before_atomic();
		clear_bit(SP_CONGESTED, &pool->sp_flags);
		clear_bit(RQ_BUSY, &rqstp->rq_flags);
		smp_mb__after_atomic();

	And xs_sock_reset_connection_flags() has this:

		smp_mb__before_atomic();
		clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
		clear_bit(XPRT_CLOSING, &xprt->state);
		xs_sock_reset_state_flags(xprt);  /* Also a clear_bit(). */
		smp_mb__after_atomic();

	Yeah, there are more than a few misuses, aren't there?  :-/
	A coccinelle script seems in order.  In 0day test robot.

But there are a number of helper functions whose purpose seems to be to wrap an atomic in smp_mb__before_atomic() and smp_mb__after_atomic(), so some of the atomic_xxx_mb() functions might be a good idea just for improved readability.

Quoted 6 lines · Paul E. McKenney

And atomic_set(): set_preempt_state(). This fails
on x86, s390, and TSO friends, does it not? Or is
this ARM-only? Still, why not just smp_mb() before and
after? Same issue in __kernfs_new_node(), bio_cnt_set(),
sbitmap_queue_update_wake_batch(),

Quoted 4 lines

Ditto for atomic64_set() in __ceph_dir_set_complete().

Ditto for atomic_read() in rvt_qp_is_avail(). This function has a couple of other oddly placed smp_mb__before_atomic().

That are just straight up bugs. The atomic_t.txt file clearly specifies the barriers only apply to RmW ops and both _set() and _read() are specified to not be a RmW.

Quoted 4 lines

And atomic_cmpxchg(): msc_buffer_alloc(). This instance of smp_mb__before_atomic() can be removed unless I am missing something subtle. Ditto for kvm_vcpu_exiting_guest_mode(), pv_kick_node(), __sbq_wake_up(),

Note that pv_kick_node() uses cmpxchg_relaxed(), which does not otherwise imply barriers.

Quoted 1 line

And lock acquisition??? acm_read_bulk_callback().

I think it goes with the set_bit() earlier, but what do I know.

Quoted 3 lines

In nfnl_acct_fill_info(), a smp_mb__before_atomic() after a atomic64_xchg()??? Also before a clear_bit(), but the clear_bit() is inside an "if".

Since it is _before, I'm thinking the pairing was intended with the clear_bit(), and yes, then I would expect the smp_mb__before_atomic() to be part of that same branch.

Quoted 16 lines

	There are a few cases that would see added overhead.  For example,
	svc_get_next_xprt() has the following:

		smp_mb__before_atomic();
		clear_bit(SP_CONGESTED, &pool->sp_flags);
		clear_bit(RQ_BUSY, &rqstp->rq_flags);
		smp_mb__after_atomic();

	And xs_sock_reset_connection_flags() has this:

		smp_mb__before_atomic();
		clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
		clear_bit(XPRT_CLOSING, &xprt->state);
		xs_sock_reset_state_flags(xprt);  /* Also a clear_bit(). */
		smp_mb__after_atomic();

Quoted 2 lines

Yeah, there are more than a few misuses, aren't there? :-/ A coccinelle script seems in order. In 0day test robot.

If we can get it to flag the right patterns, then yes that might be useful regardless of the issue at hand, people seem to get this one wrong a lot.

Quoted 4 lines

But there are a number of helper functions whose purpose seems to be to wrap an atomic in smp_mb__before_atomic() and smp_mb__after_atomic(), so some of the atomic_xxx_mb() functions might be a good idea just for improved readability.

Are there really sites where _mb() makes sense? The above is just a lot of buggy code.

There are a great many that look like this:

	smp_mb__before_atomic();
	clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
	smp_mb__after_atomic();

Quoted 5 lines · Paul E. McKenney

There are a great many that look like this:

	smp_mb__before_atomic();
	clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
	smp_mb__after_atomic();

Ooh, marvel at the comment describing the ordering there. Oh wait :-( So much for checkpatch.pl I suppose.

I think the first is a release order for the 'LOCK' bit and the second is because of wake_up_bit() being a shitty interface.

So maybe that should've been:

	clear_bit_unlock(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
	smp_mb__after_atomic();
	wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);

instead?

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Paul E. McKenney

And atomic_set(): set_preempt_state(). This fails
on x86, s390, and TSO friends, does it not? Or is
this ARM-only? Still, why not just smp_mb() before and
after? Same issue in __kernfs_new_node(), bio_cnt_set(),
sbitmap_queue_update_wake_batch(),

Ditto for atomic64_set() in __ceph_dir_set_complete().

Quoted 6 lines

Quoted 2 lines

Ditto for atomic_read() in rvt_qp_is_avail(). This function has a couple of other oddly placed smp_mb__before_atomic().

That are just straight up bugs. The atomic_t.txt file clearly specifies the barriers only apply to RmW ops and both _set() and _read() are specified to not be a RmW.

Agreed. The "Ditto" covers my atomic_set() consternation. ;-)

Quoted 10 lines · Paul E. McKenney

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Paul E. McKenney

And atomic_set(): set_preempt_state(). This fails
on x86, s390, and TSO friends, does it not? Or is
this ARM-only? Still, why not just smp_mb() before and
after? Same issue in __kernfs_new_node(), bio_cnt_set(),
sbitmap_queue_update_wake_batch(),

Ditto for atomic64_set() in __ceph_dir_set_complete().

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Ditto for atomic_read() in rvt_qp_is_avail(). This function has a couple of other oddly placed smp_mb__before_atomic().

That are just straight up bugs. The atomic_t.txt file clearly specifies the barriers only apply to RmW ops and both _set() and _read() are specified to not be a RmW.

Agreed. The "Ditto" covers my atomic_set() consternation. ;-)

I was working on some of these before the Easter break [1, 2]: the plan was to continue next week, but by addressing the remaining cases with a conservative s/that barrier/smp_mb at first; unless you've other plans?

Andrea

[1] http://lkml.kernel.org/r/1555417031-27356-1-git-send-email-andrea.parri@amarulasolutions.com
[2] http://lkml.kernel.org/r/[email protected]

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Paul E. McKenney

And lock acquisition??? acm_read_bulk_callback().

I think it goes with the set_bit() earlier, but what do I know.

Quite possibly! In that case it should be smp_mb__after_atomic(), and it would be nice if it immediately followed the set_bit().

Quoted 7 lines · Paul E. McKenney

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Paul E. McKenney

And lock acquisition??? acm_read_bulk_callback().

I think it goes with the set_bit() earlier, but what do I know.

Quoted 2 lines

Quite possibly! In that case it should be smp_mb__after_atomic(), and it would be nice if it immediately followed the set_bit().

I noticed this one last week as well. The set_bit() had been incorrectly moved and without noticing the smp_mb__before_atomic(). I've submitted a patch to restore it and to fix a related issue to due missing barriers:

	https://lkml.kernel.org/r/[email protected]