Atomic inc/dec should use ATOMIC_SEQ_CST

[ringabout]

It was introduced by 6fbc96f#diff-f533629679edcd5f0af7eef220728c122d4fc577f81590ced7fe8028ea129808
which replaced sync_add_and_fetch with atomicAddFetch(memLoc.addr, x, ATOMIC_RELAXED). However sync_add_and_fetch is close to ATOMIC_SEQ_CST instead of ATOMIC_RELAXED. It is a regression since 2013.

If you need a memory order of std::memory_order_seq_cst (which is probably the closest to the GCC __sync_fetch_and_add),

Ref https://stackoverflow.com/questions/14858770/c11-stdatomic-fetch-add-vs-sync-fetch-and-add

Also it is not correct. We cannot assume ATOMIC_RELAXED order works for all conditions. It should use stricter memory order. For example std/atomics and C++ use ATOMIC_SEQ_CST order which is correct.

operator++(int) and operator++() on atomic types are specified to have the effect of fetch_add(1), which ends up calling the member function with the default memory ordering memory_order_seq_cst.

Ref https://stackoverflow.com/questions/13482751/c-atomic-increment-memory-ordering
Ref

Nim/lib/pure/concurrency/atomics.nim

Line 355 in 0d0c249

proc fetchAdd*[T: SomeInteger](location: var Atomic[T]; value: T; order: MemoryOrder = moSequentiallyConsistent): T {.inline.} =

This PR intends to make it correct. Thanks to nim-works/nimskull#88

[Araq]

I don't agree with this. atomicInc is not used for "publish message", it's used for counting.

[planetis-m]

Thing is, https://github.com/nim-lang/threading/blob/master/threading/atomics.nim#L158 uses SeqCst too...

[ringabout]

It may be safe for atomicInc to use relaxed memory order, but atomicDec is not.

Nim/lib/pure/concurrency/atomics.nim

Line 408 in 0d0c249

proc atomicDec*[T: SomeInteger](location: var Atomic[T]; value: T = 1) {.inline.} =

And std/atomics uses SeqCst too.

It may be easily misused, for instance the example below is wrong.

It is important to enforce any possible access to the object in one thread (through an existing reference) to happen before deleting the object in a different thread. This is achieved by a "release" operation after dropping a reference (any access to the object through this reference must obviously happened before), and an "acquire" operation before deleting the object.

So decRef should not use relaxed memory order.

proc decRef(b: Buffer) {.inline.} =
  if atomicDec(b.refcount) <= 0:
    deallocShared(b)

proc incRef(b: Buffer) {.inline.} =
  atomicInc(b.refcount)

Ref

Nim/lib/deprecated/pure/sharedstrings.nim

Line 27 in c658de2

proc decRef(b: Buffer) {.inline.} =

Ref https://stackoverflow.com/questions/48124031/stdmemory-order-relaxed-atomicity-with-respect-to-the-same-atomic-variable

[ringabout]

atomicInc is not used for "publish message", it's used for counting.

I agree. And I think a typical use of atomicDec is to publish data. And I expect atomcInc, atomcDec should use the same memory order among system/atomics(relaxed), std/atomics(seq_cst) and threading/atomics(seq_cst), which gives the least surprise. Otherwise, the surprise needs to be documented.

[arnetheduck]

What's missing here is a way to control the flag, really - which flag to use depends on your specific use case for incrementing and decrementing.

