Skip to content

Commit f366e21

Browse files
mpagempage
authored
gh-114271: Make thread._rlock thread-safe in free-threaded builds (#115102)
The ID of the owning thread (`rlock_owner`) may be accessed by multiple threads without holding the underlying lock; relaxed atomics are used in place of the previous loads/stores. The number of times that the lock has been acquired (`rlock_count`) is only ever accessed by the thread that holds the lock; we do not need to use atomics to access it.
1 parent 13addd2 commit f366e21

File tree

5 files changed

+68
-10
lines changed

5 files changed

+68
-10
lines changed

‎Include/cpython/pyatomic.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,8 @@ _Py_atomic_load_ssize_relaxed(const Py_ssize_t *obj);
360360
static inline void *
361361
_Py_atomic_load_ptr_relaxed(const void *obj);
362362

363+
static inline unsigned long long
364+
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj);
363365

364366
// --- _Py_atomic_store ------------------------------------------------------
365367
// Atomically performs `*obj = value` (sequential consistency)
@@ -452,6 +454,10 @@ _Py_atomic_store_ptr_relaxed(void *obj, void *value);
452454
static inline void
453455
_Py_atomic_store_ssize_relaxed(Py_ssize_t *obj, Py_ssize_t value);
454456

457+
static inline void
458+
_Py_atomic_store_ullong_relaxed(unsigned long long *obj,
459+
unsigned long long value);
460+
455461

456462
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------
457463

‎Include/cpython/pyatomic_gcc.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,10 @@ static inline void *
358358
_Py_atomic_load_ptr_relaxed(const void *obj)
359359
{ return (void *)__atomic_load_n((const void **)obj, __ATOMIC_RELAXED); }
360360

361+
static inline unsigned long long
362+
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj)
363+
{ return __atomic_load_n(obj, __ATOMIC_RELAXED); }
364+
361365

362366
// --- _Py_atomic_store ------------------------------------------------------
363367

@@ -476,6 +480,11 @@ static inline void
476480
_Py_atomic_store_ssize_relaxed(Py_ssize_t *obj, Py_ssize_t value)
477481
{ __atomic_store_n(obj, value, __ATOMIC_RELAXED); }
478482

483+
static inline void
484+
_Py_atomic_store_ullong_relaxed(unsigned long long *obj,
485+
unsigned long long value)
486+
{ __atomic_store_n(obj, value, __ATOMIC_RELAXED); }
487+
479488

480489
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------
481490

‎Include/cpython/pyatomic_msc.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,12 @@ _Py_atomic_load_ptr_relaxed(const void *obj)
712712
return *(void * volatile *)obj;
713713
}
714714

715+
static inline unsigned long long
716+
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj)
717+
{
718+
return *(volatile unsigned long long *)obj;
719+
}
720+
715721

716722
// --- _Py_atomic_store ------------------------------------------------------
717723

@@ -886,6 +892,14 @@ _Py_atomic_store_ssize_relaxed(Py_ssize_t *obj, Py_ssize_t value)
886892
*(volatile Py_ssize_t *)obj = value;
887893
}
888894

895+
static inline void
896+
_Py_atomic_store_ullong_relaxed(unsigned long long *obj,
897+
unsigned long long value)
898+
{
899+
*(volatile unsigned long long *)obj = value;
900+
}
901+
902+
889903
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------
890904

891905
static inline void *

‎Include/cpython/pyatomic_std.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,14 @@ _Py_atomic_load_ptr_relaxed(const void *obj)
619619
memory_order_relaxed);
620620
}
621621

622+
static inline unsigned long long
623+
_Py_atomic_load_ullong_relaxed(const unsigned long long *obj)
624+
{
625+
_Py_USING_STD;
626+
return atomic_load_explicit((const _Atomic(unsigned long long)*)obj,
627+
memory_order_relaxed);
628+
}
629+
622630

623631
// --- _Py_atomic_store ------------------------------------------------------
624632

@@ -835,6 +843,15 @@ _Py_atomic_store_ssize_relaxed(Py_ssize_t *obj, Py_ssize_t value)
835843
memory_order_relaxed);
836844
}
837845

846+
static inline void
847+
_Py_atomic_store_ullong_relaxed(unsigned long long *obj,
848+
unsigned long long value)
849+
{
850+
_Py_USING_STD;
851+
atomic_store_explicit((_Atomic(unsigned long long)*)obj, value,
852+
memory_order_relaxed);
853+
}
854+
838855

839856
// --- _Py_atomic_load_ptr_acquire / _Py_atomic_store_ptr_release ------------
840857

‎Modules/_threadmodule.c

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "pycore_sysmodule.h" // _PySys_GetAttr()
1212
#include "pycore_weakref.h" // _PyWeakref_GET_REF()
1313

14+
#include <stdbool.h>
1415
#include <stddef.h> // offsetof()
1516
#ifdef HAVE_SIGNAL_H
1617
# include <signal.h> // SIGINT
@@ -489,6 +490,14 @@ rlock_dealloc(rlockobject *self)
489490
Py_DECREF(tp);
490491
}
491492

