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

[Peter Zijlstra]

On Sat, Apr 20, 2019 at 01:54:40AM -0700, Paul E. McKenney wrote:
> 	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().

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.

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

> 	And lock acquisition??? acm_read_bulk_callback().

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

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

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

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.

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