Skip to content
  • Ondřej Surý's avatar
    Convert all atomic operations in isc_rwlock to release-acquire memory ordering · b43f5e02
    Ondřej Surý authored
    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.
    b43f5e02