From e697b37a14e6e236fc59448b5e552e66ac48092f Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 26 Mar 2024 20:12:39 +0000 Subject: [PATCH 1/2] gh-110481: Fix biased reference counting queue initialization. The biased reference counting queue must be initialized from the bound (active) thread because it uses `_Py_ThreadId()` as the key in a hash table. --- Python/brc.c | 2 ++ Python/pystate.c | 10 ++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Python/brc.c b/Python/brc.c index b73c721e71aef6..ca430875fafea4 100644 --- a/Python/brc.c +++ b/Python/brc.c @@ -119,6 +119,8 @@ _Py_brc_merge_refcounts(PyThreadState *tstate) struct _brc_thread_state *brc = &((_PyThreadStateImpl *)tstate)->brc; struct _brc_bucket *bucket = get_bucket(tstate->interp, brc->tid); + assert(brc->tid == _Py_ThreadId()); + // Append all objects into a local stack. We don't want to hold the lock // while calling destructors. PyMutex_Lock(&bucket->mutex); diff --git a/Python/pystate.c b/Python/pystate.c index 921e74ed5a9826..8489f53c6e3e34 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -261,6 +261,12 @@ bind_tstate(PyThreadState *tstate) tstate->native_thread_id = PyThread_get_thread_native_id(); #endif +#ifdef Py_GIL_DISABLED + // Initialize biased reference counting inter-thread queue. Note that this + // needs to be initialized from the active thread. + _Py_brc_init_thread(tstate); +#endif + // mimalloc state needs to be initialized from the active thread. tstate_mimalloc_bind(tstate); @@ -1412,10 +1418,6 @@ init_threadstate(_PyThreadStateImpl *_tstate, tstate->what_event = -1; tstate->previous_executor = NULL; -#ifdef Py_GIL_DISABLED - // Initialize biased reference counting inter-thread queue - _Py_brc_init_thread(tstate); -#endif llist_init(&_tstate->mem_free_queue); if (interp->stoptheworld.requested || _PyRuntime.stoptheworld.requested) { From 87f390c783e38b4349aa235ea828410023008d46 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 27 Mar 2024 21:48:52 +0000 Subject: [PATCH 2/2] Fix `_Py_brc_remove_thread()`. In applications that fork(), the thread state may be initialized, but not yet bound. That means the brc state may not yet be initialized when we call `PyThreadState_Clear()` after fork. --- Python/brc.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Python/brc.c b/Python/brc.c index ca430875fafea4..8f87bc33007bcf 100644 --- a/Python/brc.c +++ b/Python/brc.c @@ -144,11 +144,12 @@ void _Py_brc_init_thread(PyThreadState *tstate) { struct _brc_thread_state *brc = &((_PyThreadStateImpl *)tstate)->brc; - brc->tid = _Py_ThreadId(); + uintptr_t tid = _Py_ThreadId(); // Add ourself to the hashtable - struct _brc_bucket *bucket = get_bucket(tstate->interp, brc->tid); + struct _brc_bucket *bucket = get_bucket(tstate->interp, tid); PyMutex_Lock(&bucket->mutex); + brc->tid = tid; llist_insert_tail(&bucket->root, &brc->bucket_node); PyMutex_Unlock(&bucket->mutex); } @@ -157,6 +158,13 @@ void _Py_brc_remove_thread(PyThreadState *tstate) { struct _brc_thread_state *brc = &((_PyThreadStateImpl *)tstate)->brc; + if (brc->tid == 0) { + // The thread state may have been created, but never bound to a native + // thread and therefore never added to the hashtable. + assert(tstate->_status.bound == 0); + return; + } + struct _brc_bucket *bucket = get_bucket(tstate->interp, brc->tid); // We need to fully process any objects to merge before removing ourself