Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
BIND
BIND
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 580
    • Issues 580
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
  • Merge Requests 113
    • Merge Requests 113
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
  • Operations
    • Operations
    • Incidents
    • Environments
  • Packages & Registries
    • Packages & Registries
    • Container Registry
  • Analytics
    • Analytics
    • CI / CD
    • Repository
    • Value Stream
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • ISC Open Source Projects
  • BINDBIND
  • Merge Requests
  • !2985

Merged
Opened Feb 01, 2020 by Ondřej Surý@ondrejOwner

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

  • Overview 1
  • Commits 1
  • Pipelines 4
  • Changes 1

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 Feb 11, 2020 by Ondřej Surý
Assignee
Assign to
Reviewer
Request review from
February 2020 (9.11.16, 9.14.11, 9.16.0, 9.16.0-S)
Milestone
February 2020 (9.11.16, 9.14.11, 9.16.0, 9.16.0-S) (Past due)
Assign milestone
Time tracking
Reference: isc-projects/bind9!2985
Source branch: 1428-possible-data-race-in-rbtdb-happens-occasionally-on-ppc64le

Revert this merge request

This will create a new commit in order to revert the existing changes.

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.

Cherry-pick this merge request

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.