Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-117511: Make PyMutex public in the non-limited API #117731

Merged
merged 14 commits into from
Jun 20, 2024
Prev Previous commit
Next Next commit
Use underscore prefix for PyMutex field.
Update code comments to further clarify that the `_bits` field is not
part of the public C API.
  • Loading branch information
colesbury committed Apr 11, 2024
commit 6533089b2539b6b90b9ce91e084fd80a7567a90f
24 changes: 13 additions & 11 deletions Include/cpython/lock.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,8 @@
#define _Py_UNLOCKED 0
#define _Py_LOCKED 1

// A mutex that occupies one byte. The lock can be zero initialized.
//
// Only the two least significant bits are used. The remaining bits should be
// zero:
// 0b00: unlocked
// 0b01: locked
// 0b10: unlocked and has parked threads
// 0b11: locked and has parked threads
// A mutex that occupies one byte. The lock can be zero initialized to
// represent the unlocked state.
//
// Typical initialization:
// PyMutex m = (PyMutex){0};
Expand All @@ -24,8 +18,16 @@
// PyMutex_Lock(&m);
// ...
// PyMutex_Unlock(&m);
//
// The contents of the PyMutex are not part of the public API, but are
// described to aid in understanding the implementation and debugging. Only
// the least significant bits are used. The remaining bits are always zero:
// 0b00: unlocked
// 0b01: locked
// 0b10: unlocked and has parked threads
// 0b11: locked and has parked threads
typedef struct PyMutex {
uint8_t v;
uint8_t _bits; // (private)
} PyMutex;

