Skip to content

gh-114940: Use fine-grained mutex protection for PyInterpreterState.threads #125561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
414cfef
Add threads.mutex member to PyInterpreState
rruuaanng Oct 16, 2024
0c815fc
Add NEWS
rruuaanng Oct 16, 2024
f5ec949
Delete Misc/NEWS.d/next/Core_and_Builtins/2024-10-16-11-07-50.gh-issu…
rruuaanng Oct 16, 2024
b595153
Change to current thread mutex
rruuaanng Oct 17, 2024
2378caa
Remove runtime variable
rruuaanng Oct 17, 2024
9cb2de0
HEAD_LOCK change to INTERP_THREAD_LOCK
rruuaanng Oct 17, 2024
afbaf04
Merge branch 'main' into _gh114940
rruuaanng Oct 17, 2024
29c4ccb
HEAD_LOCK change to INTERP_THREAD_LOCK
rruuaanng Oct 17, 2024
462f97d
HEAD_LOCK change to INTERP_THREAD_LOCK
rruuaanng Oct 18, 2024
8146b6f
Remove unused variable
rruuaanng Oct 18, 2024
74cc060
Clear unused code
rruuaanng Oct 18, 2024
f30b2b1
Change comment
rruuaanng Oct 19, 2024
8e0d324
INTERP_THREAD_LOCK rename to INTERP_HEAD_LOCK
rruuaanng Oct 19, 2024
1f488c7
INTERP_THREAD_LOCK rename to INTERP_HEAD_LOCK
rruuaanng Oct 19, 2024
c99e44e
INTERP_THREAD_LOCK rename to INTERP_HEAD_LOCK
rruuaanng Oct 19, 2024
7ad9c32
Using raw member
rruuaanng Oct 19, 2024
2131d69
Fix possible race conditions
rruuaanng Oct 19, 2024
7bf9a1c
INTERP_THREAD_LOCK rename to INTERP_HEAD_LOCK
rruuaanng Oct 19, 2024
b947bc9
Rollback to unmodified
rruuaanng Oct 20, 2024
46c2709
Use variable
rruuaanng Oct 20, 2024
3c208c2
Recover assert
rruuaanng Oct 24, 2024
2bb60d9
Add mutex for get_mimalloc_allocated_blocks
rruuaanng Oct 24, 2024
3798bed
Add mutex for get_reftotal
rruuaanng Oct 24, 2024
5dc1ced
Merge branch 'main' into _gh114940
rruuaanng Nov 2, 2024
18fbaa0
Merge branch 'main' into _gh114940
rruuaanng Nov 20, 2024
6257837
add lock to codeobject
rruuaanng Nov 20, 2024
4b9a569
clear ci warning
rruuaanng Nov 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Include/internal/pycore_interp.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ struct _is {
or the size specified by the THREAD_STACK_SIZE macro. */
/* Used in Python/thread.c. */
size_t stacksize;
/* The lock that protects this struct. */
PyMutex mutex;
} threads;

