From e32400384e13a2aca48b201f8b1edc6ed6fa1227 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Tue, 30 Apr 2024 14:22:12 -0700 Subject: [PATCH 1/7] Enable the GIL while loading C extension modules --- Include/internal/pycore_ceval.h | 43 +++++++- Include/internal/pycore_gil.h | 16 ++- Include/internal/pycore_import.h | 22 +++++ Python/ceval_gil.c | 164 ++++++++++++++++++++++++++++--- Python/import.c | 123 +++++++++++++++++++++++ Python/importdl.c | 9 ++ Python/pystate.c | 36 +++++-- 7 files changed, 389 insertions(+), 24 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index cfb88c3f4c8e15..400571f859f79f 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -130,9 +130,50 @@ extern int _PyEval_ThreadsInitialized(void); extern void _PyEval_InitGIL(PyThreadState *tstate, int own_gil); extern void _PyEval_FiniGIL(PyInterpreterState *interp); -extern void _PyEval_AcquireLock(PyThreadState *tstate); +// Acquire the GIL and return 1. In free-threaded builds, this function may +// return 0 to indicate that the GIL was disabled and therefore not acquired. +extern int _PyEval_AcquireLock(PyThreadState *tstate); + extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *); +#ifdef Py_GIL_DISABLED +// Returns 0 or 1 if the GIL for the given thread's interpreter is disabled or +// enabled, respectively. +// +// The enabled state of the GIL will not change while one or more threads are +// attached. +extern int _PyEval_IsGILEnabled(PyThreadState *tstate); + +// Enable or disable the GIL used by the interpreter that owns tstate, which +// must be the current thread. This may affect other interpreters, if the GIL +// is shared. All three functions will be no-ops (and return 0) if the +// interpreter's `enable_gil' config is not _PyConfig_GIL_DEFAULT. +// +// Every call to _PyEval_EnableGILTransient() must be paired with exactly one +// call to either _PyEval_EnableGILPermanent() or +// _PyEval_DisableGIL(). _PyEval_EnableGILPermanent() and _PyEval_DisableGIL() +// must only be called while the GIL is enabled from a call to +// _PyEval_EnableGILTransient(). +// +// _PyEval_EnableGILTransient() returns 1 if it enabled the GIL, or 0 if the +// GIL was already enabled, whether transiently or permanently. The caller will +// hold the GIL upon return. +// +// _PyEval_EnableGILPermanent() returns 1 if it permanently enabled the GIL +// (which must already be enabled), or 0 if it was already permanently +// enabled. Once _PyEval_EnableGILPermanent() has been called once, all +// subsequent calls to any of the three functions will be no-ops. +// +// _PyEval_DisableGIL() returns 1 if it disabled the GIL, or 0 if the GIL was +// kept enabled because of another request, whether transient or permanent. +// +// All three functions must be called by an attached thread (this implies that +// if the GIL is enabled, the current thread must hold it). +extern int _PyEval_EnableGILTransient(PyThreadState *tstate); +extern int _PyEval_EnableGILPermanent(PyThreadState *tstate); +extern int _PyEval_DisableGIL(PyThreadState *state); +#endif + extern void _PyEval_DeactivateOpCache(void); diff --git a/Include/internal/pycore_gil.h b/Include/internal/pycore_gil.h index d36b4c0db010b2..a2de5077371eba 100644 --- a/Include/internal/pycore_gil.h +++ b/Include/internal/pycore_gil.h @@ -21,8 +21,20 @@ extern "C" { struct _gil_runtime_state { #ifdef Py_GIL_DISABLED - /* Whether or not this GIL is being used. Can change from 0 to 1 at runtime - if, for example, a module that requires the GIL is loaded. */ + /* If this GIL is disabled, enabled == 0. + + If this GIL is enabled transiently (most likely to initialize a module + of unknown safety), enabled indicates the number of active transient + requests. + + If this GIL is enabled permanently, enabled == INT_MAX. + + It must not be modified directly; use _PyEval_EnableGILTransiently(), + _PyEval_EnableGILPermanently(), and _PyEval_DisableGIL() + + It is always read and written atomically, but a thread can assume its + value will be stable as long as that thread is attached or knows that no + other threads are attached (e.g., during a stop-the-world.). */ int enabled; #endif /* microseconds (the Python API uses seconds, though) */ diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index b02769903a6f9b..3a5f0529f30379 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -83,6 +83,12 @@ struct _import_state { This is initialized lazily in PyState_AddModule(), which is also where modules get added. */ PyObject *modules_by_index; +#ifdef Py_GIL_DISABLED + /* This list contains GIL slots (see Py_mod_gil) for modules that may be + reinitialized by reload_singlephase_extension(). It is indexed the same + way as modules_by_index. */ + PyObject *module_gil_by_index; +#endif /* importlib module._bootstrap */ PyObject *importlib; /* override for config->use_frozen_modules (for tests) @@ -206,6 +212,22 @@ extern int _PyImport_CheckSubinterpIncompatibleExtensionAllowed( // Export for '_testinternalcapi' shared extension PyAPI_FUNC(int) _PyImport_ClearExtension(PyObject *name, PyObject *filename); +#ifdef Py_GIL_DISABLED +extern int _PyImport_SetModuleGIL(PyObject *module, void *gil); +extern void *_PyImport_GetModuleDefGIL(PyModuleDef *def); + +// Assuming that the GIL is enabled from a call to +// _PyEval_EnableGILTransient(), resolve the transient request depending on the +// state of the module argument: +// - If module is NULL or a PyModuleObject with md_gil == Py_MOD_GIL_NOT_USED, +// call _PyEval_DisableGIL(). +// - Otherwise, call _PyEval_EnableGILPermanent(). If the GIL was not already +// enabled permanently, issue a warning referencing the module's name. +// +// This function may raise an exception. +extern int _PyImport_CheckGILForModule(PyObject *module, PyObject *module_name); +#endif + #ifdef __cplusplus } #endif diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index e4f7217255efd8..5ec92c2026eb22 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -205,6 +205,16 @@ static void recreate_gil(struct _gil_runtime_state *gil) } #endif +static void +drop_gil_impl(struct _gil_runtime_state *gil) +{ + MUTEX_LOCK(gil->mutex); + _Py_ANNOTATE_RWLOCK_RELEASED(&gil->locked, /*is_write=*/1); + _Py_atomic_store_int_relaxed(&gil->locked, 0); + COND_SIGNAL(gil->cond); + MUTEX_UNLOCK(gil->mutex); +} + static void drop_gil(PyInterpreterState *interp, PyThreadState *tstate) { @@ -220,7 +230,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate) struct _gil_runtime_state *gil = ceval->gil; #ifdef Py_GIL_DISABLED - if (!gil->enabled) { + if (!_Py_atomic_load_int_relaxed(&gil->enabled)) { return; } #endif @@ -236,11 +246,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate) _Py_atomic_store_ptr_relaxed(&gil->last_holder, tstate); } - MUTEX_LOCK(gil->mutex); - _Py_ANNOTATE_RWLOCK_RELEASED(&gil->locked, /*is_write=*/1); - _Py_atomic_store_int_relaxed(&gil->locked, 0); - COND_SIGNAL(gil->cond); - MUTEX_UNLOCK(gil->mutex); + drop_gil_impl(gil); #ifdef FORCE_SWITCHING /* We check tstate first in case we might be releasing the GIL for @@ -275,8 +281,10 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate) The function saves errno at entry and restores its value at exit. - tstate must be non-NULL. */ -static void + tstate must be non-NULL. + + Returns 1 if the GIL was acquired, or 0 if not. */ +static int take_gil(PyThreadState *tstate) { int err = errno; @@ -300,8 +308,8 @@ take_gil(PyThreadState *tstate) PyInterpreterState *interp = tstate->interp; struct _gil_runtime_state *gil = interp->ceval.gil; #ifdef Py_GIL_DISABLED - if (!gil->enabled) { - return; + if (!_Py_atomic_load_int_relaxed(&gil->enabled)) { + return 0; } #endif @@ -346,6 +354,17 @@ take_gil(PyThreadState *tstate) } } +#ifdef Py_GIL_DISABLED + if (!_Py_atomic_load_int_relaxed(&gil->enabled)) { + // Another thread disabled the GIL between our check above and + // now. Don't take the GIL, signal any other waiting threads, and + // return 0. + COND_SIGNAL(gil->cond); + MUTEX_UNLOCK(gil->mutex); + return 0; + } +#endif + #ifdef FORCE_SWITCHING /* This mutex must be taken before modifying gil->last_holder: see drop_gil(). */ @@ -387,6 +406,7 @@ take_gil(PyThreadState *tstate) MUTEX_UNLOCK(gil->mutex); errno = err; + return 1; } void _PyEval_SetSwitchInterval(unsigned long microseconds) @@ -451,7 +471,8 @@ init_own_gil(PyInterpreterState *interp, struct _gil_runtime_state *gil) { assert(!gil_created(gil)); #ifdef Py_GIL_DISABLED - gil->enabled = _PyInterpreterState_GetConfig(interp)->enable_gil == _PyConfig_GIL_ENABLE; + const PyConfig *config = _PyInterpreterState_GetConfig(interp); + gil->enabled = config->enable_gil == _PyConfig_GIL_ENABLE ? INT_MAX : 0; #endif create_gil(gil); assert(gil_created(gil)); @@ -545,11 +566,11 @@ PyEval_ReleaseLock(void) drop_gil(tstate->interp, tstate); } -void +int _PyEval_AcquireLock(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - take_gil(tstate); + return take_gil(tstate); } void @@ -1011,6 +1032,123 @@ _PyEval_InitState(PyInterpreterState *interp) _gil_initialize(&interp->_gil); } +#ifdef Py_GIL_DISABLED +int +_PyEval_IsGILEnabled(PyThreadState *tstate) +{ + return tstate->interp->ceval.gil->enabled != 0; +} + +int +_PyEval_EnableGILTransient(PyThreadState *tstate) +{ + if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != + _PyConfig_GIL_DEFAULT) { + return 0; + } + struct _gil_runtime_state *gil = tstate->interp->ceval.gil; + + int enabled = _Py_atomic_load_int_relaxed(&gil->enabled); + if (enabled == INT_MAX) { + // The GIL is already enabled permanently. + return 0; + } + if (enabled == INT_MAX - 1) { + Py_FatalError("Too many transient requests to enable the GIL"); + } + if (enabled > 0) { + // If enabled is nonzero, we know we hold the GIL. This means that no + // other threads are attached, and nobody else can be concurrently + // mutating it. + _Py_atomic_store_int_relaxed(&gil->enabled, enabled + 1); + return 0; + } + + // Enabling the GIL changes what it means to be an "attached" thread. To + // safely make this transition, we: + // 1. Detach the current thread. + // 2. Stop the world to detach (and suspend) all other threads. + // 3. Enable the GIL, if nobody else did between our check above and when + // our stop-the-world begins. + // 4. Start the world. + // 5. Attach the current thread. Other threads may attach and hold the GIL + // before this thread, which is harmless. + _PyThreadState_Detach(tstate); + + // This could be an interpreter-local stop-the-world in situations where we + // know that this interpreter's GIL is not shared, and that it won't become + // shared before the stop-the-world begins. For now, we always stop all + // interpreters for simplicity. + _PyEval_StopTheWorldAll(&_PyRuntime); + + enabled = _Py_atomic_load_int_relaxed(&gil->enabled); + int this_thread_enabled = enabled == 0; + _Py_atomic_store_int_relaxed(&gil->enabled, enabled + 1); + + _PyEval_StartTheWorldAll(&_PyRuntime); + _PyThreadState_Attach(tstate); + + return this_thread_enabled; +} + +int +_PyEval_EnableGILPermanent(PyThreadState *tstate) +{ + if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != + _PyConfig_GIL_DEFAULT) { + return 0; + } + + struct _gil_runtime_state *gil = tstate->interp->ceval.gil; + assert(current_thread_holds_gil(gil, tstate)); + + int enabled = _Py_atomic_load_int_relaxed(&gil->enabled); + if (enabled == INT_MAX) { + return 0; + } + + _Py_atomic_store_int_relaxed(&gil->enabled, INT_MAX); + return 1; +} + +int +_PyEval_DisableGIL(PyThreadState *tstate) +{ + if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != + _PyConfig_GIL_DEFAULT) { + return 0; + } + + struct _gil_runtime_state *gil = tstate->interp->ceval.gil; + assert(current_thread_holds_gil(gil, tstate)); + + int enabled = _Py_atomic_load_int_relaxed(&gil->enabled); + if (enabled == INT_MAX) { + return 0; + } + + assert(enabled >= 1); + enabled--; + + // Disabling the GIL is much simpler than enabling it, since we know we are + // the only attached thread. Other threads may start free-threading as soon + // as this store is complete, if it sets gil->enabled to 0. + _Py_atomic_store_int_relaxed(&gil->enabled, enabled); + + if (enabled == 0) { + // We're attached, so we know the GIL will remain disabled until at + // least the next time we detach, which must be after this function + // returns. + // + // Drop the GIL, which will wake up any threads waiting in take_gil() + // and let them resume execution without the GIL. + drop_gil_impl(gil); + return 1; + } + return 0; +} +#endif + /* Do periodic things, like check for signals and async I/0. * We need to do reasonably frequently, but not too frequently. diff --git a/Python/import.c b/Python/import.c index 4f91f03f091edd..efa3b0e0c44399 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1,6 +1,7 @@ /* Module definition and import implementation */ #include "Python.h" +#include "pycore_ceval.h" #include "pycore_hashtable.h" // _Py_hashtable_new_full() #include "pycore_import.h" // _PyImport_BootstrapImp() #include "pycore_initconfig.h" // _PyStatus_OK() @@ -68,6 +69,8 @@ static struct _inittab *inittab_copy = NULL; (interp)->imports.modules #define MODULES_BY_INDEX(interp) \ (interp)->imports.modules_by_index +#define MODULE_GIL_BY_INDEX(interp) \ + (interp)->imports.module_gil_by_index #define IMPORTLIB(interp) \ (interp)->imports.importlib #define OVERRIDE_MULTI_INTERP_EXTENSIONS_CHECK(interp) \ @@ -498,6 +501,59 @@ _modules_by_index_clear_one(PyInterpreterState *interp, PyModuleDef *def) return PyList_SetItem(MODULES_BY_INDEX(interp), index, Py_NewRef(Py_None)); } +#ifdef Py_GIL_DISABLED +int +_PyImport_SetModuleGIL(PyObject *module, void *gil) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + assert(PyModule_Check(module)); + PyModuleDef *def = ((PyModuleObject *)module)->md_def; + if (def->m_size != -1) { + return 0; + } + + Py_ssize_t index = def->m_base.m_index; + assert(index > 0); + + // This sequence assumes that it is called during module loading, when the + // GIL is enabled. + assert(_PyEval_IsGILEnabled(_PyThreadState_GET())); + PyObject *gil_list = MODULE_GIL_BY_INDEX(interp); + if (gil_list == NULL) { + gil_list = MODULE_GIL_BY_INDEX(interp) = PyList_New(0); + if (gil_list == NULL) { + return -1; + } + } + while (PyList_GET_SIZE(gil_list) <= index) { + if (PyList_Append(gil_list, Py_None) < 0) { + return -1; + } + } + + PyObject *gil_long = PyLong_FromVoidPtr(gil); + if (gil_long == NULL) { + return -1; + } + return PyList_SetItem(gil_list, index, gil_long); +} + +static void * +_get_moduledef_gil(PyModuleDef *def) +{ + assert(def->m_size == -1); + Py_ssize_t index = def->m_base.m_index; + assert(index > 0); + + PyObject *gil_list = MODULE_GIL_BY_INDEX(_PyInterpreterState_GET()); + assert(gil_list != NULL); + assert(index < PyList_GET_SIZE(gil_list)); + + PyObject *gil = PyList_GET_ITEM(gil_list, index); + assert(PyLong_CheckExact(gil)); + return PyLong_AsVoidPtr(gil); +} +#endif PyObject* PyState_FindModule(PyModuleDef* module) @@ -1172,6 +1228,47 @@ _PyImport_CheckSubinterpIncompatibleExtensionAllowed(const char *name) return 0; } +#ifdef Py_GIL_DISABLED +int +_PyImport_CheckGILForModule(PyObject* module, PyObject *module_name) +{ + PyThreadState *tstate = _PyThreadState_GET(); + if (module == NULL) { + _PyEval_DisableGIL(tstate); + return 0; + } + + if (!PyModule_Check(module) || + ((PyModuleObject *)module)->md_gil == Py_MOD_GIL_USED) { + if (_PyEval_EnableGILPermanent(tstate)) { + int warn_result = PyErr_WarnFormat( + PyExc_RuntimeWarning, + 1, + "The global interpreter lock (GIL) has been enabled to load " + "module '%U', which has not declared that it can run safely " + "without the GIL. To override this behavior and keep the GIL " + "disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.", + module_name + ); + if (warn_result < 0) { + return warn_result; + } + } + + const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); + if (config->enable_gil == _PyConfig_GIL_DEFAULT && config->verbose) { + PySys_FormatStderr("# loading module '%U', which requires the GIL\n", + module_name); + } + } + else { + _PyEval_DisableGIL(tstate); + } + + return 0; +} +#endif + static PyObject * get_core_module_dict(PyInterpreterState *interp, PyObject *name, PyObject *path) @@ -1416,6 +1513,13 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, Py_DECREF(mod); return NULL; } +#ifdef Py_GIL_DISABLED + if (def->m_base.m_copy != NULL) { + // For non-core modules, fetch the GIL slot that was remembered in + // _PyImport_RunModInitFunc(). + ((PyModuleObject *)mod)->md_gil = _get_moduledef_gil(def); + } +#endif /* We can't set mod->md_def if it's missing, * because _PyImport_ClearModulesByIndex() might break * due to violating interpreter isolation. See the note @@ -1700,6 +1804,9 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) return NULL; } +#ifdef Py_GIL_DISABLED + _PyEval_EnableGILTransient(tstate); +#endif PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); @@ -1735,6 +1842,11 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) tstate, p0, &info, spec, get_modules_dict(tstate, true)); finally: +#ifdef Py_GIL_DISABLED + if (_PyImport_CheckGILForModule(mod, info.name) < 0) { + Py_CLEAR(mod); + } +#endif _Py_ext_module_loader_info_clear(&info); return mod; } @@ -3467,6 +3579,9 @@ _PyImport_ClearCore(PyInterpreterState *interp) by _PyImport_FiniCore(). */ Py_CLEAR(MODULES(interp)); Py_CLEAR(MODULES_BY_INDEX(interp)); +#ifdef Py_GIL_DISABLED + Py_CLEAR(MODULE_GIL_BY_INDEX(interp)); +#endif Py_CLEAR(IMPORTLIB(interp)); Py_CLEAR(IMPORT_FUNC(interp)); } @@ -4054,6 +4169,9 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) return NULL; } +#ifdef Py_GIL_DISABLED + _PyEval_EnableGILTransient(tstate); +#endif mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); @@ -4101,6 +4219,11 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) } finally: +#ifdef Py_GIL_DISABLED + if (_PyImport_CheckGILForModule(mod, info.name) < 0) { + Py_CLEAR(mod); + } +#endif _Py_ext_module_loader_info_clear(&info); return mod; } diff --git a/Python/importdl.c b/Python/importdl.c index a3091946bc94bf..172ea5575287e7 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -472,6 +472,15 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, &res, _Py_ext_module_loader_result_ERR_MISSING_DEF); goto error; } + +#ifdef Py_GIL_DISABLED + if (res.def->m_size == -1) { + // This module may be recreated (without running its init function) + // in reload_singlephase_extension(), so remember its GIL slot + // here. + _PyImport_SetModuleGIL(m, ((PyModuleObject *)m)->md_gil); + } +#endif } assert(!PyErr_Occurred()); diff --git a/Python/pystate.c b/Python/pystate.c index f442d87ba3150e..27e57841304ca8 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -847,6 +847,9 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) assert(interp->imports.modules == NULL); assert(interp->imports.modules_by_index == NULL); +#ifdef Py_GIL_DISABLED + assert(interp->imports.module_gil_by_index == NULL); +#endif assert(interp->imports.importlib == NULL); assert(interp->imports.import_func == NULL); @@ -2055,19 +2058,36 @@ _PyThreadState_Attach(PyThreadState *tstate) Py_FatalError("non-NULL old thread state"); } - _PyEval_AcquireLock(tstate); - // XXX assert(tstate_is_alive(tstate)); - current_fast_set(&_PyRuntime, tstate); - tstate_activate(tstate); + while (1) { + int acquired_gil = _PyEval_AcquireLock(tstate); - if (!tstate_try_attach(tstate)) { - tstate_wait_attach(tstate); - } + // XXX assert(tstate_is_alive(tstate)); + current_fast_set(&_PyRuntime, tstate); + tstate_activate(tstate); + + if (!tstate_try_attach(tstate)) { + tstate_wait_attach(tstate); + } #ifdef Py_GIL_DISABLED - _Py_qsbr_attach(((_PyThreadStateImpl *)tstate)->qsbr); + if (!!tstate->interp->ceval.gil->enabled != acquired_gil) { + // The GIL was enabled between our call to _PyEval_AcquireLock() + // and when we attached (the GIL can't go from enabled to disabled + // here because only a thread holding the GIL can disable + // it). Detach and try again. + assert(!acquired_gil); + tstate_set_detached(tstate, _Py_THREAD_DETACHED); + tstate_deactivate(tstate); + current_fast_clear(&_PyRuntime); + continue; + } + _Py_qsbr_attach(((_PyThreadStateImpl *)tstate)->qsbr); +#else + (void)acquired_gil; #endif + break; + } // Resume previous critical section. This acquires the lock(s) from the // top-most critical section. From 3e8fee5067ce84d32d25b2251cb5f5922062a6e7 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 3 May 2024 17:49:38 +0000 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-05-03-17-49-37.gh-issue-116322.Gy6M4j.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-05-03-17-49-37.gh-issue-116322.Gy6M4j.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-03-17-49-37.gh-issue-116322.Gy6M4j.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-03-17-49-37.gh-issue-116322.Gy6M4j.rst new file mode 100644 index 00000000000000..d3ea5e0d0f3596 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-03-17-49-37.gh-issue-116322.Gy6M4j.rst @@ -0,0 +1 @@ +In ``--disable-gil`` builds, the GIL will be enabled while loading C extension modules. If the module indicates that it supports running without the GIL, the GIL will be disabled once loading is complete. Otherwise, the GIL will remain enabled for the remainder of the interpreter's lifetime. This behavior does not apply if the GIL has been explicitly enabled or disabled with ``PYTHON_GIL`` or ``-Xgil``. From 637c3e50750f65c12a41f9946a5ce64c7328af11 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Fri, 3 May 2024 11:35:47 -0700 Subject: [PATCH 3/7] Mark _wmi module --- PC/_wmimodule.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/PC/_wmimodule.cpp b/PC/_wmimodule.cpp index 22ed05276e6f07..48863b90f4cc27 100644 --- a/PC/_wmimodule.cpp +++ b/PC/_wmimodule.cpp @@ -362,12 +362,18 @@ static PyMethodDef wmi_functions[] = { { NULL, NULL, 0, NULL } }; +static PyModuleDef_Slot wmi_slots[] = { + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, + {0, NULL}, +}; + static PyModuleDef wmi_def = { PyModuleDef_HEAD_INIT, "_wmi", - NULL, // doc - 0, // m_size - wmi_functions + NULL, // doc + 0, // m_size + wmi_functions, // m_methods + wmi_slots, // m_slots }; extern "C" { From fecf4bd2a97ceae90acac713bbf8d74d3d248b57 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Fri, 3 May 2024 12:19:20 -0700 Subject: [PATCH 4/7] Use _PyEval_IsGILEnabled() in _is_gil_enabled() --- Python/sysmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 645b76fccf602c..fdb583b6faf086 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -2404,8 +2404,7 @@ sys__is_gil_enabled_impl(PyObject *module) /*[clinic end generated code: output=57732cf53f5b9120 input=7e9c47f15a00e809]*/ { #ifdef Py_GIL_DISABLED - PyInterpreterState *interp = _PyInterpreterState_GET(); - return interp->ceval.gil->enabled; + return _PyEval_IsGILEnabled(_PyThreadState_GET()); #else return 1; #endif From 0fd1967be2117d80d265f1b7fe998c3b22a9812c Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Fri, 3 May 2024 16:09:44 -0700 Subject: [PATCH 5/7] Review comments from Sam, plus some cleanup --- Include/internal/pycore_ceval.h | 6 +++++- Include/internal/pycore_import.h | 5 ++++- Python/ceval_gil.c | 18 ++++++------------ Python/import.c | 4 +--- Python/importdl.c | 6 +++++- Python/pystate.c | 2 +- 6 files changed, 22 insertions(+), 19 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 400571f859f79f..03069b46c88135 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -142,7 +142,11 @@ extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *); // // The enabled state of the GIL will not change while one or more threads are // attached. -extern int _PyEval_IsGILEnabled(PyThreadState *tstate); +static inline int +_PyEval_IsGILEnabled(PyThreadState *tstate) +{ + return tstate->interp->ceval.gil->enabled != 0; +} // Enable or disable the GIL used by the interpreter that owns tstate, which // must be the current thread. This may affect other interpreters, if the GIL diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index 3a5f0529f30379..b04d62c0014c9c 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -213,8 +213,11 @@ extern int _PyImport_CheckSubinterpIncompatibleExtensionAllowed( PyAPI_FUNC(int) _PyImport_ClearExtension(PyObject *name, PyObject *filename); #ifdef Py_GIL_DISABLED +// Store an association between module and gil (which should be one of the +// values for the Py_mod_gil module slot) in the current interpreter. +// +// Only for use on modules with md_def->m_size == -1. extern int _PyImport_SetModuleGIL(PyObject *module, void *gil); -extern void *_PyImport_GetModuleDefGIL(PyModuleDef *def); // Assuming that the GIL is enabled from a call to // _PyEval_EnableGILTransient(), resolve the transient request depending on the diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 5ec92c2026eb22..58167ed1cad953 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -1033,17 +1033,11 @@ _PyEval_InitState(PyInterpreterState *interp) } #ifdef Py_GIL_DISABLED -int -_PyEval_IsGILEnabled(PyThreadState *tstate) -{ - return tstate->interp->ceval.gil->enabled != 0; -} - int _PyEval_EnableGILTransient(PyThreadState *tstate) { - if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != - _PyConfig_GIL_DEFAULT) { + const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); + if (config->enable_gil != _PyConfig_GIL_DEFAULT) { return 0; } struct _gil_runtime_state *gil = tstate->interp->ceval.gil; @@ -1094,8 +1088,8 @@ _PyEval_EnableGILTransient(PyThreadState *tstate) int _PyEval_EnableGILPermanent(PyThreadState *tstate) { - if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != - _PyConfig_GIL_DEFAULT) { + const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); + if (config->enable_gil != _PyConfig_GIL_DEFAULT) { return 0; } @@ -1114,8 +1108,8 @@ _PyEval_EnableGILPermanent(PyThreadState *tstate) int _PyEval_DisableGIL(PyThreadState *tstate) { - if (_PyInterpreterState_GetConfig(tstate->interp)->enable_gil != - _PyConfig_GIL_DEFAULT) { + const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); + if (config->enable_gil != _PyConfig_GIL_DEFAULT) { return 0; } diff --git a/Python/import.c b/Python/import.c index efa3b0e0c44399..86fe336a615099 100644 --- a/Python/import.c +++ b/Python/import.c @@ -508,9 +508,7 @@ _PyImport_SetModuleGIL(PyObject *module, void *gil) PyInterpreterState *interp = _PyInterpreterState_GET(); assert(PyModule_Check(module)); PyModuleDef *def = ((PyModuleObject *)module)->md_def; - if (def->m_size != -1) { - return 0; - } + assert(def->m_size == -1); Py_ssize_t index = def->m_base.m_index; assert(index > 0); diff --git a/Python/importdl.c b/Python/importdl.c index 172ea5575287e7..829e815c3ea4fd 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -478,7 +478,11 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, // This module may be recreated (without running its init function) // in reload_singlephase_extension(), so remember its GIL slot // here. - _PyImport_SetModuleGIL(m, ((PyModuleObject *)m)->md_gil); + if (_PyImport_SetModuleGIL(m, ((PyModuleObject *)m)->md_gil) < 0) { + _Py_ext_module_loader_result_set_error( + &res, _Py_ext_module_loader_result_EXCEPTION); + goto error; + } } #endif } diff --git a/Python/pystate.c b/Python/pystate.c index 27e57841304ca8..ac135bfdffca99 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2071,7 +2071,7 @@ _PyThreadState_Attach(PyThreadState *tstate) } #ifdef Py_GIL_DISABLED - if (!!tstate->interp->ceval.gil->enabled != acquired_gil) { + if (_PyEval_IsGILEnabled(tstate) != acquired_gil) { // The GIL was enabled between our call to _PyEval_AcquireLock() // and when we attached (the GIL can't go from enabled to disabled // here because only a thread holding the GIL can disable From 0271a168dcffee449b0511c5978c95de392ba1c0 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Mon, 6 May 2024 13:48:31 -0700 Subject: [PATCH 6/7] Store GIL slot in cache instead of separate list, other fixes from Eric --- Include/internal/pycore_import.h | 12 --- Python/import.c | 132 +++++++++++++------------------ Python/importdl.c | 13 --- Python/pystate.c | 3 - 4 files changed, 57 insertions(+), 103 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index b04d62c0014c9c..bd40707fed21a8 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -83,12 +83,6 @@ struct _import_state { This is initialized lazily in PyState_AddModule(), which is also where modules get added. */ PyObject *modules_by_index; -#ifdef Py_GIL_DISABLED - /* This list contains GIL slots (see Py_mod_gil) for modules that may be - reinitialized by reload_singlephase_extension(). It is indexed the same - way as modules_by_index. */ - PyObject *module_gil_by_index; -#endif /* importlib module._bootstrap */ PyObject *importlib; /* override for config->use_frozen_modules (for tests) @@ -213,12 +207,6 @@ extern int _PyImport_CheckSubinterpIncompatibleExtensionAllowed( PyAPI_FUNC(int) _PyImport_ClearExtension(PyObject *name, PyObject *filename); #ifdef Py_GIL_DISABLED -// Store an association between module and gil (which should be one of the -// values for the Py_mod_gil module slot) in the current interpreter. -// -// Only for use on modules with md_def->m_size == -1. -extern int _PyImport_SetModuleGIL(PyObject *module, void *gil); - // Assuming that the GIL is enabled from a call to // _PyEval_EnableGILTransient(), resolve the transient request depending on the // state of the module argument: diff --git a/Python/import.c b/Python/import.c index 34d34b77788b5d..a9fadb58155ba7 100644 --- a/Python/import.c +++ b/Python/import.c @@ -80,8 +80,6 @@ static struct _inittab *inittab_copy = NULL; (interp)->imports.modules #define MODULES_BY_INDEX(interp) \ (interp)->imports.modules_by_index -#define MODULE_GIL_BY_INDEX(interp) \ - (interp)->imports.module_gil_by_index #define IMPORTLIB(interp) \ (interp)->imports.importlib #define OVERRIDE_MULTI_INTERP_EXTENSIONS_CHECK(interp) \ @@ -546,57 +544,6 @@ _modules_by_index_clear_one(PyInterpreterState *interp, Py_ssize_t index) return PyList_SetItem(MODULES_BY_INDEX(interp), index, Py_NewRef(Py_None)); } -#ifdef Py_GIL_DISABLED -int -_PyImport_SetModuleGIL(PyObject *module, void *gil) -{ - PyInterpreterState *interp = _PyInterpreterState_GET(); - assert(PyModule_Check(module)); - PyModuleDef *def = ((PyModuleObject *)module)->md_def; - assert(def->m_size == -1); - - Py_ssize_t index = def->m_base.m_index; - assert(index > 0); - - // This sequence assumes that it is called during module loading, when the - // GIL is enabled. - assert(_PyEval_IsGILEnabled(_PyThreadState_GET())); - PyObject *gil_list = MODULE_GIL_BY_INDEX(interp); - if (gil_list == NULL) { - gil_list = MODULE_GIL_BY_INDEX(interp) = PyList_New(0); - if (gil_list == NULL) { - return -1; - } - } - while (PyList_GET_SIZE(gil_list) <= index) { - if (PyList_Append(gil_list, Py_None) < 0) { - return -1; - } - } - - PyObject *gil_long = PyLong_FromVoidPtr(gil); - if (gil_long == NULL) { - return -1; - } - return PyList_SetItem(gil_list, index, gil_long); -} - -static void * -_get_moduledef_gil(PyModuleDef *def) -{ - assert(def->m_size == -1); - Py_ssize_t index = def->m_base.m_index; - assert(index > 0); - - PyObject *gil_list = MODULE_GIL_BY_INDEX(_PyInterpreterState_GET()); - assert(gil_list != NULL); - assert(index < PyList_GET_SIZE(gil_list)); - - PyObject *gil = PyList_GET_ITEM(gil_list, index); - assert(PyLong_CheckExact(gil)); - return PyLong_AsVoidPtr(gil); -} -#endif PyObject* PyState_FindModule(PyModuleDef* module) @@ -1077,6 +1024,12 @@ struct extensions_cache_value { struct cached_m_dict _m_dict; _Py_ext_module_origin origin; + +#ifdef Py_GIL_DISABLED + /* The module's md_gil slot, for legacy modules that are reinitialized from + m_dict rather than calling their initialization function again. */ + void *md_gil; +#endif }; static struct extensions_cache_value * @@ -1405,7 +1358,7 @@ static struct extensions_cache_value * _extensions_cache_set(PyObject *path, PyObject *name, PyModuleDef *def, PyModInitFunction m_init, Py_ssize_t m_index, PyObject *m_dict, - _Py_ext_module_origin origin) + _Py_ext_module_origin origin, void *md_gil) { struct extensions_cache_value *value = NULL; void *key = NULL; @@ -1455,7 +1408,13 @@ _extensions_cache_set(PyObject *path, PyObject *name, .m_index=m_index, /* m_dict is set by set_cached_m_dict(). */ .origin=origin, +#ifdef Py_GIL_DISABLED + .md_gil=md_gil, +#endif }; +#ifndef Py_GIL_DISABLED + (void)md_gil; +#endif if (init_cached_m_dict(newvalue, m_dict) < 0) { goto finally; } @@ -1720,6 +1679,9 @@ struct singlephase_global_update { Py_ssize_t m_index; PyObject *m_dict; _Py_ext_module_origin origin; +#ifdef Py_GIL_DISABLED + void *md_gil; +#endif }; static struct extensions_cache_value * @@ -1778,7 +1740,13 @@ update_global_state_for_extension(PyThreadState *tstate, #endif cached = _extensions_cache_set( path, name, def, m_init, singlephase->m_index, m_dict, - singlephase->origin); + singlephase->origin, +#ifdef Py_GIL_DISABLED + singlephase->md_gil +#else + NULL +#endif + ); if (cached == NULL) { // XXX Ignore this error? Doing so would effectively // mark the module as not loadable. @@ -1865,9 +1833,9 @@ reload_singlephase_extension(PyThreadState *tstate, } #ifdef Py_GIL_DISABLED if (def->m_base.m_copy != NULL) { - // For non-core modules, fetch the GIL slot that was remembered in - // _PyImport_RunModInitFunc(). - ((PyModuleObject *)mod)->md_gil = _get_moduledef_gil(def); + // For non-core modules, fetch the GIL slot that was stored by + // import_run_extension(). + ((PyModuleObject *)mod)->md_gil = cached->md_gil; } #endif /* We can't set mod->md_def if it's missing, @@ -2023,6 +1991,9 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, // cache is less reliable than it should be). .m_index=def->m_base.m_index, .origin=info->origin, +#ifdef Py_GIL_DISABLED + .md_gil=((PyModuleObject *)mod)->md_gil, +#endif }; // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. @@ -2141,6 +2112,10 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, /* We don't want def->m_base.m_copy populated. */ .m_dict=NULL, .origin=_Py_ext_module_origin_CORE, +#ifdef Py_GIL_DISABLED + /* Unused when m_dict == NULL. */ + .md_gil=NULL, +#endif }; cached = update_global_state_for_extension( tstate, nameobj, nameobj, def, &singlephase); @@ -2186,9 +2161,6 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) return NULL; } -#ifdef Py_GIL_DISABLED - _PyEval_EnableGILTransient(tstate); -#endif struct extensions_cache_value *cached = NULL; PyObject *mod = import_find_extension(tstate, &info, &cached); if (mod != NULL) { @@ -2233,16 +2205,25 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) goto finally; } +#ifdef Py_GIL_DISABLED + // This call (and the corresponding call to _PyImport_CheckGILForModule()) + // would ideally be inside import_run_extension(). They are kept in the + // callers for now because that would complicate the control flow inside + // import_run_extension(). It should be possible to restructure + // import_run_extension() to address this. + _PyEval_EnableGILTransient(tstate); +#endif /* Now load it. */ mod = import_run_extension( tstate, p0, &info, spec, get_modules_dict(tstate, true)); - -finally: #ifdef Py_GIL_DISABLED if (_PyImport_CheckGILForModule(mod, info.name) < 0) { Py_CLEAR(mod); + goto finally; } #endif + +finally: _Py_ext_module_loader_info_clear(&info); return mod; } @@ -3978,9 +3959,6 @@ _PyImport_ClearCore(PyInterpreterState *interp) by _PyImport_FiniCore(). */ Py_CLEAR(MODULES(interp)); Py_CLEAR(MODULES_BY_INDEX(interp)); -#ifdef Py_GIL_DISABLED - Py_CLEAR(MODULE_GIL_BY_INDEX(interp)); -#endif Py_CLEAR(IMPORTLIB(interp)); Py_CLEAR(IMPORT_FUNC(interp)); } @@ -4568,9 +4546,6 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) return NULL; } -#ifdef Py_GIL_DISABLED - _PyEval_EnableGILTransient(tstate); -#endif struct extensions_cache_value *cached = NULL; mod = import_find_extension(tstate, &info, &cached); if (mod != NULL) { @@ -4621,10 +4596,22 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) goto finally; } +#ifdef Py_GIL_DISABLED + // This call (and the corresponding call to _PyImport_CheckGILForModule()) + // would ideally be inside import_run_extension(). They are kept in the + // callers for now because that would complicate the control flow inside + // import_run_extension(). It should be possible to restructure + // import_run_extension() to address this. + _PyEval_EnableGILTransient(tstate); +#endif mod = import_run_extension( tstate, p0, &info, spec, get_modules_dict(tstate, true)); - if (mod == NULL) { +#ifdef Py_GIL_DISABLED + if (_PyImport_CheckGILForModule(mod, info.name) < 0) { + Py_CLEAR(mod); + goto finally; } +#endif // XXX Shouldn't this happen in the error cases too (i.e. in "finally")? if (fp) { @@ -4632,11 +4619,6 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) } finally: -#ifdef Py_GIL_DISABLED - if (_PyImport_CheckGILForModule(mod, info.name) < 0) { - Py_CLEAR(mod); - } -#endif _Py_ext_module_loader_info_clear(&info); return mod; } diff --git a/Python/importdl.c b/Python/importdl.c index ba65320b198f75..38f56db4b4cc96 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -490,19 +490,6 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, &res, _Py_ext_module_loader_result_ERR_MISSING_DEF); goto error; } - -#ifdef Py_GIL_DISABLED - if (res.def->m_size == -1) { - // This module may be recreated (without running its init function) - // in reload_singlephase_extension(), so remember its GIL slot - // here. - if (_PyImport_SetModuleGIL(m, ((PyModuleObject *)m)->md_gil) < 0) { - _Py_ext_module_loader_result_set_error( - &res, _Py_ext_module_loader_result_EXCEPTION); - goto error; - } - } -#endif } assert(!PyErr_Occurred()); diff --git a/Python/pystate.c b/Python/pystate.c index ac135bfdffca99..660454330a2448 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -847,9 +847,6 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) assert(interp->imports.modules == NULL); assert(interp->imports.modules_by_index == NULL); -#ifdef Py_GIL_DISABLED - assert(interp->imports.module_gil_by_index == NULL); -#endif assert(interp->imports.importlib == NULL); assert(interp->imports.import_func == NULL); From 2d8c2b2bd317fc02e10d304242cf062928732fba Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Mon, 6 May 2024 15:28:11 -0700 Subject: [PATCH 7/7] Always define singlephase_global_update.md_gil to clean up callsite --- Python/import.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index a9fadb58155ba7..0b58567dde1fec 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1679,9 +1679,7 @@ struct singlephase_global_update { Py_ssize_t m_index; PyObject *m_dict; _Py_ext_module_origin origin; -#ifdef Py_GIL_DISABLED void *md_gil; -#endif }; static struct extensions_cache_value * @@ -1740,13 +1738,7 @@ update_global_state_for_extension(PyThreadState *tstate, #endif cached = _extensions_cache_set( path, name, def, m_init, singlephase->m_index, m_dict, - singlephase->origin, -#ifdef Py_GIL_DISABLED - singlephase->md_gil -#else - NULL -#endif - ); + singlephase->origin, singlephase->md_gil); if (cached == NULL) { // XXX Ignore this error? Doing so would effectively // mark the module as not loadable.