]> git.ipfire.org Git - thirdparty/bind9.git/commit
Convert all atomic operations in isc_rwlock to sequentially-consistent ordering
authorOndřej Surý <ondrej@isc.org>
Sat, 1 Feb 2020 09:48:20 +0000 (10:48 +0100)
committerOndřej Surý <ondrej@sury.org>
Tue, 11 Feb 2020 20:20:14 +0000 (21:20 +0100)
commitf71a8d1120d1d7a44843f379c7cf4a696573b2bd
treec4ff06966c84680bb5560d0e3359e2c0c02eece4
parent542517b194de11cdd1cf88e6056180a196a2a7bc
Convert all atomic operations in isc_rwlock to sequentially-consistent ordering

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.

  Sequentially-consistent ordering

  Atomic operations tagged memory_order_seq_cst not only order memory
  the same way as release/acquire ordering (everything that
  happened-before a store in one thread becomes a visible side effect in
  the thread that did a load), but also establish a single total
  modification order of all atomic operations that are so tagged.

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.

As part of this commit, I have also cleaned up the #ifdef spaghetti,
and fixed the isc_atomic API usage.
lib/isc/include/isc/rwlock.h
lib/isc/rwlock.c