That said, +1 for SEQ_CST as default in general, with an option to "relax" it - there is no case where SEQ_CST yields an invalid result - conversely, there are some cases where inc without SEQ_CST does produce a hard-to-debug, invalid result (the fact that you can implement smart pointers in one particular way with a relaxed inc is a lucky/clever outcome of that special case - in that particular implementation, you can also relax atomicDec "a little": you don't need full SEQ_CST (you can do a release decrease followed by an acquire fence, then deallocate the instance - splitting these up allows a "cheaper" dec in some cases)

[ringabout]

That said, +1 for SEQ_CST as default in general, with an option to "relax" it - there is no case where SEQ_CST yields an invalid result - conversely, there are some cases where inc without SEQ_CST does produce a hard-to-debug, invalid result (the fact that you can implement smart pointers in one particular way with a relaxed inc is a lucky/clever outcome of that special case - in that particular implementation, you can also relax atomicDec "a little": you don't need full SEQ_CST (you can do a release decrease followed by an acquire fence, then deallocate the instance - splitting these up allows a "cheaper" dec in some cases)

In that case atomicAddFetch, atomicSubFetch should be used instead. They are exported procs and support memory orders. There is no need to add an option to AtomicInc and AtomicDec which are just simple wrappers for forementioned procs.

Similiar operator can be found in C++ (no extra option)

operator++(int) and operator++() on atomic types are specified to have the effect of fetch_add(1), which ends up calling the member function with the default memory ordering memory_order_seq_cst.

[arnetheduck]

They are exported procs

So are atomicInc and atomicDec - in C++ though, you get a nice and safe "shortcut" way of writing it (++) - in nim, the closest operator would be += or possibly inc - once you call them atomicInc, it's no reason to use it really, over atomicAddFetch.. another good option is simply to deprecate them since they're already incorrect for the intended use case (reference counting) and don't provide any clear advantage of a simple add, clarity-wise.

[Araq]

IIRC atomicInc predated C++'s memory model for threading, so it's no surprise we have something here which makes little sense. That said, the primary use case was reference counting and for that the "relaxed" mode makes more sense. (You need a "full" memory barrier before the delete but only after the last RC decrement, you don't need to have the barrier for every decrement.)

[ringabout]

That said, the primary use case was reference counting and for that the "relaxed" mode makes more sense. (You need a "full" memory barrier before the delete but only after the last RC decrement, you don't need to have the barrier for every decrement.)

@Araq so the code of std/sharedstring should be changed into something below?

proc decRef(b: Buffer) {.inline.} =
  if atomicDec(b.refcount) <= 0:
    fence() # seq_cst
    deallocShared(b)

proc incRef(b: Buffer) {.inline.} =
  atomicInc(b.refcount)

Ref

Nim/lib/deprecated/pure/sharedstrings.nim

Line 27 in c658de2

proc decRef(b: Buffer) {.inline.} =

[Araq]

@Araq so the code of std/sharedstring should be changed into something below?

Correct. But let's also ask @planetis-m and @mratsim on this matter.

[planetis-m]

Probably the best explanation about atomic reference counting is in Herb Sutter - atomic Weapons 2 of 2 talk. (what Arne already said)

[Araq]

That means I'm wrong, doesn't it?

[Araq]

Or maybe I'm right, see https://youtu.be/M15UKpNlpeM?t=2962

[ringabout]

I added a "help wanted" label. If someone is familiar with atomics, please explain to me why
#19212 (comment) is correct/wrong. Thanks!

IMO, considering all platforms (not just x86), supposing there is a thread A changed the content of the shared string; thread A needs to release the result to the last thread which would free the shared string. With atomicDec (relaxed order), thread A may not release the result to store buffer. If there is only a fence for the last thread, the last thread may see an old value before freeing the shared string.

[Araq]

If there is only a fence for the last thread, the last thread may see an old value before freeing the shared string.

Well the last thread has a "acquire" fence, so how would this be possible? Hans Boehm claimed it works in 2016, see my link. (Argument by authority.)

[ringabout]

Well this is an example of boost:

#include <boost/intrusive_ptr.hpp>
#include <boost/atomic.hpp>

class X {
public:
  typedef boost::intrusive_ptr<X> pointer;
  X() : refcount_(0) {}

private:
  mutable boost::atomic<int> refcount_;
  friend void intrusive_ptr_add_ref(const X * x)
  {
    x->refcount_.fetch_add(1, boost::memory_order_relaxed);
  }
  friend void intrusive_ptr_release(const X * x)
  {
    if (x->refcount_.fetch_sub(1, boost::memory_order_release) == 1) { // why it is not omitted
      boost::atomic_thread_fence(boost::memory_order_acquire);
      delete x;
    }
  }
};

Ref https://www.boost.org/doc/libs/1_77_0/doc/html/atomic/usage_examples.html

It uses release order though the last thread does have acquire fence. So I'm not sure.

[Araq]

https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3390 also uses relaxed for the inc operation. So at least let the inc use relaxed.

[ringabout]

also uses relaxed for the inc operation.

I agree atomicInc can use relaxed order in many situations.

However

the non-default orderings are meant for experts

(it drives me mad :)

So I still prefer the solutions below

• deprecate atomcInc and atomicDec and document the memory order
• default atomcInc and atomicDec to seq_cst

I can make following up PRs to replace them with atomicAddFetch and atomicSubFecth in stdlib for efficiency.

BTW relaxed order implies acq_rel in x86 architects, so it still has some penalties compared to that on ARM.

Ref https://stackoverflow.com/questions/28049976/x86-relaxed-ordering-performance

[Araq]

Alright.

[ringabout]

Thank you!

[mratsim]

That said, the primary use case was reference counting and for that the "relaxed" mode makes more sense. (You need a "full" memory barrier before the delete but only after the last RC decrement, you don't need to have the barrier for every decrement.)

@Araq so the code of std/sharedstring should be changed into something below?

proc decRef(b: Buffer) {.inline.} =
  if atomicDec(b.refcount) <= 0:
    fence() # seq_cst
    deallocShared(b)

proc incRef(b: Buffer) {.inline.} =
  atomicInc(b.refcount)

Ref

Nim/lib/deprecated/pure/sharedstrings.nim

Line 27 in c658de2

proc decRef(b: Buffer) {.inline.} =

I think it's a bad idea to use {.inline.} with atomics that are not sequentially consistent.
Because there is usually an assumption that function calls with side-effect are not reordered, it would make spotting a memory ordering issue really hard.

This atomic refcounting has been well tested:

https://github.com/mratsim/weave/blob/71dc2d700d6f509c71e5b659ab4c5e9cb4020dde/weave/cross_thread_com/flow_events.nim#L176-L201

# Refcounting is started from 0 and we avoid fetchSub with release semantics
# in the common case of only one reference being live.

proc `=destroy`*(event: var FlowEvent) =
  if event.e.isNil:
    return

  let count = event.e.refCount.load(moRelaxed)
  fence(moAcquire)
  if count == 0:
    # We have the last reference
    if not event.e.isNil:
      if event.e.kind == Iteration:
        wv_free(event.e.union.iter.singles)
      # Return memory to the memory pool
      recycle(event.e)
  else:
    discard fetchSub(event.e.refCount, 1, moRelease)
  event.e = nil

proc `=sink`*(dst: var FlowEvent, src: FlowEvent) {.inline.} =
  # Don't pay for atomic refcounting when compiler can prove there is no ref change
  `=destroy`(dst)
  system.`=sink`(dst.e, src.e)

proc `=`*(dst: var FlowEvent, src: FlowEvent) {.inline.} =
  `=destroy`(dst)
  discard fetchAdd(src.e.refCount, 1, moRelaxed)
  dst.e = src.e

This is a common pattern:
Acquire loading, check if we have the last reference and in that case, no need for atomics anymore. Otherwise use release ordering.

Ultimately, having formally verified code would dispel all doubt (M Thread Collider is long overdue) but you can at least try with threadsanitizer.

[arnetheduck]

I think it's a bad idea to use {.inline.} with atomics that are not sequentially consistent.

tbh, I don't see what one thing has to do with the other - {.inline.} for a private function in nim is a mostly a no-op - for non-private functions, it merely spams the function into all C sources.

atomics and fences on the other hand have special meaning to the C compiler which explicitly prevents reordering - the two things are unrelated.

[Araq]

Your = is wrong though, you need to change the order to:

proc `=`*(dst: var FlowEvent, src: FlowEvent) {.inline.} =
  discard fetchAdd(src.e.refCount, 1, moRelaxed)
  `=destroy`(dst)
  dst.e = src.e

in order to correctly deal with self-assignments.

[mratsim]

I think it's a bad idea to use {.inline.} with atomics that are not sequentially consistent.

tbh, I don't see what one thing has to do with the other - {.inline.} for a private function in nim is a mostly a no-op - for non-private functions, it merely spams the function into all C sources.

atomics and fences on the other hand have special meaning to the C compiler which explicitly prevents reordering - the two things are unrelated.

A C compiler will never reorder function calls. One of the goal of atomics is telling the compiler which instruction can be reordered and not. An inlined function can be reordered.

[arnetheduck]

An inlined function can be reordered.

The inline keyword in nim has nothing to do with whether a function gets inlined or not in the final assembly - it controls whether the function is copy-pasted to every C source as a C static function or not - neither does the inline keyword in C - the C compiler is free to not inline functions marked inline and vice versa. The inline keyword is a linking directive, first and foremost - it controls whether inlining is possible under certain conditions and whether the linker also has to provide a non-inlined version of the function in the final executable / library (in addition to any inlined versions of it).

As a consequence of the above, functions in general have nothing to do with with reordering - reordering is done in later stages typically, to hoist variables from loops and to provide efficient pipelining based on the CPU architecture.

The C compiler is free to inline anything it wants unless explicitly prohibited, and reorder anything it wants unless explicitly prohibited as long as it maintains an as-if guarantee under single-threaded conditions.

Atomic instructions do two things, as commonly understood:

• they ensure that the entire operation is performed atomically (for example that 64 bits get written in one go, and cannot be observed "partially" written, for example as would happen with two 32-bit writes) - "relaxed" atomic operations provide only this guarantee
• they provide fences that prevent reordering under specific conditions, depending on the fence type

These special prohibitions are done both at a compiler and at a hardware level (preventing the compiler from reordering and adding fences and locks to instructions so that the CPU itself doesn't reorder them).