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.
python · jbower-fb · May 5, 2023 · May 5, 2023 · May 5, 2023 · May 5, 2023
commit 74d38eaae640eb1b3bc9cc2154bd7aed1c3fce6e
diff --git 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);
 

@@ -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");
         }
     }

@@ -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);

@@ -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,13 +1881,41 @@ _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;
 #endif
     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)
 {