/* Reference to the _PyRuntime global variable. This field exists
Expand Down
5 changes: 5 additions & 0 deletions Include/internal/pycore_pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ extern int _PyOS_InterruptOccurred(PyThreadState *tstate);
#define HEAD_UNLOCK(runtime) \
PyMutex_Unlock(&(runtime)->interpreters.mutex)

#define INTERP_HEAD_LOCK(interpstate) \
PyMutex_LockFlags(&(interpstate)->threads.mutex, _Py_LOCK_DONT_DETACH)
#define INTERP_HEAD_UNLOCK(interpstate) \
PyMutex_Unlock(&(interpstate)->threads.mutex)

// Get the configuration of the current interpreter.
// The caller must hold the GIL.
// Export for test_peg_generator.
Expand Down
22 changes: 10 additions & 12 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -2376,20 +2376,19 @@ void
PyEval_SetProfileAllThreads(Py_tracefunc func, PyObject *arg)
{
PyThreadState *this_tstate = _PyThreadState_GET();
PyInterpreterState* interp = this_tstate->interp;
PyInterpreterState *interp = this_tstate->interp;

_PyRuntimeState *runtime = &_PyRuntime;
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
PyThreadState* ts = PyInterpreterState_ThreadHead(interp);
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);

while (ts) {
if (_PyEval_SetProfile(ts, func, arg) < 0) {
PyErr_FormatUnraisable("Exception ignored in PyEval_SetProfileAllThreads");
}
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
ts = PyThreadState_Next(ts);
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);
}
}

Expand All @@ -2407,20 +2406,19 @@ void
PyEval_SetTraceAllThreads(Py_tracefunc func, PyObject *arg)
{
PyThreadState *this_tstate = _PyThreadState_GET();
PyInterpreterState* interp = this_tstate->interp;
PyInterpreterState *interp = this_tstate->interp;

_PyRuntimeState *runtime = &_PyRuntime;
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
PyThreadState* ts = PyInterpreterState_ThreadHead(interp);
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);

while (ts) {
if (_PyEval_SetTrace(ts, func, arg) < 0) {
PyErr_FormatUnraisable("Exception ignored in PyEval_SetTraceAllThreads");
}
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
ts = PyThreadState_Next(ts);
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);
}
}

Expand Down
20 changes: 10 additions & 10 deletions Python/gc_free_threading.c
Original file line number Diff line number Diff line change
Expand Up @@ -344,9 +344,9 @@ gc_visit_heaps(PyInterpreterState *interp, mi_block_visit_fun *visitor,
assert(interp->stoptheworld.world_stopped);

int err;
HEAD_LOCK(&_PyRuntime);
INTERP_HEAD_LOCK(interp);
err = gc_visit_heaps_lock_held(interp, visitor, arg);
HEAD_UNLOCK(&_PyRuntime);
INTERP_HEAD_UNLOCK(interp);
return err;
}

Expand All @@ -368,7 +368,7 @@ gc_visit_stackref(_PyStackRef stackref)
static void
gc_visit_thread_stacks(PyInterpreterState *interp)
{
HEAD_LOCK(&_PyRuntime);
INTERP_HEAD_LOCK(interp);
for (PyThreadState *p = interp->threads.head; p != NULL; p = p->next) {
for (_PyInterpreterFrame *f = p->current_frame; f != NULL; f = f->previous) {
PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
Expand All @@ -384,7 +384,7 @@ gc_visit_thread_stacks(PyInterpreterState *interp)
}
}
}
HEAD_UNLOCK(&_PyRuntime);
INTERP_HEAD_UNLOCK(interp);
}

static void
Expand Down Expand Up @@ -423,14 +423,14 @@ process_delayed_frees(PyInterpreterState *interp)

// Merge the queues from other threads into our own queue so that we can
// process all of the pending delayed free requests at once.
HEAD_LOCK(&_PyRuntime);
INTERP_HEAD_LOCK(interp);
for (PyThreadState *p = interp->threads.head; p != NULL; p = p->next) {
_PyThreadStateImpl *other = (_PyThreadStateImpl *)p;
if (other != current_tstate) {
llist_concat(&current_tstate->mem_free_queue, &other->mem_free_queue);
}
}
HEAD_UNLOCK(&_PyRuntime);
INTERP_HEAD_UNLOCK(interp);

_PyMem_ProcessDelayed((PyThreadState *)current_tstate);
}
Expand Down Expand Up @@ -1217,7 +1217,7 @@ gc_collect_internal(PyInterpreterState *interp, struct collection_state *state,
state->gcstate->old[i-1].count = 0;
}

HEAD_LOCK(&_PyRuntime);
INTERP_HEAD_LOCK(interp);
for (PyThreadState *p = interp->threads.head; p != NULL; p = p->next) {
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)p;

Expand All @@ -1227,7 +1227,7 @@ gc_collect_internal(PyInterpreterState *interp, struct collection_state *state,
// merge refcounts for all queued objects
merge_queued_objects(tstate, state);
}
HEAD_UNLOCK(&_PyRuntime);
INTERP_HEAD_UNLOCK(interp);

process_delayed_frees(interp);

Expand Down Expand Up @@ -1985,13 +1985,13 @@ PyUnstable_GC_VisitObjects(gcvisitobjects_t callback, void *arg)
void
_PyGC_ClearAllFreeLists(PyInterpreterState *interp)
{
HEAD_LOCK(&_PyRuntime);
INTERP_HEAD_LOCK(interp);
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)interp->threads.head;
while (tstate != NULL) {
_PyObject_ClearFreeLists(&tstate->freelists, 0);
tstate = (_PyThreadStateImpl *)tstate->base.next;
}
HEAD_UNLOCK(&_PyRuntime);
INTERP_HEAD_UNLOCK(interp);
}

#endif // Py_GIL_DISABLED
5 changes: 2 additions & 3 deletions Python/instrumentation.c
Original file line number Diff line number Diff line change
Expand Up @@ -988,13 +988,12 @@ set_global_version(PyThreadState *tstate, uint32_t version)

#ifdef Py_GIL_DISABLED
// Set the version on all threads in free-threaded builds.
_PyRuntimeState *runtime = &_PyRuntime;
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
for (tstate = interp->threads.head; tstate;
tstate = PyThreadState_Next(tstate)) {
set_version_raw(&tstate->eval_breaker, version);
};
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);
#else
// Normal builds take the current version from instrumentation_version when
// attaching a thread, so we only have to set the current thread's version.
Expand Down
23 changes: 7 additions & 16 deletions Python/pystate.c
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,6 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
{
assert(interp != NULL);
assert(tstate != NULL);
_PyRuntimeState *runtime = interp->runtime;

/* XXX Conditions we need to enforce:

Expand All @@ -791,18 +790,16 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
}

// Clear the current/main thread state last.
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @ericsnowcurrently pointed this out in a different comment, but we should not change where the lock/unlocks happen in this PR.

The existing code has a race condition, but holding the lock across the PyThreadState_Clear() call risks deadlock (see the code comment below).

PyThreadState *p = interp->threads.head;
HEAD_UNLOCK(runtime);
while (p != NULL) {
// See https://github.com/python/cpython/issues/102126
// Must be called without HEAD_LOCK held as it can deadlock
// if any finalizer tries to acquire that lock.
PyThreadState_Clear(p);
HEAD_LOCK(runtime);
p = p->next;
HEAD_UNLOCK(runtime);
}
INTERP_HEAD_UNLOCK(interp);
if (tstate->interp == interp) {
/* We fix tstate->_status below when we for sure aren't using it
(e.g. no longer need the GIL). */
Expand Down Expand Up @@ -1848,13 +1845,8 @@ _PyThreadState_RemoveExcept(PyThreadState *tstate)
{
assert(tstate != NULL);
PyInterpreterState *interp = tstate->interp;
_PyRuntimeState *runtime = interp->runtime;

#ifdef Py_GIL_DISABLED
assert(runtime->stoptheworld.world_stopped);
#endif

HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
/* Remove all thread states, except tstate, from the linked list of
thread states. */
PyThreadState *list = interp->threads.head;
Expand All @@ -1869,7 +1861,7 @@ _PyThreadState_RemoveExcept(PyThreadState *tstate)
}
tstate->prev = tstate->next = NULL;
interp->threads.head = tstate;
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);

