From 1d219569859572dcae0776cff039215dae63353f Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Tue, 7 May 2024 15:48:32 -0700 Subject: [PATCH 1/7] Don't drop the GIL during thread deletion unless the current thread holds it --- Lib/test/test_importlib/test_threaded_import.py | 5 +---- Python/pystate.c | 13 ++++++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_importlib/test_threaded_import.py b/Lib/test/test_importlib/test_threaded_import.py index 3477112927a0b6..9af1e4d505c66e 100644 --- a/Lib/test/test_importlib/test_threaded_import.py +++ b/Lib/test/test_importlib/test_threaded_import.py @@ -17,7 +17,7 @@ from test.support import verbose from test.support.import_helper import forget, mock_register_at_fork from test.support.os_helper import (TESTFN, unlink, rmtree) -from test.support import script_helper, threading_helper, requires_gil_enabled +from test.support import script_helper, threading_helper threading_helper.requires_working_threading(module=True) @@ -248,9 +248,6 @@ def test_concurrent_futures_circular_import(self): 'partial', 'cfimport.py') script_helper.assert_python_ok(fn) - # gh-118727 and gh-118729: pool_in_threads.py may crash in free-threaded - # builds, which can hang the Tsan test so temporarily skip it for now. - @requires_gil_enabled("gh-118727: test may crash in free-threaded builds") def test_multiprocessing_pool_circular_import(self): # Regression test for bpo-41567 fn = os.path.join(os.path.dirname(__file__), diff --git a/Python/pystate.c b/Python/pystate.c index b1e085bb806915..2a5e96bcedc17f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1831,10 +1831,21 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) _Py_EnsureTstateNotNULL(tstate); #ifdef Py_GIL_DISABLED _Py_qsbr_detach(((_PyThreadStateImpl *)tstate)->qsbr); + // tstate_delete_common() removes tstate from consideration for + // stop-the-worlds. This means that another thread could enable the GIL + // before our call to _PyEval_ReleaseLock(), violating its invariant that + // the calling thread holds the GIL if and only if the GIL is enabled. Deal + // with this by deciding if we need to release the GIL before + // tstate_delete_common(), when the invariant is still true. + int holds_gil = _PyEval_IsGILEnabled(tstate); +#else + int holds_gil = 1; #endif current_fast_clear(tstate->interp->runtime); tstate_delete_common(tstate); - _PyEval_ReleaseLock(tstate->interp, NULL); + if (holds_gil) { + _PyEval_ReleaseLock(tstate->interp, NULL); + } free_threadstate((_PyThreadStateImpl *)tstate); } From b91a1d59862b1a95d098574347d5b45d62265f87 Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Wed, 8 May 2024 12:09:21 -0700 Subject: [PATCH 2/7] Store holds_gil flag on PyThreadState instead, and use it where appropriate --- Include/cpython/pystate.h | 8 +++- Include/internal/pycore_ceval.h | 7 ++- Python/ceval_gil.c | 82 ++++++++++++++++++--------------- Python/pystate.c | 23 ++------- 4 files changed, 61 insertions(+), 59 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 2df9ecd6d52084..67a09d29207130 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -83,6 +83,12 @@ struct _ts { unsigned int bound_gilstate:1; /* Currently in use (maybe holds the GIL). */ unsigned int active:1; +#ifdef Py_GIL_DISABLED + /* Currently holds the GIL. */ + unsigned int holds_gil:1; +#else + unsigned int _unused:1; +#endif /* various stages of finalization */ unsigned int finalizing:1; @@ -90,7 +96,7 @@ struct _ts { unsigned int finalized:1; /* padding to align to 4 bytes */ - unsigned int :24; + unsigned int :23; } _status; #ifdef Py_BUILD_CORE # define _PyThreadState_WHENCE_NOTSET -1 diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 48ad0678995904..87fae1bb4790bc 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -131,11 +131,10 @@ extern int _PyEval_ThreadsInitialized(void); extern void _PyEval_InitGIL(PyThreadState *tstate, int own_gil); extern void _PyEval_FiniGIL(PyInterpreterState *interp); -// 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_AcquireLock(PyThreadState *tstate); -extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *); +extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *, + int thread_dying); #ifdef Py_GIL_DISABLED // Returns 0 or 1 if the GIL for the given thread's interpreter is disabled or diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 7a54c185303cd1..72cc11caed5b50 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -205,32 +205,36 @@ static void recreate_gil(struct _gil_runtime_state *gil) } #endif -static void -drop_gil_impl(struct _gil_runtime_state *gil) +static inline void +drop_gil_impl(PyThreadState *tstate, 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); +#ifdef Py_GIL_DISABLED + tstate->_status.holds_gil = 0; +#endif COND_SIGNAL(gil->cond); MUTEX_UNLOCK(gil->mutex); } static void -drop_gil(PyInterpreterState *interp, PyThreadState *tstate) +drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) { struct _ceval_state *ceval = &interp->ceval; - /* If tstate is NULL, the caller is indicating that we're releasing + /* If thread_dying is true, the caller is indicating that we're releasing the GIL for the last time in this thread. This is particularly relevant when the current thread state is finalizing or its interpreter is finalizing (either may be in an inconsistent state). In that case the current thread will definitely never try to acquire the GIL again. */ // XXX It may be more correct to check tstate->_status.finalizing. - // XXX assert(tstate == NULL || !tstate->_status.cleared); + // XXX assert(thread_dying || !tstate->_status.cleared); + assert(tstate != NULL); struct _gil_runtime_state *gil = ceval->gil; #ifdef Py_GIL_DISABLED - if (!_Py_atomic_load_int_relaxed(&gil->enabled)) { + if (!tstate->_status.holds_gil) { return; } #endif @@ -238,26 +242,23 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate) Py_FatalError("drop_gil: GIL is not locked"); } - /* tstate is allowed to be NULL (early interpreter init) */ - if (tstate != NULL) { + if (!thread_dying) { /* Sub-interpreter support: threads might have been switched under our feet using PyThreadState_Swap(). Fix the GIL last holder variable so that our heuristics work. */ _Py_atomic_store_ptr_relaxed(&gil->last_holder, tstate); } - drop_gil_impl(gil); + drop_gil_impl(tstate, gil); #ifdef FORCE_SWITCHING - /* We check tstate first in case we might be releasing the GIL for - the last time in this thread. In that case there's a possible - race with tstate->interp getting deleted after gil->mutex is - unlocked and before the following code runs, leading to a crash. - We can use (tstate == NULL) to indicate the thread is done with - the GIL, and that's the only time we might delete the - interpreter, so checking tstate first prevents the crash. - See https://github.com/python/cpython/issues/104341. */ - if (tstate != NULL && + /* We might be releasing the GIL for the last time in this thread. In that + case there's a possible race with tstate->interp getting deleted after + gil->mutex is unlocked and before the following code runs, leading to a + crash. We can use thread_dying to indicate the thread is done with the + GIL, and that's the only time we might delete the interpreter. See + https://github.com/python/cpython/issues/104341. */ + if (!thread_dying && _Py_eval_breaker_bit_is_set(tstate, _PY_GIL_DROP_REQUEST_BIT)) { MUTEX_LOCK(gil->switch_mutex); /* Not switched yet => wait */ @@ -284,7 +285,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate) tstate must be non-NULL. Returns 1 if the GIL was acquired, or 0 if not. */ -static int +static void take_gil(PyThreadState *tstate) { int err = errno; @@ -309,7 +310,7 @@ take_gil(PyThreadState *tstate) struct _gil_runtime_state *gil = interp->ceval.gil; #ifdef Py_GIL_DISABLED if (!_Py_atomic_load_int_relaxed(&gil->enabled)) { - return 0; + return; } #endif @@ -358,10 +359,10 @@ take_gil(PyThreadState *tstate) 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. + // return. COND_SIGNAL(gil->cond); MUTEX_UNLOCK(gil->mutex); - return 0; + return; } #endif @@ -371,6 +372,9 @@ take_gil(PyThreadState *tstate) MUTEX_LOCK(gil->switch_mutex); #endif /* We now hold the GIL */ +#ifdef Py_GIL_DISABLED + tstate->_status.holds_gil = 1; +#endif _Py_atomic_store_int_relaxed(&gil->locked, 1); _Py_ANNOTATE_RWLOCK_ACQUIRED(&gil->locked, /*is_write=*/1); @@ -393,9 +397,7 @@ take_gil(PyThreadState *tstate) in take_gil() while the main thread called wait_for_thread_shutdown() from Py_Finalize(). */ MUTEX_UNLOCK(gil->mutex); - /* Passing NULL to drop_gil() indicates that this thread is about to - terminate and will never hold the GIL again. */ - drop_gil(interp, NULL); + drop_gil(interp, tstate, 1); PyThread_exit_thread(); } assert(_PyThreadState_CheckConsistency(tstate)); @@ -406,7 +408,7 @@ take_gil(PyThreadState *tstate) MUTEX_UNLOCK(gil->mutex); errno = err; - return 1; + return; } void _PyEval_SetSwitchInterval(unsigned long microseconds) @@ -452,9 +454,16 @@ static inline int current_thread_holds_gil(struct _gil_runtime_state *gil, PyThreadState *tstate) { if (((PyThreadState*)_Py_atomic_load_ptr_relaxed(&gil->last_holder)) != tstate) { +#ifdef Py_GIL_DISABLED + assert(!tstate->_status.holds_gil); +#endif return 0; } - return _Py_atomic_load_int_relaxed(&gil->locked); + int locked = _Py_atomic_load_int_relaxed(&gil->locked); +#ifdef Py_GIL_DISABLED + assert(!tstate->_status.holds_gil || locked); +#endif + return locked; } #endif @@ -563,23 +572,24 @@ PyEval_ReleaseLock(void) /* This function must succeed when the current thread state is NULL. We therefore avoid PyThreadState_Get() which dumps a fatal error in debug mode. */ - drop_gil(tstate->interp, tstate); + drop_gil(tstate->interp, tstate, 0); } -int +void _PyEval_AcquireLock(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - return take_gil(tstate); + take_gil(tstate); } void -_PyEval_ReleaseLock(PyInterpreterState *interp, PyThreadState *tstate) +_PyEval_ReleaseLock(PyInterpreterState *interp, + PyThreadState *tstate, + int thread_dying) { - /* If tstate is NULL then we do not expect the current thread - to acquire the GIL ever again. */ - assert(tstate == NULL || tstate->interp == interp); - drop_gil(interp, tstate); + assert(tstate != NULL); + assert(tstate->interp == interp); + drop_gil(interp, tstate, thread_dying); } void @@ -1136,7 +1146,7 @@ _PyEval_DisableGIL(PyThreadState *tstate) // // 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); + drop_gil_impl(tstate, gil); return 1; } return 0; diff --git a/Python/pystate.c b/Python/pystate.c index 2a5e96bcedc17f..fb04b0c428cd76 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1831,21 +1831,10 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) _Py_EnsureTstateNotNULL(tstate); #ifdef Py_GIL_DISABLED _Py_qsbr_detach(((_PyThreadStateImpl *)tstate)->qsbr); - // tstate_delete_common() removes tstate from consideration for - // stop-the-worlds. This means that another thread could enable the GIL - // before our call to _PyEval_ReleaseLock(), violating its invariant that - // the calling thread holds the GIL if and only if the GIL is enabled. Deal - // with this by deciding if we need to release the GIL before - // tstate_delete_common(), when the invariant is still true. - int holds_gil = _PyEval_IsGILEnabled(tstate); -#else - int holds_gil = 1; #endif current_fast_clear(tstate->interp->runtime); tstate_delete_common(tstate); - if (holds_gil) { - _PyEval_ReleaseLock(tstate->interp, NULL); - } + _PyEval_ReleaseLock(tstate->interp, tstate, 1); free_threadstate((_PyThreadStateImpl *)tstate); } @@ -2070,7 +2059,7 @@ _PyThreadState_Attach(PyThreadState *tstate) while (1) { - int acquired_gil = _PyEval_AcquireLock(tstate); + _PyEval_AcquireLock(tstate); // XXX assert(tstate_is_alive(tstate)); current_fast_set(&_PyRuntime, tstate); @@ -2081,20 +2070,18 @@ _PyThreadState_Attach(PyThreadState *tstate) } #ifdef Py_GIL_DISABLED - if (_PyEval_IsGILEnabled(tstate) != acquired_gil) { + if (_PyEval_IsGILEnabled(tstate) != tstate->_status.holds_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); + assert(!tstate->_status.holds_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; } @@ -2125,7 +2112,7 @@ detach_thread(PyThreadState *tstate, int detached_state) tstate_deactivate(tstate); tstate_set_detached(tstate, detached_state); current_fast_clear(&_PyRuntime); - _PyEval_ReleaseLock(tstate->interp, tstate); + _PyEval_ReleaseLock(tstate->interp, tstate, 0); } void From 29c853d520b81105ff62b5e61e6d04d5d3630ecf Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Wed, 8 May 2024 13:22:28 -0700 Subject: [PATCH 3/7] Account for tstate == NULL case --- Python/ceval_gil.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 72cc11caed5b50..63743efc8050b8 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -212,7 +212,9 @@ drop_gil_impl(PyThreadState *tstate, struct _gil_runtime_state *gil) _Py_ANNOTATE_RWLOCK_RELEASED(&gil->locked, /*is_write=*/1); _Py_atomic_store_int_relaxed(&gil->locked, 0); #ifdef Py_GIL_DISABLED - tstate->_status.holds_gil = 0; + if (tstate != NULL) { + tstate->_status.holds_gil = 0; + } #endif COND_SIGNAL(gil->cond); MUTEX_UNLOCK(gil->mutex); @@ -231,10 +233,13 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) // XXX It may be more correct to check tstate->_status.finalizing. // XXX assert(thread_dying || !tstate->_status.cleared); - assert(tstate != NULL); + assert(thread_dying || tstate != NULL); struct _gil_runtime_state *gil = ceval->gil; #ifdef Py_GIL_DISABLED - if (!tstate->_status.holds_gil) { + // Check if we have the GIL before dropping it. tstate will be NULL if + // take_gil() detected that this thread has been destroyed, in which case + // we know we have the GIL. + if (tstate != NULL && !tstate->_status.holds_gil) { return; } #endif @@ -397,7 +402,7 @@ take_gil(PyThreadState *tstate) in take_gil() while the main thread called wait_for_thread_shutdown() from Py_Finalize(). */ MUTEX_UNLOCK(gil->mutex); - drop_gil(interp, tstate, 1); + drop_gil(interp, NULL, 1); PyThread_exit_thread(); } assert(_PyThreadState_CheckConsistency(tstate)); From 694926fac8fce7067260a967b8c93769277c2dce Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Wed, 8 May 2024 16:25:42 -0700 Subject: [PATCH 4/7] Always track holds_gil, related cleanup --- Include/cpython/pystate.h | 4 ---- Python/ceval_gil.c | 26 ++++++++++++-------------- Python/pystate.c | 3 +-- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 67a09d29207130..ed3ee090ae53db 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -83,12 +83,8 @@ struct _ts { unsigned int bound_gilstate:1; /* Currently in use (maybe holds the GIL). */ unsigned int active:1; -#ifdef Py_GIL_DISABLED /* Currently holds the GIL. */ unsigned int holds_gil:1; -#else - unsigned int _unused:1; -#endif /* various stages of finalization */ unsigned int finalizing:1; diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 63743efc8050b8..de30215ac0ada6 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -211,11 +211,9 @@ drop_gil_impl(PyThreadState *tstate, 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); -#ifdef Py_GIL_DISABLED if (tstate != NULL) { tstate->_status.holds_gil = 0; } -#endif COND_SIGNAL(gil->cond); MUTEX_UNLOCK(gil->mutex); } @@ -377,9 +375,7 @@ take_gil(PyThreadState *tstate) MUTEX_LOCK(gil->switch_mutex); #endif /* We now hold the GIL */ -#ifdef Py_GIL_DISABLED tstate->_status.holds_gil = 1; -#endif _Py_atomic_store_int_relaxed(&gil->locked, 1); _Py_ANNOTATE_RWLOCK_ACQUIRED(&gil->locked, /*is_write=*/1); @@ -402,6 +398,8 @@ take_gil(PyThreadState *tstate) in take_gil() while the main thread called wait_for_thread_shutdown() from Py_Finalize(). */ MUTEX_UNLOCK(gil->mutex); + /* tstate could be a dangling pointer, so don't pass it to + drop_gil(). */ drop_gil(interp, NULL, 1); PyThread_exit_thread(); } @@ -458,17 +456,17 @@ PyEval_ThreadsInitialized(void) static inline int current_thread_holds_gil(struct _gil_runtime_state *gil, PyThreadState *tstate) { - if (((PyThreadState*)_Py_atomic_load_ptr_relaxed(&gil->last_holder)) != tstate) { -#ifdef Py_GIL_DISABLED - assert(!tstate->_status.holds_gil); -#endif - return 0; - } + int holds_gil = tstate->_status.holds_gil; + + // holds_gil is the source of truth; check that last_holder and gil->locked + // are consistent with it. int locked = _Py_atomic_load_int_relaxed(&gil->locked); -#ifdef Py_GIL_DISABLED - assert(!tstate->_status.holds_gil || locked); -#endif - return locked; + int is_last_holder = + ((PyThreadState*)_Py_atomic_load_ptr_relaxed(&gil->last_holder)) == tstate; + assert(!holds_gil || locked); + assert(!holds_gil || is_last_holder); + + return holds_gil; } #endif diff --git a/Python/pystate.c b/Python/pystate.c index fb04b0c428cd76..cf586fc95f6c41 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2070,12 +2070,11 @@ _PyThreadState_Attach(PyThreadState *tstate) } #ifdef Py_GIL_DISABLED - if (_PyEval_IsGILEnabled(tstate) != tstate->_status.holds_gil) { + if (_PyEval_IsGILEnabled(tstate) && !tstate->_status.holds_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(!tstate->_status.holds_gil); tstate_set_detached(tstate, _Py_THREAD_DETACHED); tstate_deactivate(tstate); current_fast_clear(&_PyRuntime); From 480c07d47e21a82e5073e709aec917a4b085bcfe Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Wed, 8 May 2024 17:12:19 -0700 Subject: [PATCH 5/7] Fix ASAN error, pull drop_gil_impl() back into drop_gil() --- Python/ceval_gil.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index de30215ac0ada6..291b5307e0e573 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -205,19 +205,6 @@ static void recreate_gil(struct _gil_runtime_state *gil) } #endif -static inline void -drop_gil_impl(PyThreadState *tstate, 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); - if (tstate != NULL) { - tstate->_status.holds_gil = 0; - } - COND_SIGNAL(gil->cond); - MUTEX_UNLOCK(gil->mutex); -} - static void drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) { @@ -252,7 +239,14 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) _Py_atomic_store_ptr_relaxed(&gil->last_holder, tstate); } - drop_gil_impl(tstate, gil); + MUTEX_LOCK(gil->mutex); + _Py_ANNOTATE_RWLOCK_RELEASED(&gil->locked, /*is_write=*/1); + _Py_atomic_store_int_relaxed(&gil->locked, 0); + if (tstate != NULL) { + tstate->_status.holds_gil = 0; + } + COND_SIGNAL(gil->cond); + MUTEX_UNLOCK(gil->mutex); #ifdef FORCE_SWITCHING /* We might be releasing the GIL for the last time in this thread. In that @@ -375,7 +369,6 @@ take_gil(PyThreadState *tstate) MUTEX_LOCK(gil->switch_mutex); #endif /* We now hold the GIL */ - tstate->_status.holds_gil = 1; _Py_atomic_store_int_relaxed(&gil->locked, 1); _Py_ANNOTATE_RWLOCK_ACQUIRED(&gil->locked, /*is_write=*/1); @@ -405,6 +398,7 @@ take_gil(PyThreadState *tstate) } assert(_PyThreadState_CheckConsistency(tstate)); + tstate->_status.holds_gil = 1; _Py_unset_eval_breaker_bit(tstate, _PY_GIL_DROP_REQUEST_BIT); update_eval_breaker_for_thread(interp, tstate); @@ -1149,7 +1143,7 @@ _PyEval_DisableGIL(PyThreadState *tstate) // // Drop the GIL, which will wake up any threads waiting in take_gil() // and let them resume execution without the GIL. - drop_gil_impl(tstate, gil); + drop_gil(tstate->interp, tstate, 0); return 1; } return 0; From d28bf133656f5f729ef6330c9dcd0cd3b81f112c Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Thu, 9 May 2024 13:26:10 -0700 Subject: [PATCH 6/7] Rename thread_dying to final_release --- Include/internal/pycore_ceval.h | 2 +- Python/ceval_gil.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 87fae1bb4790bc..bd3ba1225f2597 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -134,7 +134,7 @@ extern void _PyEval_FiniGIL(PyInterpreterState *interp); extern void _PyEval_AcquireLock(PyThreadState *tstate); extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *, - int thread_dying); + int final_release); #ifdef Py_GIL_DISABLED // Returns 0 or 1 if the GIL for the given thread's interpreter is disabled or diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 291b5307e0e573..16b5b8b947d4ba 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -206,19 +206,19 @@ static void recreate_gil(struct _gil_runtime_state *gil) #endif static void -drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) +drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release) { struct _ceval_state *ceval = &interp->ceval; - /* If thread_dying is true, the caller is indicating that we're releasing + /* If final_release is true, the caller is indicating that we're releasing the GIL for the last time in this thread. This is particularly relevant when the current thread state is finalizing or its interpreter is finalizing (either may be in an inconsistent state). In that case the current thread will definitely never try to acquire the GIL again. */ // XXX It may be more correct to check tstate->_status.finalizing. - // XXX assert(thread_dying || !tstate->_status.cleared); + // XXX assert(final_release || !tstate->_status.cleared); - assert(thread_dying || tstate != NULL); + assert(final_release || tstate != NULL); struct _gil_runtime_state *gil = ceval->gil; #ifdef Py_GIL_DISABLED // Check if we have the GIL before dropping it. tstate will be NULL if @@ -232,7 +232,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) Py_FatalError("drop_gil: GIL is not locked"); } - if (!thread_dying) { + if (!final_release) { /* Sub-interpreter support: threads might have been switched under our feet using PyThreadState_Swap(). Fix the GIL last holder variable so that our heuristics work. */ @@ -252,10 +252,10 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int thread_dying) /* We might be releasing the GIL for the last time in this thread. In that case there's a possible race with tstate->interp getting deleted after gil->mutex is unlocked and before the following code runs, leading to a - crash. We can use thread_dying to indicate the thread is done with the + crash. We can use final_release to indicate the thread is done with the GIL, and that's the only time we might delete the interpreter. See https://github.com/python/cpython/issues/104341. */ - if (!thread_dying && + if (!final_release && _Py_eval_breaker_bit_is_set(tstate, _PY_GIL_DROP_REQUEST_BIT)) { MUTEX_LOCK(gil->switch_mutex); /* Not switched yet => wait */ @@ -582,11 +582,11 @@ _PyEval_AcquireLock(PyThreadState *tstate) void _PyEval_ReleaseLock(PyInterpreterState *interp, PyThreadState *tstate, - int thread_dying) + int final_release) { assert(tstate != NULL); assert(tstate->interp == interp); - drop_gil(interp, tstate, thread_dying); + drop_gil(interp, tstate, final_release); } void From d056a46b35544bafb9c9223031fb4be2a08dfc9f Mon Sep 17 00:00:00 2001 From: Brett Simmers Date: Mon, 13 May 2024 11:29:49 -0700 Subject: [PATCH 7/7] Restore drop_gil_impl(), clear GIL drop request when disabling GIL --- Python/ceval_gil.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 16b5b8b947d4ba..5617504a495686 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -205,6 +205,19 @@ static void recreate_gil(struct _gil_runtime_state *gil) } #endif +static inline void +drop_gil_impl(PyThreadState *tstate, 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); + if (tstate != NULL) { + tstate->_status.holds_gil = 0; + } + COND_SIGNAL(gil->cond); + MUTEX_UNLOCK(gil->mutex); +} + static void drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release) { @@ -239,14 +252,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release) _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); - if (tstate != NULL) { - tstate->_status.holds_gil = 0; - } - COND_SIGNAL(gil->cond); - MUTEX_UNLOCK(gil->mutex); + drop_gil_impl(tstate, gil); #ifdef FORCE_SWITCHING /* We might be releasing the GIL for the last time in this thread. In that @@ -1143,7 +1149,12 @@ _PyEval_DisableGIL(PyThreadState *tstate) // // Drop the GIL, which will wake up any threads waiting in take_gil() // and let them resume execution without the GIL. - drop_gil(tstate->interp, tstate, 0); + drop_gil_impl(tstate, gil); + + // If another thread asked us to drop the GIL, they should be + // free-threading by now. Remove any such request so we have a clean + // slate if/when the GIL is enabled again. + _Py_unset_eval_breaker_bit(tstate, _PY_GIL_DROP_REQUEST_BIT); return 1; } return 0;