microsoft / microsoft/STL

<atomic>, <memory>, <execution>: make sure acquire and release are safe to use and start using them

Open
#1,133 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

performance
Dominant language
C++
Stars
11.1k
Forks
1.7k
Avg merge
4d 15h
Merged PRs (30d)
22

Description

Currently memory_order_acquire and memory_order_release are considered unsafe:
The problem is critical sections overlap in the following situation with mutexes or other synch object:

T1: a.acquire(); a.release(); b.acquire(); b.release(), 
T2: b.acquire(); b.release(); a.acquire(); a.release();

Release reorders past subsequent unrelated acquire, so sections overlap and deadlock occurs.
The current resolution is believed to be the following:

  1. Acquire should observe the release result in a finite time, so release operations cannot be reordered past infinite amount of acquire attempts
  2. In hardware, memory changes take time to propagate, but relatively a very small time, definitely not infinite time
  3. In software, the compiler either does not reorder operations at all, or does not reorder them past potentially infinite amount of other operations
Unfortunately, 1 is not what Standard currently says, and 2 and 3 has to be confirmed with compiler vendors

Before the status of acquire / release is clarified, currently seq_cst is used in some places, specifically:
  1. atomic_shared_ptr internal spinlock:
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/memory#L3130
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/memory#L3150
  2. Non-lock-free atomic
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/atomic#L394-L407
  3. Parallel algorithms in <execution> (more than just this occurrence):
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/execution#L3624
  4. memory_resource.cpp
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L24
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L33
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L43
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L53
  5. filesystem.cpp
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/filesystem.cpp#L36-L50

Some places believed to be not affected by the issue still use acquire / release, specifically:
  1. shared_ptr external spinlock:
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/atomic.cpp#L16-L34
  2. <system_error>
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/system_error#L590-L597
  3. atomic_wait.cpp
    https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/atomic_wait.cpp#L147-L152

The task is to confirm the situation with compiler team and decide on using memory_order_acquire / memory_order_release in mentioned and possibly unmentioned preexisting code and new code

Note also that memory model implementation on arm may change in the future, see #83 , see also ##488 , #775 , #1082

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the acquire/release uses in stl/inc/memory, stl/inc/atomic, stl/inc/execution, stl/inc/system_error, and the atomic.cpp, atomic_wait.cpp, memory_resource.cpp, and filesystem.cpp sources. Confirm the C++ memory-model and compiler behavior with the compiler team, then audit the listed and related synchronization sites. Done means the safety question is resolved and the appropriate memory orders are consistently chosen.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, operating-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.