From 0fa9fa309a6c008525d093adbd9e557a9f95f143 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 2 May 2024 19:00:38 +0000 Subject: [PATCH 1/4] gh-117657: Use PyMutex based recursive lock for import lock This adds a `_PyRecursiveMutex` type based on `PyMutex` and uses that for the import lock. This fixes some data races in the free-threaded build and generally simplifies the import lock code. --- Include/internal/pycore_import.h | 18 +---- Include/internal/pycore_lock.h | 12 ++++ Modules/_testinternalcapi/test_lock.c | 25 +++++++ Modules/posixmodule.c | 11 +-- Python/import.c | 83 ++-------------------- Python/lock.c | 42 +++++++++++ Tools/tsan/suppressions_free_threading.txt | 2 - 7 files changed, 90 insertions(+), 103 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index b02769903a6f9b..3ac62c282cd07c 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -20,7 +20,7 @@ PyAPI_FUNC(int) _PyImport_SetModule(PyObject *name, PyObject *module); extern int _PyImport_SetModuleString(const char *name, PyObject* module); extern void _PyImport_AcquireLock(PyInterpreterState *interp); -extern int _PyImport_ReleaseLock(PyInterpreterState *interp); +extern void _PyImport_ReleaseLock(PyInterpreterState *interp); // This is used exclusively for the sys and builtins modules: extern int _PyImport_FixupBuiltin( @@ -94,11 +94,7 @@ struct _import_state { #endif PyObject *import_func; /* The global import lock. */ - struct { - PyThread_type_lock mutex; - unsigned long thread; - int level; - } lock; + _PyRecursiveMutex lock; /* diagnostic info in PyImport_ImportModuleLevelObject() */ struct { int import_level; @@ -123,11 +119,6 @@ struct _import_state { #define IMPORTS_INIT \ { \ DLOPENFLAGS_INIT \ - .lock = { \ - .mutex = NULL, \ - .thread = PYTHREAD_INVALID_THREAD_ID, \ - .level = 0, \ - }, \ .find_and_load = { \ .header = 1, \ }, \ @@ -180,11 +171,6 @@ extern void _PyImport_FiniCore(PyInterpreterState *interp); extern void _PyImport_FiniExternal(PyInterpreterState *interp); -#ifdef HAVE_FORK -extern PyStatus _PyImport_ReInitLock(PyInterpreterState *interp); -#endif - - extern PyObject* _PyImport_GetBuiltinModuleNames(void); struct _module_alias { diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index a5b28e4bd4744e..d5853b2c9ff464 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -219,6 +219,18 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) return _PyOnceFlag_CallOnceSlow(flag, fn, arg); } +// A recursive mutex. The mutex should zero-initialized. +typedef struct { + PyMutex mutex; + unsigned long long thread; // i.e., PyThread_get_thread_ident_ex() + size_t level; +} _PyRecursiveMutex; + +PyAPI_FUNC(int) _PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m); +PyAPI_FUNC(void) _PyRecursiveMutex_Lock(_PyRecursiveMutex *m); +PyAPI_FUNC(void) _PyRecursiveMutex_Unlock(_PyRecursiveMutex *m); + + // A readers-writer (RW) lock. The lock supports multiple concurrent readers or // a single writer. The lock is write-preferring: if a writer is waiting while // the lock is read-locked then, new readers will be blocked. This avoids diff --git a/Modules/_testinternalcapi/test_lock.c b/Modules/_testinternalcapi/test_lock.c index 1c5048170e9f2e..3c9c577f690075 100644 --- a/Modules/_testinternalcapi/test_lock.c +++ b/Modules/_testinternalcapi/test_lock.c @@ -2,6 +2,7 @@ #include "parts.h" #include "pycore_lock.h" +#include "pycore_pythread.h" // PyThread_get_thread_ident_ex() #include "pycore_time.h" // _PyTime_MonotonicUnchecked() #include "clinic/test_lock.c.h" @@ -471,6 +472,29 @@ test_lock_rwlock(PyObject *self, PyObject *obj) Py_RETURN_NONE; } +static PyObject * +test_lock_recursive(PyObject *self, PyObject *obj) +{ + _PyRecursiveMutex m = (_PyRecursiveMutex){0}; + assert(!_PyRecursiveMutex_IsLockedByCurrentThread(&m)); + + _PyRecursiveMutex_Lock(&m); + assert(m.thread == PyThread_get_thread_ident_ex()); + assert(PyMutex_IsLocked(&m.mutex)); + assert(m.level == 0); + + _PyRecursiveMutex_Lock(&m); + assert(m.level == 1); + _PyRecursiveMutex_Unlock(&m); + + _PyRecursiveMutex_Unlock(&m); + assert(m.thread == 0); + assert(!PyMutex_IsLocked(&m.mutex)); + assert(m.level == 0); + + Py_RETURN_NONE; +} + static PyMethodDef test_methods[] = { {"test_lock_basic", test_lock_basic, METH_NOARGS}, {"test_lock_two_threads", test_lock_two_threads, METH_NOARGS}, @@ -480,6 +504,7 @@ static PyMethodDef test_methods[] = { {"test_lock_benchmark", test_lock_benchmark, METH_NOARGS}, {"test_lock_once", test_lock_once, METH_NOARGS}, {"test_lock_rwlock", test_lock_rwlock, METH_NOARGS}, + {"test_lock_recursive", test_lock_recursive, METH_NOARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index f9533577a8fa34..5fef9ffbe9df66 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -16,7 +16,6 @@ #include "pycore_call.h" // _PyObject_CallNoArgs() #include "pycore_ceval.h" // _PyEval_ReInitThreads() #include "pycore_fileutils.h" // _Py_closerange() -#include "pycore_import.h" // _PyImport_ReInitLock() #include "pycore_initconfig.h" // _PyStatus_EXCEPTION() #include "pycore_moduleobject.h" // _PyModule_GetState() #include "pycore_object.h" // _PyObject_LookupSpecial() @@ -626,10 +625,7 @@ PyOS_AfterFork_Parent(void) _PyEval_StartTheWorldAll(&_PyRuntime); PyInterpreterState *interp = _PyInterpreterState_GET(); - if (_PyImport_ReleaseLock(interp) <= 0) { - Py_FatalError("failed releasing import lock after fork"); - } - + _PyImport_ReleaseLock(interp); run_at_forkers(interp->after_forkers_parent, 0); } @@ -674,10 +670,7 @@ PyOS_AfterFork_Child(void) _PyEval_StartTheWorldAll(&_PyRuntime); _PyThreadState_DeleteList(list); - status = _PyImport_ReInitLock(tstate->interp); - if (_PyStatus_EXCEPTION(status)) { - goto fatal_error; - } + _PyImport_ReleaseLock(tstate->interp); _PySignal_AfterFork(); diff --git a/Python/import.c b/Python/import.c index 0c51ffc6285a9c..2fe8386d2c59ee 100644 --- a/Python/import.c +++ b/Python/import.c @@ -82,11 +82,7 @@ static struct _inittab *inittab_copy = NULL; (interp)->imports.import_func #define IMPORT_LOCK(interp) \ - (interp)->imports.lock.mutex -#define IMPORT_LOCK_THREAD(interp) \ - (interp)->imports.lock.thread -#define IMPORT_LOCK_LEVEL(interp) \ - (interp)->imports.lock.level + (interp)->imports.lock #define FIND_AND_LOAD(interp) \ (interp)->imports.find_and_load @@ -103,74 +99,14 @@ static struct _inittab *inittab_copy = NULL; void _PyImport_AcquireLock(PyInterpreterState *interp) { - unsigned long me = PyThread_get_thread_ident(); - if (me == PYTHREAD_INVALID_THREAD_ID) - return; /* Too bad */ - if (IMPORT_LOCK(interp) == NULL) { - IMPORT_LOCK(interp) = PyThread_allocate_lock(); - if (IMPORT_LOCK(interp) == NULL) - return; /* Nothing much we can do. */ - } - if (IMPORT_LOCK_THREAD(interp) == me) { - IMPORT_LOCK_LEVEL(interp)++; - return; - } - if (IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID || - !PyThread_acquire_lock(IMPORT_LOCK(interp), 0)) - { - PyThreadState *tstate = PyEval_SaveThread(); - PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK); - PyEval_RestoreThread(tstate); - } - assert(IMPORT_LOCK_LEVEL(interp) == 0); - IMPORT_LOCK_THREAD(interp) = me; - IMPORT_LOCK_LEVEL(interp) = 1; + _PyRecursiveMutex_Lock(&IMPORT_LOCK(interp)); } -int +void _PyImport_ReleaseLock(PyInterpreterState *interp) { - unsigned long me = PyThread_get_thread_ident(); - if (me == PYTHREAD_INVALID_THREAD_ID || IMPORT_LOCK(interp) == NULL) - return 0; /* Too bad */ - if (IMPORT_LOCK_THREAD(interp) != me) - return -1; - IMPORT_LOCK_LEVEL(interp)--; - assert(IMPORT_LOCK_LEVEL(interp) >= 0); - if (IMPORT_LOCK_LEVEL(interp) == 0) { - IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID; - PyThread_release_lock(IMPORT_LOCK(interp)); - } - return 1; -} - -#ifdef HAVE_FORK -/* This function is called from PyOS_AfterFork_Child() to ensure that newly - created child processes do not share locks with the parent. - We now acquire the import lock around fork() calls but on some platforms - (Solaris 9 and earlier? see isue7242) that still left us with problems. */ -PyStatus -_PyImport_ReInitLock(PyInterpreterState *interp) -{ - if (IMPORT_LOCK(interp) != NULL) { - if (_PyThread_at_fork_reinit(&IMPORT_LOCK(interp)) < 0) { - return _PyStatus_ERR("failed to create a new lock"); - } - } - - if (IMPORT_LOCK_LEVEL(interp) > 1) { - /* Forked as a side effect of import */ - unsigned long me = PyThread_get_thread_ident(); - PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK); - IMPORT_LOCK_THREAD(interp) = me; - IMPORT_LOCK_LEVEL(interp)--; - } else { - IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID; - IMPORT_LOCK_LEVEL(interp) = 0; - } - return _PyStatus_OK(); + _PyRecursiveMutex_Unlock(&IMPORT_LOCK(interp)); } -#endif /***************/ @@ -3446,11 +3382,6 @@ _PyImport_FiniCore(PyInterpreterState *interp) PyErr_FormatUnraisable("Exception ignored on clearing sys.modules"); } - if (IMPORT_LOCK(interp) != NULL) { - PyThread_free_lock(IMPORT_LOCK(interp)); - IMPORT_LOCK(interp) = NULL; - } - _PyImport_ClearCore(interp); } @@ -3583,8 +3514,7 @@ _imp_lock_held_impl(PyObject *module) /*[clinic end generated code: output=8b89384b5e1963fc input=9b088f9b217d9bdf]*/ { PyInterpreterState *interp = _PyInterpreterState_GET(); - return PyBool_FromLong( - IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID); + return PyBool_FromLong(PyMutex_IsLocked(&IMPORT_LOCK(interp).mutex)); } /*[clinic input] @@ -3618,11 +3548,12 @@ _imp_release_lock_impl(PyObject *module) /*[clinic end generated code: output=7faab6d0be178b0a input=934fb11516dd778b]*/ { PyInterpreterState *interp = _PyInterpreterState_GET(); - if (_PyImport_ReleaseLock(interp) < 0) { + if (!_PyRecursiveMutex_IsLockedByCurrentThread(&IMPORT_LOCK(interp))) { PyErr_SetString(PyExc_RuntimeError, "not holding the import lock"); return NULL; } + _PyImport_ReleaseLock(interp); Py_RETURN_NONE; } diff --git a/Python/lock.c b/Python/lock.c index 5ed95fcaf4188c..42f68c2eed54c9 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -362,6 +362,48 @@ _PyOnceFlag_CallOnceSlow(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) } } +static int +recursive_mutex_is_owned_by(_PyRecursiveMutex *m, PyThread_ident_t tid) +{ + return _Py_atomic_load_ullong_relaxed(&m->thread) == tid; +} + +int +_PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m) +{ + return recursive_mutex_is_owned_by(m, PyThread_get_thread_ident_ex()); +} + +void +_PyRecursiveMutex_Lock(_PyRecursiveMutex *m) +{ + PyThread_ident_t thread = PyThread_get_thread_ident_ex(); + if (recursive_mutex_is_owned_by(m, thread)) { + m->level++; + return; + } + PyMutex_Lock(&m->mutex); + _Py_atomic_store_ullong_relaxed(&m->thread, thread); + assert(m->level == 0); +} + +void +_PyRecursiveMutex_Unlock(_PyRecursiveMutex *m) +{ + PyThread_ident_t thread = PyThread_get_thread_ident_ex(); + if (!recursive_mutex_is_owned_by(m, thread)) { + Py_FatalError("unlocking a recursive mutex that is not owned by the" + " current thread"); + } + if (m->level > 0) { + m->level--; + return; + } + assert(m->level == 0); + _Py_atomic_store_ullong_relaxed(&m->thread, 0); + PyMutex_Unlock(&m->mutex); +} + #define _Py_WRITE_LOCKED 1 #define _PyRWMutex_READER_SHIFT 2 #define _Py_RWMUTEX_MAX_READERS (UINTPTR_MAX >> _PyRWMutex_READER_SHIFT) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 4f6648a7573184..a2244f461e7b18 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -16,8 +16,6 @@ race:_in_weak_set race:_mi_heap_delayed_free_partial race:_PyEval_EvalFrameDefault race:_PyFunction_SetVersion -race:_PyImport_AcquireLock -race:_PyImport_ReleaseLock race:_PyInterpreterState_SetNotRunningMain race:_PyInterpreterState_IsRunningMain race:_PyObject_GC_IS_SHARED From 72f4797406f9c83856ee65cb7a11d85913ae4553 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 31 May 2024 18:51:53 +0000 Subject: [PATCH 2/4] Remove fixed races --- Tools/tsan/suppressions_free_threading.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index cda57d78067bb3..cd5604aff102a0 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -31,8 +31,6 @@ race_top:_add_to_weak_set race_top:_in_weak_set race_top:_mi_heap_delayed_free_partial race_top:_PyEval_EvalFrameDefault -race_top:_PyImport_AcquireLock -race_top:_PyImport_ReleaseLock # https://gist.github.com/mpage/0a24eb2dd458441ededb498e9b0e5de8 race_top:_PyParkingLot_Park race_top:_PyType_HasFeature From 42eaae0711f9e7d646ce721c68edbae21f5cf154 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 31 May 2024 19:13:56 +0000 Subject: [PATCH 3/4] Remove fixed race --- Tools/tsan/suppressions_free_threading.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index cd5604aff102a0..a1892a752d54be 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -51,7 +51,6 @@ race_top:Py_SET_TYPE race_top:_PyDict_CheckConsistency race_top:_PyImport_AcquireLock race_top:_Py_dict_lookup_threadsafe -race_top:_imp_release_lock race_top:_multiprocessing_SemLock_acquire_impl race_top:builtin_compile_impl race_top:dictiter_new From 7191cda5536fd0fb62add26c9a6c46de0acbf2c3 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 31 May 2024 19:19:09 +0000 Subject: [PATCH 4/4] Remove another fixed race --- Tools/tsan/suppressions_free_threading.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index a1892a752d54be..c88944f0365cb1 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -49,7 +49,6 @@ race_top:tstate_set_detached race_top:unicode_hash race_top:Py_SET_TYPE race_top:_PyDict_CheckConsistency -race_top:_PyImport_AcquireLock race_top:_Py_dict_lookup_threadsafe race_top:_multiprocessing_SemLock_acquire_impl race_top:builtin_compile_impl