RE: [PATCH] refcount_t: add ACQUIRE ordering on success for dec(sub)_and_test variants

[Reshetova, Elena]

> On Mon, Jan 28, 2019 at 1:10 PM Elena Reshetova
> <elena.reshetova@xxxxxxxxx> wrote:
> >
> > This adds an smp_acquire__after_ctrl_dep() barrier on successful
> > decrease of refcounter value from 1 to 0 for refcount_dec(sub)_and_test
> > variants and therefore gives stronger memory ordering guarantees than
> > prior versions of these functions.
> >
> > Co-Developed-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Elena Reshetova <elena.reshetova@xxxxxxxxx>
> > ---
> >  Documentation/core-api/refcount-vs-atomic.rst | 28
> +++++++++++++++++++++++----
> >  arch/x86/include/asm/refcount.h               | 21 ++++++++++++++++----
> >  lib/refcount.c                                | 16 ++++++++++-----
> >  3 files changed, 52 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/core-api/refcount-vs-atomic.rst
> b/Documentation/core-api/refcount-vs-atomic.rst
> > index 322851b..95d4b4e 100644
> > --- a/Documentation/core-api/refcount-vs-atomic.rst
> > +++ b/Documentation/core-api/refcount-vs-atomic.rst
> > @@ -54,6 +54,14 @@ must propagate to all other CPUs before the release
> operation
> >  (A-cumulative property). This is implemented using
> >  :c:func:`smp_store_release`.
> >
> > +An ACQUIRE memory ordering guarantees that all post loads and
> > +stores (all po-later instructions) on the same CPU are
> > +completed after the acquire operation. It also guarantees that all
> > +po-later stores on the same CPU and all propagated stores from other CPUs
> > +must propagate to all other CPUs after the acquire operation
> > +(A-cumulative property). This is implemented using
> > +:c:func:`smp_acquire__after_ctrl_dep`.
> 
> The second part starting from "It also guarantees that". I am not sure
> I understand what it means. Is it just a copy-paste from RELEASE? I am
> not sure ACQUIRE provides anything like this.
> 	

So, you are saying that ACQUIRE does not guarantee that "po-later stores
on the same CPU and all propagated stores from other CPUs
must propagate to all other CPUs after the acquire operation "? 
I was reading about acquire before posting this and trying to understand,
and this was my conclusion that it should provide this, but I can easily be wrong
on this. 

Andrea, Peter, could you please comment?


> 
> > +
> >  A control dependency (on success) for refcounters guarantees that
> >  if a reference for an object was successfully obtained (reference
> >  counter increment or addition happened, function returned true),
> > @@ -119,24 +127,36 @@ Memory ordering guarantees changes:
> >     result of obtaining pointer to the object!
> >
> >
> > -case 5) - decrement-based RMW ops that return a value
> > ------------------------------------------------------
> > +case 5) - generic dec/sub decrement-based RMW ops that return a value
> > +---------------------------------------------------------------------
> >
> >  Function changes:
> >
> >   * :c:func:`atomic_dec_and_test` --> :c:func:`refcount_dec_and_test`
> >   * :c:func:`atomic_sub_and_test` --> :c:func:`refcount_sub_and_test`
> > +
> > +Memory ordering guarantees changes:
> > +
> > + * fully ordered --> RELEASE ordering + ACQUIRE ordering and control
> dependency
> > +   on success.
> 
> Is ACQUIRE strictly stronger than control dependency?

In my understanding yes.

> It generally looks so unless there is something very subtle that I am
> missing. If so, should we replace it with just "RELEASE ordering +
> ACQUIRE ordering on success"? Looks simpler with less magic trickery.

I was just trying to mention all the applicable orderings/guarantees. 
I can remove "control dependency" part if it is easier for people to understand
(the main goal of documentation).

Best Regards,
Elena.