From ca1f78e8a17759157df576b410d3a61e807aa307 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 10 Apr 2023 10:39:30 -0600 Subject: [PATCH 1/7] Add _PyEval_AcquireLock(). --- Include/internal/pycore_ceval.h | 1 + Python/ceval_gil.c | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index b7a9bf40425bc7..3a57b562ca49d9 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -100,6 +100,7 @@ extern int _PyEval_ThreadsInitialized(void); extern PyStatus _PyEval_InitGIL(PyThreadState *tstate, int own_gil); extern void _PyEval_FiniGIL(PyInterpreterState *interp); +extern void _PyEval_AcquireLock(PyThreadState *tstate); extern void _PyEval_ReleaseLock(PyThreadState *tstate); extern void _PyEval_DeactivateOpCache(void); diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index a390bec80d556c..698c28593c5f69 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -609,9 +609,17 @@ PyEval_ReleaseLock(void) drop_gil(ceval, tstate); } +void +_PyEval_AcquireLock(PyThreadState *tstate) +{ + _Py_EnsureTstateNotNULL(tstate); + take_gil(tstate); +} + void _PyEval_ReleaseLock(PyThreadState *tstate) { + _Py_EnsureTstateNotNULL(tstate); struct _ceval_state *ceval = &tstate->interp->ceval; drop_gil(ceval, tstate); } From c5d3b2cc5843e66c652bff50c6f3373c15ea98e6 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 10 Apr 2023 11:16:12 -0600 Subject: [PATCH 2/7] Add _PyThreadState_SwapNoGIL(). --- Include/internal/pycore_ceval.h | 1 + Python/ceval_gil.c | 14 +++++------ Python/pylifecycle.c | 17 ++++++++++--- Python/pystate.c | 42 +++++++++++++++++++++++++-------- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 3a57b562ca49d9..9fd8571cbc87f4 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -102,6 +102,7 @@ extern void _PyEval_FiniGIL(PyInterpreterState *interp); extern void _PyEval_AcquireLock(PyThreadState *tstate); extern void _PyEval_ReleaseLock(PyThreadState *tstate); +extern PyThreadState * _PyThreadState_SwapNoGIL(PyThreadState *); extern void _PyEval_DeactivateOpCache(void); diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 698c28593c5f69..a53344409854ca 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -631,7 +631,7 @@ PyEval_AcquireThread(PyThreadState *tstate) take_gil(tstate); - if (_PyThreadState_Swap(tstate->interp->runtime, tstate) != NULL) { + if (_PyThreadState_SwapNoGIL(tstate) != NULL) { Py_FatalError("non-NULL old thread state"); } } @@ -641,8 +641,7 @@ PyEval_ReleaseThread(PyThreadState *tstate) { assert(is_tstate_valid(tstate)); - _PyRuntimeState *runtime = tstate->interp->runtime; - PyThreadState *new_tstate = _PyThreadState_Swap(runtime, NULL); + PyThreadState *new_tstate = _PyThreadState_SwapNoGIL(NULL); if (new_tstate != tstate) { Py_FatalError("wrong thread state"); } @@ -690,8 +689,7 @@ _PyEval_SignalAsyncExc(PyInterpreterState *interp) PyThreadState * PyEval_SaveThread(void) { - _PyRuntimeState *runtime = &_PyRuntime; - PyThreadState *tstate = _PyThreadState_Swap(runtime, NULL); + PyThreadState *tstate = _PyThreadState_SwapNoGIL(NULL); _Py_EnsureTstateNotNULL(tstate); struct _ceval_state *ceval = &tstate->interp->ceval; @@ -707,7 +705,7 @@ PyEval_RestoreThread(PyThreadState *tstate) take_gil(tstate); - _PyThreadState_Swap(tstate->interp->runtime, tstate); + _PyThreadState_SwapNoGIL(tstate); } @@ -1011,7 +1009,7 @@ _Py_HandlePending(PyThreadState *tstate) /* GIL drop request */ if (_Py_atomic_load_relaxed_int32(&interp_ceval_state->gil_drop_request)) { /* Give another thread a chance */ - if (_PyThreadState_Swap(runtime, NULL) != tstate) { + if (_PyThreadState_SwapNoGIL(NULL) != tstate) { Py_FatalError("tstate mix-up"); } drop_gil(interp_ceval_state, tstate); @@ -1020,7 +1018,7 @@ _Py_HandlePending(PyThreadState *tstate) take_gil(tstate); - if (_PyThreadState_Swap(runtime, tstate) != NULL) { + if (_PyThreadState_SwapNoGIL(tstate) != NULL) { Py_FatalError("orphan tstate"); } } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 705708698c6a8b..1dd6ce2c7ddae7 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -591,6 +591,7 @@ init_interp_create_gil(PyThreadState *tstate, int own_gil) /* finalize_interp_delete() comment explains why _PyEval_FiniGIL() is only called here. */ + // XXX This is broken with a per-interpreter GIL. _PyEval_FiniGIL(tstate->interp); /* Auto-thread-state API */ @@ -645,7 +646,8 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return _PyStatus_ERR("can't make first thread"); } _PyThreadState_Bind(tstate); - (void) PyThreadState_Swap(tstate); + // XXX For now we do this before the GIL is created. + (void) _PyThreadState_SwapNoGIL(tstate); status = init_interp_create_gil(tstate, config.own_gil); if (_PyStatus_EXCEPTION(status)) { @@ -2025,11 +2027,14 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) } _PyThreadState_Bind(tstate); - PyThreadState *save_tstate = PyThreadState_Swap(tstate); + // XXX For now we do this before the GIL is created. + PyThreadState *save_tstate = _PyThreadState_SwapNoGIL(tstate); + int has_gil = 0; /* Copy the current interpreter config into the new interpreter */ const PyConfig *src_config; if (save_tstate != NULL) { + _PyEval_ReleaseLock(save_tstate); src_config = _PyInterpreterState_GetConfig(save_tstate->interp); } else @@ -2053,6 +2058,7 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) if (_PyStatus_EXCEPTION(status)) { goto error; } + has_gil = 1; status = pycore_interp_init(tstate); if (_PyStatus_EXCEPTION(status)) { @@ -2072,7 +2078,12 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) /* Oops, it didn't work. Undo it all. */ PyErr_PrintEx(0); - PyThreadState_Swap(save_tstate); + if (has_gil) { + PyThreadState_Swap(save_tstate); + } + else { + _PyThreadState_SwapNoGIL(save_tstate); + } PyThreadState_Clear(tstate); PyThreadState_Delete(tstate); PyInterpreterState_Delete(interp); diff --git a/Python/pystate.c b/Python/pystate.c index 75bd9f41e301a3..f14934361dab78 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1863,17 +1863,11 @@ PyThreadState_Get(void) } -PyThreadState * -_PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts) +static void +_swap_thread_states(_PyRuntimeState *runtime, + PyThreadState *oldts, PyThreadState *newts) { -#if defined(Py_DEBUG) - /* This can be called from PyEval_RestoreThread(). Similar - to it, we need to ensure errno doesn't change. - */ - int err = errno; -#endif - PyThreadState *oldts = current_fast_get(runtime); - + // XXX Do this only if oldts != NULL? current_fast_clear(runtime); if (oldts != NULL) { @@ -1887,6 +1881,20 @@ _PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts) current_fast_set(runtime, newts); tstate_activate(newts); } +} + +PyThreadState * +_PyThreadState_SwapNoGIL(PyThreadState *newts) +{ +#if defined(Py_DEBUG) + /* This can be called from PyEval_RestoreThread(). Similar + to it, we need to ensure errno doesn't change. + */ + int err = errno; +#endif + + PyThreadState *oldts = current_fast_get(&_PyRuntime); + _swap_thread_states(&_PyRuntime, oldts, newts); #if defined(Py_DEBUG) errno = err; @@ -1894,6 +1902,20 @@ _PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts) return oldts; } +PyThreadState * +_PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts) +{ + PyThreadState *oldts = current_fast_get(runtime); + if (oldts != NULL) { + _PyEval_ReleaseLock(oldts); + } + _swap_thread_states(runtime, oldts, newts); + if (newts != NULL) { + _PyEval_AcquireLock(newts); + } + return oldts; +} + PyThreadState * PyThreadState_Swap(PyThreadState *newts) { From 2404310dea7c14a59fba9a7ffae4f203005bbf4f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 10 Apr 2023 12:39:33 -0600 Subject: [PATCH 3/7] Acquire the GIL when not owned. --- Python/ceval_gil.c | 54 +++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index a53344409854ca..adf8e7b365895c 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -499,42 +499,56 @@ PyEval_ThreadsInitialized(void) return _PyEval_ThreadsInitialized(); } +static void +init_shared_gil(PyInterpreterState *interp, struct _gil_runtime_state *gil) +{ + assert(gil_created(gil)); + interp->ceval.gil = gil; + interp->ceval.own_gil = 0; +} + +static void +init_own_gil(PyInterpreterState *interp, struct _gil_runtime_state *gil) +{ + assert(!gil_created(gil)); + create_gil(gil); + assert(gil_created(gil)); + interp->ceval.gil = gil; + interp->ceval.own_gil = 1; +} + PyStatus _PyEval_InitGIL(PyThreadState *tstate, int own_gil) { assert(tstate->interp->ceval.gil == NULL); + int locked; if (!own_gil) { PyInterpreterState *main_interp = _PyInterpreterState_Main(); assert(tstate->interp != main_interp); - struct _gil_runtime_state *gil = main_interp->ceval.gil; - assert(gil_created(gil)); - tstate->interp->ceval.gil = gil; - tstate->interp->ceval.own_gil = 0; - return _PyStatus_OK(); + init_shared_gil(tstate->interp, main_interp->ceval.gil); + locked = _Py_atomic_load_relaxed(&main_interp->ceval.gil->locked); } - /* XXX per-interpreter GIL */ - struct _gil_runtime_state *gil = &tstate->interp->runtime->ceval.gil; - if (!_Py_IsMainInterpreter(tstate->interp)) { + else if (!_Py_IsMainInterpreter(tstate->interp)) { /* Currently, the GIL is shared by all interpreters, and only the main interpreter is responsible to create and destroy it. */ - assert(gil_created(gil)); - tstate->interp->ceval.gil = gil; + struct _gil_runtime_state *main_gil = _PyInterpreterState_Main()->ceval.gil; + init_shared_gil(tstate->interp, main_gil); // XXX For now we lie. tstate->interp->ceval.own_gil = 1; - return _PyStatus_OK(); + locked = _Py_atomic_load_relaxed(&main_gil->locked); + } + else { + PyThread_init_thread(); + // XXX per-interpreter GIL: switch to interp->ceval._gil. + init_own_gil(tstate->interp, &tstate->interp->runtime->ceval.gil); + locked = 0; + } + if (!locked) { + take_gil(tstate); } - assert(own_gil); - - assert(!gil_created(gil)); - PyThread_init_thread(); - create_gil(gil); - assert(gil_created(gil)); - tstate->interp->ceval.gil = gil; - tstate->interp->ceval.own_gil = 1; - take_gil(tstate); return _PyStatus_OK(); } From 85702ae366068db7923dca2190f665e970f6ab60 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 6 May 2023 10:34:06 -0600 Subject: [PATCH 4/7] Fix a typo. --- Python/ceval_gil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index adf8e7b365895c..56122183c2bde8 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -541,7 +541,7 @@ _PyEval_InitGIL(PyThreadState *tstate, int own_gil) } else { PyThread_init_thread(); - // XXX per-interpreter GIL: switch to interp->ceval._gil. + // XXX per-interpreter GIL: switch to interp->_gil. init_own_gil(tstate->interp, &tstate->interp->runtime->ceval.gil); locked = 0; } From 3cc9dc3d8382aa4d84017b6f2adfa9e23e9cdf01 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 6 May 2023 10:36:12 -0600 Subject: [PATCH 5/7] Do not release the GIL when not necessary. --- Python/pylifecycle.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 1dd6ce2c7ddae7..818df0fddd0874 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2031,10 +2031,14 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) PyThreadState *save_tstate = _PyThreadState_SwapNoGIL(tstate); int has_gil = 0; + /* From this point until the init_interp_create_gil() call, + we must not do anything that requires that the GIL be held + (or otherwise exist). That applies whether or not the new + interpreter has its own GIL (e.g. the main interpreter). */ + /* Copy the current interpreter config into the new interpreter */ const PyConfig *src_config; if (save_tstate != NULL) { - _PyEval_ReleaseLock(save_tstate); src_config = _PyInterpreterState_GetConfig(save_tstate->interp); } else @@ -2044,11 +2048,13 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) src_config = _PyInterpreterState_GetConfig(main_interp); } + /* This does not require that the GIL be held. */ status = _PyConfig_Copy(&interp->config, src_config); if (_PyStatus_EXCEPTION(status)) { goto error; } + /* This does not require that the GIL be held. */ status = init_interp_settings(interp, config); if (_PyStatus_EXCEPTION(status)) { goto error; From 3104796f16f3a6e25340fa9247e532d8863ff283 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 6 May 2023 14:17:07 -0600 Subject: [PATCH 6/7] Make sure the GIL is held by the current thread in _PyEval_InitGIL(). --- Python/ceval_gil.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 56122183c2bde8..8c47fb7f7a0014 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -499,6 +499,15 @@ PyEval_ThreadsInitialized(void) return _PyEval_ThreadsInitialized(); } +static inline int +current_thread_holds_gil(struct _gil_runtime_state *gil, PyThreadState *tstate) +{ + if (((PyThreadState*)_Py_atomic_load_relaxed(&gil->last_holder)) != tstate) { + return 0; + } + return _Py_atomic_load_relaxed(&gil->locked); +} + static void init_shared_gil(PyInterpreterState *interp, struct _gil_runtime_state *gil) { @@ -525,8 +534,9 @@ _PyEval_InitGIL(PyThreadState *tstate, int own_gil) if (!own_gil) { PyInterpreterState *main_interp = _PyInterpreterState_Main(); assert(tstate->interp != main_interp); - init_shared_gil(tstate->interp, main_interp->ceval.gil); - locked = _Py_atomic_load_relaxed(&main_interp->ceval.gil->locked); + struct _gil_runtime_state *gil = main_interp->ceval.gil; + init_shared_gil(tstate->interp, gil); + locked = current_thread_holds_gil(gil, tstate); } /* XXX per-interpreter GIL */ else if (!_Py_IsMainInterpreter(tstate->interp)) { @@ -537,7 +547,7 @@ _PyEval_InitGIL(PyThreadState *tstate, int own_gil) init_shared_gil(tstate->interp, main_gil); // XXX For now we lie. tstate->interp->ceval.own_gil = 1; - locked = _Py_atomic_load_relaxed(&main_gil->locked); + locked = current_thread_holds_gil(main_gil, tstate); } else { PyThread_init_thread(); From 56064c7973aad97e9c5fe745796e62d46ed6442c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 6 May 2023 15:28:44 -0600 Subject: [PATCH 7/7] Release the GIL after swapping thread states. --- Python/pylifecycle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 818df0fddd0874..61f87c5eba60ed 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2039,6 +2039,8 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) /* Copy the current interpreter config into the new interpreter */ const PyConfig *src_config; if (save_tstate != NULL) { + // XXX Might new_interpreter() have been called without the GIL held? + _PyEval_ReleaseLock(save_tstate); src_config = _PyInterpreterState_GetConfig(save_tstate->interp); } else