Re: [RFC][PATCH v3]: documentation,atomic: Add new documents

[Boqun Feng]

On Mon, Jul 31, 2017 at 10:43:45AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 31, 2017 at 07:04:03PM +0800, Boqun Feng wrote:
> > On Mon, Jul 31, 2017 at 11:05:35AM +0200, Peter Zijlstra wrote:
> > > On Wed, Jul 26, 2017 at 08:47:50PM +0800, Boqun Feng wrote:
> > > 
> > > > > +
> > > > > +Further, while something like:
> > > > > +
> > > > > +  smp_mb__before_atomic();
> > > > > +  atomic_dec(&X);
> > > > > +
> > > > > +is a 'typical' RELEASE pattern, the barrier is strictly stronger than
> > > > > +a RELEASE. Similarly for something like:
> > > > > +
> > > > 
> > > > .. at here. Maybe you planned to put stronger ACQUIRE pattern?
> > > 
> > > Yes, although I struggled to find a sensible one. The problem is that
> > > ACQUIRE is on loads and value returning atomics have an ACQUIRE variant,
> > > so why would you ever want to use smp_mb__after_atomic() for this.
> > > 
> > > 
> > > That is, the best I could come up with is something like:
> > > 
> > > 	val = atomic_fetch_or_relaxed(1, &var);
> > > 	smp_mb__after_atomic();
> > > 
> > > But in that case we should've just written:
> > > 
> > > 	val = atomic_fetch_or_acquire(1, &var);
> > > 
> > 
> > Agreed.
> > 
> > And besides, in memory-barriers.txt, the wording is:
> > 
> >  (*) smp_mb__before_atomic();
> >  (*) smp_mb__after_atomic();
> > 
> >      These are for use with atomic (such as add, subtract, increment and
> >      decrement) functions that don't return a value, especially when used for
> >      reference counting. 
> > 
> > So actually, using smp_mb__after_atomic() for ACQUIRE is a misuse.
> 
> You lost me on this one.
> 
> Why wouldn't the following have ACQUIRE semantics?
> 
> 	atomic_inc(&var);
> 	smp_mb__after_atomic();
> 
> Is the issue that there is no actual value returned or some such?
> 

That "misuse" is a wrong word there ;-( 

I actually meant "the usage is correct but we don't encourage using
smp_mb__after_atomic() *only* for an ACQUIRE, because _acquire() could
always be used in that case, as Peter said".

In fact in your case, the ordering is stronger, both the load and store
part of the atomic op are ordered with the memory ops following it.

In short, I suggested we tell users to use
smp_mb__{before,after}_atomic() only when _{release,acquire} ops don't
suffice, i.e. for an RCsc RELEASE or a smp_mb() to order ops other than
the atomic op itself(like the one you use in __call_rcu_nocb_enqueue()).

But maybe this is too strict, and Peter said he would write something
about it in IRC, so I'm not that stick to this suggestion ;-)

Regards,
Boqun

> > > Suggestions?
> > 
> > As a result, I think it's better we say smp_mb__{before,after}_atomic()
> > are only for 1) non-value-returning RmW atomic ops, 2)
> > {set,clear,change}_bit and 3) internal use of atomic primitives(e.g. the
> > generic version of fully ordered atomics).
> > 
> > 1) prevents people to use it for an ACQUIRE, but allows for a RELEASE.
> > 1) & 2) makes atomic_t.txt consistent with memory-barriers.txt
> > 3) explains our usage of those barriers internally.
> > 
> > Thoughts?
> 
> So if I have something like this, the assertion really can trigger?
> 
> 	WRITE_ONCE(x, 1);		atomic_inc(&y);
> 	r0 = xchg_release(&y, 5);	smp_mb__after_atomic();
> 					r1 = READ_ONCE(x);
> 
> 
> 	WARN_ON(r0 == 0 && r1 == 0);
> 
> I must confess that I am not seeing why we would want to allow this
> outcome.
> 
> 							Thanx, Paul
>

Attachment: signature.asc
Description: PGP signature