Skip to content

Convert all atomic operations in isc_rwlock to release-acquire memory ordering

Ondřej Surý requested to merge 1428-possible-data-race-in-rbtdb-happens-occasionally-on-ppc64le into master

The memory ordering in the rwlock was all wrong, I am copying excerpts from the https://en.cppreference.com/w/c/atomic/memory_order#Relaxed_ordering for the convenience of the reader:

Relaxed ordering

Atomic operations tagged memory_order_relaxed are not synchronization operations; they do not impose an order among concurrent memory accesses. They only guarantee atomicity and modification order consistency.

Release-Acquire ordering

If an atomic store in thread A is tagged memory_order_release and an atomic load in thread B from the same variable is tagged memory_order_acquire, all memory writes (non-atomic and relaxed atomic) that happened-before the atomic store from the point of view of thread A, become visible side-effects in thread B. That is, once the atomic load is completed, thread B is guaranteed to see everything thread A wrote to memory.

The synchronization is established only between the threads releasing and acquiring the same atomic variable. Other threads can see different order of memory accesses than either or both of the synchronized threads.

Which basically means that we had no or weak synchronization between threads using the same variables in the rwlock structure. There should not be a significant performance drop because the critical sections were already protected by:

  while(1) {
    if (relaxed_atomic_operation) {
      break;
    }
    LOCK(lock);
    if (!relaxed_atomic_operation) {
      WAIT(sem, lock);
    }
    UNLOCK(lock)l
  }

I would add one more thing to "Don't do your own crypto, folks.":

Also don't do your own locking, folks.

Closes #1428 (closed)

Edited by Ondřej Surý

Merge request reports