return list;
}
Expand Down Expand Up @@ -2336,7 +2328,6 @@ _PyEval_StartTheWorld(PyInterpreterState *interp)
int
PyThreadState_SetAsyncExc(unsigned long id, PyObject *exc)
{
_PyRuntimeState *runtime = &_PyRuntime;
PyInterpreterState *interp = _PyInterpreterState_GET();

/* Although the GIL is held, a few C API functions can be called
Expand All @@ -2345,7 +2336,7 @@ PyThreadState_SetAsyncExc(unsigned long id, PyObject *exc)
* list of thread states we're traversing, so to prevent that we lock
* head_mutex for the duration.
*/
HEAD_LOCK(runtime);
INTERP_HEAD_LOCK(interp);
for (PyThreadState *tstate = interp->threads.head; tstate != NULL; tstate = tstate->next) {
if (tstate->thread_id != id) {
continue;
Expand All @@ -2360,13 +2351,13 @@ PyThreadState_SetAsyncExc(unsigned long id, PyObject *exc)
*/
Py_XINCREF(exc);
PyObject *old_exc = _Py_atomic_exchange_ptr(&tstate->async_exc, exc);
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);

Py_XDECREF(old_exc);
_Py_set_eval_breaker_bit(tstate, _PY_ASYNC_EXCEPTION_BIT);
return 1;
}
HEAD_UNLOCK(runtime);
INTERP_HEAD_UNLOCK(interp);
return 0;
}

Expand Down
Loading