493+
static bool
494+
rlock_is_owned_by(rlockobject *self, PyThread_ident_t tid)
495+
{
496+
PyThread_ident_t owner_tid =
497+
_Py_atomic_load_ullong_relaxed(&self->rlock_owner);
498+
return owner_tid == tid && self->rlock_count > 0;
499+
}
500+
492501
static PyObject *
493502
rlock_acquire(rlockobject *self, PyObject *args, PyObject *kwds)
494503
{
@@ -500,7 +509,7 @@ rlock_acquire(rlockobject *self, PyObject *args, PyObject *kwds)
500509
return NULL;
501510

502511
tid = PyThread_get_thread_ident_ex();
503-
if (self->rlock_count > 0 && tid == self->rlock_owner) {
512+
if (rlock_is_owned_by(self, tid)) {
504513
unsigned long count = self->rlock_count + 1;
505514
if (count <= self->rlock_count) {
506515
PyErr_SetString(PyExc_OverflowError,
@@ -513,7 +522,7 @@ rlock_acquire(rlockobject *self, PyObject *args, PyObject *kwds)
513522
r = acquire_timed(self->rlock_lock, timeout);
514523
if (r == PY_LOCK_ACQUIRED) {
515524
assert(self->rlock_count == 0);
516-
self->rlock_owner = tid;
525+
_Py_atomic_store_ullong_relaxed(&self->rlock_owner, tid);
517526
self->rlock_count = 1;
518527
}
519528
else if (r == PY_LOCK_INTR) {
@@ -544,13 +553,13 @@ rlock_release(rlockobject *self, PyObject *Py_UNUSED(ignored))
544553
{
545554
PyThread_ident_t tid = PyThread_get_thread_ident_ex();
546555

547-
if (self->rlock_count == 0 || self->rlock_owner != tid) {
556+
if (!rlock_is_owned_by(self, tid)) {
548557
PyErr_SetString(PyExc_RuntimeError,
549558
"cannot release un-acquired lock");
550559
return NULL;
551560
}
552561
if (--self->rlock_count == 0) {
553-
self->rlock_owner = 0;
562+
_Py_atomic_store_ullong_relaxed(&self->rlock_owner, 0);
554563
PyThread_release_lock(self->rlock_lock);
555564
}
556565
Py_RETURN_NONE;
@@ -589,7 +598,7 @@ rlock_acquire_restore(rlockobject *self, PyObject *args)
589598
return NULL;
590599
}
591600
assert(self->rlock_count == 0);
592-
self->rlock_owner = owner;
601+
_Py_atomic_store_ullong_relaxed(&self->rlock_owner, owner);
593602
self->rlock_count = count;
594603
Py_RETURN_NONE;
595604
}
@@ -614,7 +623,7 @@ rlock_release_save(rlockobject *self, PyObject *Py_UNUSED(ignored))
614623
owner = self->rlock_owner;
615624
count = self->rlock_count;
616625
self->rlock_count = 0;
617-
self->rlock_owner = 0;
626+
_Py_atomic_store_ullong_relaxed(&self->rlock_owner, 0);
618627
PyThread_release_lock(self->rlock_lock);
619628
return Py_BuildValue("k" Py_PARSE_THREAD_IDENT_T, count, owner);
620629
}
@@ -628,8 +637,9 @@ static PyObject *
628637
rlock_recursion_count(rlockobject *self, PyObject *Py_UNUSED(ignored))
629638
{
630639
PyThread_ident_t tid = PyThread_get_thread_ident_ex();
631-
return PyLong_FromUnsignedLong(
632-
self->rlock_owner == tid ? self->rlock_count : 0UL);
640+
PyThread_ident_t owner =
641+
_Py_atomic_load_ullong_relaxed(&self->rlock_owner);
642+
return PyLong_FromUnsignedLong(owner == tid ? self->rlock_count : 0UL);
633643
}
634644

635645
PyDoc_STRVAR(rlock_recursion_count_doc,
@@ -642,7 +652,7 @@ rlock_is_owned(rlockobject *self, PyObject *Py_UNUSED(ignored))
642652
{
643653
PyThread_ident_t tid = PyThread_get_thread_ident_ex();
644654

645-
if (self->rlock_count > 0 && self->rlock_owner == tid) {
655+
if (rlock_is_owned_by(self, tid)) {
646656
Py_RETURN_TRUE;
647657
}
648658
Py_RETURN_FALSE;
@@ -676,10 +686,12 @@ rlock_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
676686
static PyObject *
677687
rlock_repr(rlockobject *self)
678688
{
689+
PyThread_ident_t owner =
690+
_Py_atomic_load_ullong_relaxed(&self->rlock_owner);
679691
return PyUnicode_FromFormat(
680692
"<%s %s object owner=%" PY_FORMAT_THREAD_IDENT_T " count=%lu at %p>",
681693
self->rlock_count ? "locked" : "unlocked",
682-
Py_TYPE(self)->tp_name, self->rlock_owner,
694+
Py_TYPE(self)->tp_name, owner,
683695
self->rlock_count, self);
684696
}
685697

0 commit comments

Comments
 (0)