[Python-checkins] gh-99113: Make Sure the GIL is Acquired at the Right Places (gh-104208)
ericsnowcurrently
webhook-mailer at python.org
Sat May 6 17:59:37 EDT 2023
https://github.com/python/cpython/commit/92d8bfffbf377e91d8b92666525cb8700bb1d5e8
commit: 92d8bfffbf377e91d8b92666525cb8700bb1d5e8
branch: main
author: Eric Snow <ericsnowcurrently at gmail.com>
committer: ericsnowcurrently <ericsnowcurrently at gmail.com>
date: 2023-05-06T15:59:30-06:00
summary:
gh-99113: Make Sure the GIL is Acquired at the Right Places (gh-104208)
This is a pre-requisite for a per-interpreter GIL. Without it this change isn't strictly necessary. However, there is no real downside otherwise.
files:
M Include/internal/pycore_ceval.h
M Python/ceval_gil.c
M Python/pylifecycle.c
M Python/pystate.c
diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h
index b7a9bf40425b..9fd8571cbc87 100644
--- a/Include/internal/pycore_ceval.h
+++ b/Include/internal/pycore_ceval.h
@@ -100,7 +100,9 @@ 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 PyThreadState * _PyThreadState_SwapNoGIL(PyThreadState *);
extern void _PyEval_DeactivateOpCache(void);
diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c
index 1ac0dbcf2ecf..9958856bae80 100644
--- a/Python/ceval_gil.c
+++ b/Python/ceval_gil.c
@@ -499,42 +499,66 @@ 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)
+{
+ 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, gil);
+ locked = current_thread_holds_gil(gil, tstate);
}
-
/* 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 = current_thread_holds_gil(main_gil, tstate);
+ }
+ else {
+ PyThread_init_thread();
+ // XXX per-interpreter GIL: switch to interp->_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();
}
@@ -611,9 +635,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);
}
@@ -625,7 +657,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");
}
}
@@ -635,8 +667,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");
}
@@ -684,8 +715,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;
@@ -701,7 +731,7 @@ PyEval_RestoreThread(PyThreadState *tstate)
take_gil(tstate);
- _PyThreadState_Swap(tstate->interp->runtime, tstate);
+ _PyThreadState_SwapNoGIL(tstate);
}
@@ -1005,7 +1035,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);
@@ -1014,7 +1044,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 705708698c6a..61f87c5eba60 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,20 @@ 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;
+
+ /* 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) {
+ // XXX Might new_interpreter() have been called without the GIL held?
+ _PyEval_ReleaseLock(save_tstate);
src_config = _PyInterpreterState_GetConfig(save_tstate->interp);
}
else
@@ -2039,11 +2050,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;
@@ -2053,6 +2066,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 +2086,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 75bd9f41e301..f14934361dab 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)
{
More information about the Python-checkins
mailing list