Re: [RFC][PATCH]: documentation,atomic: Add a new atomic_t document

[Will Deacon <will.deacon@arm.com>]

Hi Peter,

On Fri, Jun 09, 2017 at 11:24:50AM +0200, Peter Zijlstra wrote:
> 
> Since we've vastly expanded the atomic_t interface in recent years the
> existing documentation is woefully out of date and people seem to get
> confused a bit.
> 
> Start a new document to hopefully better explain the current state of
> affairs.
>
> The old atomic_ops.txt also covers bitmaps and a few more details so
> this is not a full replacement and we'll therefore keep that document
> around until such a time that we've managed to write more text to cover
> its entire.

Yeah, we should aim at killing (replacing) most of atomic_ops.txt in the
medium term, but this is a good start.

> Also please, ReST people, go away.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> 
> --- /dev/null	2017-05-05 13:16:22.636212333 +0200
> +++ b/Documentation/atomic_t.txt	2017-06-09 11:05:31.501599153 +0200
> @@ -0,0 +1,147 @@
> +
> +On atomic types (atomic_t atomic64_t and atomic_long_t).
> +
> +The atomic type provides an interface to the architecture's means of atomic
> +RmW operations between CPUs (it specifically does not order/work/etc. on
> +IO).

We should be stronger here: atomics to IO could lead to kernel panics (i.e.
raise a fatal abort), whereas this sounds like they just lose some ordering
or atomicity guarantees.

> +The 'full' API consists of:

Need to mention the 64-bit and the long variants?

> +Non RmW ops:
> +
> +  atomic_read(), atomic_set()
> +  atomic_read_acquire(), atomic_set_release()
> +
> +
> +RmW atomic operations:
> +
> +Arithmetic:
> +
> +  atomic_{add,sub,inc,dec}()
> +  atomic_{add,sub,inc,dec}_return{,_relaxed,_acquire,_release}()
> +  atomic_fetch_{add,sub,inc,dec}{,_relaxed,_acquire,_release)()
> +
> +
> +Bitwise:
> +
> +  atomic_{and,or,xor,notand}()
> +  atomic_fetch_{and,or,xor,notand}{,_relaxed,_acquire,_release}()

s/notand/andnot/

> +
> +Swap:
> +
> +  atomic_xchg{,_relaxed,_acquire,_release}()
> +  atomic_cmpxchg{,_relaxed,_acquire,_release}()
> +  atomic_try_cmpxchg{,_relaxed,_acquire,_release}()
> +
> +
> +Reference count (but please see refcount_t):
> +
> +  atomic_add_unless(), atomic_inc_not_zero()
> +  atomic_sub_and_test(), atomic_dec_and_test()
> +
> +
> +Misc:
> +
> +  atomic_inc_and_test(), atomic_add_negative()
> +  atomic_dec_unless_positive(), atomic_inc_unless_negative()

I *think* you have all of them here.

> +
> +Barriers:
> +
> +  smp_mb__{before,after}_atomic()
> +
> +
> +
> +Non RmW ops:
> +
> +The non-RmW ops are (typically) regular LOADs and STOREs and are canonically
> +implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
> +smp_store_release() respectively.
> +
> +The one detail to this is that atomic_set() should be observable to the RmW
> +ops. That is:
> +
> +  CPU0						CPU1
> +
> +  val = atomic_read(&X)
> +  do {
> +						atomic_set(&X, 0)
> +	new = val + 1;
> +  } while (!atomic_try_cmpxchg(&X, &val, new));
> +
> +Should cause the cmpxchg to *FAIL* (when @val != 0). This is typically true;
> +on 'normal' platforms; a regular competing STORE will invalidate a LL/SC.

I see what you're getting at here, but the example is a bit weird because
CPU1 might hold the store to X in a local store-buffer and not shoot down
the cmpxchg immediately. I think you need something to show how the write
to X has at least partially propagated.

> +The obvious case where this is not so is where we need to implement atomic ops
> +with a spinlock hashtable; the typical solution is to then implement
> +atomic_set() with atomic_xchg().

Looking at sparc32, atomic_set takes the hashed lock, so I can't see what
goes wrong here: atomic_try_cmpxchg will get called with val !=0, but the
comparison will fail because the value in memory will be 0. What am I
missing?

> +
> +
> +RmW ops:
> +
> +These come in various forms:
> +
> + - plain operations without return value: atomic_{}()

Maybe just list the API here, instead of having the separate section
previously?

> + - operations which return the modified value: atomic_{}_return()
> +
> +   these are limited to the arithmetic operations because those are
> +   reversible. Bitops are irreversible and therefore the modified value
> +   is of dubious utility.
> +
> + - operations which return the original value: atomic_fetch_{}()
> +
> + - swap operations: xchg(), cmpxchg() and try_cmpxchg()
> +
> + - misc; the special purpose operations that are commonly used and would,
> +   given the interface, normally be implemented using (try_)cmpxchg loops but
> +   are time critical and can, (typically) on LL/SC architectures, be more
> +   efficiently implemented.
> +
> +
> +All these operations are SMP atomic; that is, the operations (for a single
> +atomic variable) can be fully ordered and no intermediate state is lost or
> +visible.
> +
> +
> +Ordering:  (go read memory-barriers.txt first)
> +
> +The rule of thumb:
> +
> + - non-RmW operations are unordered;
> +
> + - RmW operations that have no return value are unordered;
> +
> + - RmW operations that have a return value are Sequentially Consistent;

I think it's stronger than that, because they also order non-RmW operations,
whereas this makes it sounds like there's just a total order over all RmW
operations.

> + - RmW operations that are conditional are unordered on FAILURE, otherwise the
> +   above rules apply.

We should make it clear that "unordered" here refers to accesses to other
memory locations.

> +
> +Except of course when an operation has an explicit ordering like:
> +
> + {}_relaxed: unordered
> + {}_acquire: the R of the RmW is an ACQUIRE
> + {}_release: the W of the RmW is a  RELEASE
> +
> +NOTE: our ACQUIRE/RELEASE are RCpc

The NOTE belongs in memory-barriers.txt

> +The barriers:
> +
> +  smp_mb__{before,after}_atomic()
> +
> +only apply to the RmW ops and can be used to augment/upgrade the ordering
> +inherit to the used atomic op. These barriers provide a full smp_mb().

inherit?

Will