// (private) slow path for locking the mutex
Expand All @@ -43,7 +45,7 @@ static inline void
PyMutex_Lock(PyMutex *m)
{
uint8_t expected = _Py_UNLOCKED;
if (!_Py_atomic_compare_exchange_uint8(&m->v, &expected, _Py_LOCKED)) {
if (!_Py_atomic_compare_exchange_uint8(&m->_bits, &expected, _Py_LOCKED)) {
_PyMutex_LockSlow(m);
}
}
Expand All @@ -53,7 +55,7 @@ static inline void
PyMutex_Unlock(PyMutex *m)
{
uint8_t expected = _Py_LOCKED;
if (!_Py_atomic_compare_exchange_uint8(&m->v, &expected, _Py_UNLOCKED)) {
if (!_Py_atomic_compare_exchange_uint8(&m->_bits, &expected, _Py_UNLOCKED)) {
_PyMutex_UnlockSlow(m);
}
}
6 changes: 3 additions & 3 deletions Include/internal/pycore_critical_section.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ _PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2,
static inline void
_PyCriticalSection_Begin(_PyCriticalSection *c, PyMutex *m)
{
if (PyMutex_LockFast(&m->v)) {
if (PyMutex_LockFast(&m->_bits)) {
PyThreadState *tstate = _PyThreadState_GET();
c->mutex = m;
c->prev = tstate->critical_section;
Expand Down Expand Up @@ -262,8 +262,8 @@ _PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2)
m2 = tmp;
}

if (PyMutex_LockFast(&m1->v)) {
if (PyMutex_LockFast(&m2->v)) {
if (PyMutex_LockFast(&m1->_bits)) {
if (PyMutex_LockFast(&m2->_bits)) {
PyThreadState *tstate = _PyThreadState_GET();
c->base.mutex = m1;
c->mutex2 = m2;
Expand Down
4 changes: 2 additions & 2 deletions Include/internal/pycore_lock.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ PyMutex_LockFast(uint8_t *lock_bits)
static inline int
PyMutex_IsLocked(PyMutex *m)
{
return (_Py_atomic_load_uint8(&m->v) & _Py_LOCKED) != 0;
return (_Py_atomic_load_uint8(&m->_bits) & _Py_LOCKED) != 0;
}

// Re-initializes the mutex after a fork to the unlocked state.
Expand Down Expand Up @@ -59,7 +59,7 @@ static inline void
PyMutex_LockFlags(PyMutex *m, _PyLockFlags flags)
{
uint8_t expected = _Py_UNLOCKED;
if (!_Py_atomic_compare_exchange_uint8(&m->v, &expected, _Py_LOCKED)) {
if (!_Py_atomic_compare_exchange_uint8(&m->_bits, &expected, _Py_LOCKED)) {
_PyMutex_LockTimed(m, -1, flags);
}
}
Expand Down
16 changes: 8 additions & 8 deletions Modules/_testinternalcapi/test_lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ test_lock_basic(PyObject *self, PyObject *obj)

// uncontended lock and unlock
PyMutex_Lock(&m);
assert(m.v == 1);
assert(m._bits == 1);
PyMutex_Unlock(&m);
assert(m.v == 0);
assert(m._bits == 0);

Py_RETURN_NONE;
}
Expand All @@ -57,10 +57,10 @@ lock_thread(void *arg)
_Py_atomic_store_int(&test_data->started, 1);

PyMutex_Lock(m);
assert(m->v == 1);
assert(m->_bits == 1);

PyMutex_Unlock(m);
assert(m->v == 0);
assert(m->_bits == 0);

_PyEvent_Notify(&test_data->done);
}
Expand All @@ -73,7 +73,7 @@ test_lock_two_threads(PyObject *self, PyObject *obj)
memset(&test_data, 0, sizeof(test_data));

PyMutex_Lock(&test_data.m);
assert(test_data.m.v == 1);
assert(test_data.m._bits == 1);

PyThread_start_new_thread(lock_thread, &test_data);

Expand All @@ -82,17 +82,17 @@ test_lock_two_threads(PyObject *self, PyObject *obj)
uint8_t v;
do {
pysleep(10); // allow some time for the other thread to try to lock
v = _Py_atomic_load_uint8_relaxed(&test_data.m.v);
v = _Py_atomic_load_uint8_relaxed(&test_data.m._bits);
assert(v == 1 || v == 3);
iters++;
} while (v != 3 && iters < 200);

// both the "locked" and the "has parked" bits should be set
assert(test_data.m.v == 3);
assert(test_data.m._bits == 3);

PyMutex_Unlock(&test_data.m);
PyEvent_Wait(&test_data.done);
assert(test_data.m.v == 0);
assert(test_data.m._bits == 0);

Py_RETURN_NONE;
}
Expand Down
22 changes: 11 additions & 11 deletions Python/lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ _PyMutex_LockSlow(PyMutex *m)
PyLockStatus
_PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags)
{
uint8_t v = _Py_atomic_load_uint8_relaxed(&m->v);
uint8_t v = _Py_atomic_load_uint8_relaxed(&m->_bits);
if ((v & _Py_LOCKED) == 0) {
if (_Py_atomic_compare_exchange_uint8(&m->v, &v, v|_Py_LOCKED)) {
if (_Py_atomic_compare_exchange_uint8(&m->_bits, &v, v|_Py_LOCKED)) {
return PY_LOCK_ACQUIRED;
}
}
Expand All @@ -81,7 +81,7 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags)
for (;;) {
if ((v & _Py_LOCKED) == 0) {
// The lock is unlocked. Try to grab it.
if (_Py_atomic_compare_exchange_uint8(&m->v, &v, v|_Py_LOCKED)) {
if (_Py_atomic_compare_exchange_uint8(&m->_bits, &v, v|_Py_LOCKED)) {
return PY_LOCK_ACQUIRED;
}
continue;
Expand All @@ -102,17 +102,17 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags)
if (!(v & _Py_HAS_PARKED)) {
// We are the first waiter. Set the _Py_HAS_PARKED flag.
newv = v | _Py_HAS_PARKED;
if (!_Py_atomic_compare_exchange_uint8(&m->v, &v, newv)) {
if (!_Py_atomic_compare_exchange_uint8(&m->_bits, &v, newv)) {
continue;
}
}

int ret = _PyParkingLot_Park(&m->v, &newv, sizeof(newv), timeout,
int ret = _PyParkingLot_Park(&m->_bits, &newv, sizeof(newv), timeout,
&entry, (flags & _PY_LOCK_DETACH) != 0);
if (ret == Py_PARK_OK) {
if (entry.handed_off) {
// We own the lock now.
assert(_Py_atomic_load_uint8_relaxed(&m->v) & _Py_LOCKED);
assert(_Py_atomic_load_uint8_relaxed(&m->_bits) & _Py_LOCKED);
return PY_LOCK_ACQUIRED;
}
}
Expand All @@ -134,7 +134,7 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags)
}
}

v = _Py_atomic_load_uint8_relaxed(&m->v);
v = _Py_atomic_load_uint8_relaxed(&m->_bits);
}
}

Expand All @@ -154,24 +154,24 @@ mutex_unpark(PyMutex *m, struct mutex_entry *entry, int has_more_waiters)
v |= _Py_HAS_PARKED;
}
}
_Py_atomic_store_uint8(&m->v, v);
_Py_atomic_store_uint8(&m->_bits, v);
}

int
_PyMutex_TryUnlock(PyMutex *m)
{
uint8_t v = _Py_atomic_load_uint8(&m->v);
uint8_t v = _Py_atomic_load_uint8(&m->_bits);
for (;;) {
if ((v & _Py_LOCKED) == 0) {
// error: the mutex is not locked
return -1;
}
else if ((v & _Py_HAS_PARKED)) {
// wake up a single thread
_PyParkingLot_Unpark(&m->v, (_Py_unpark_fn_t *)mutex_unpark, m);
_PyParkingLot_Unpark(&m->_bits, (_Py_unpark_fn_t *)mutex_unpark, m);
return 0;
}
else if (_Py_atomic_compare_exchange_uint8(&m->v, &v, _Py_UNLOCKED)) {
else if (_Py_atomic_compare_exchange_uint8(&m->_bits, &v, _Py_UNLOCKED)) {
// fast-path: no waiters
return 0;
}
Expand Down
Loading