gh-152330: Make perf_trampoline thread safe.#152331
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
YukaSadaoka
changed the title
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
StanFromIreland
changed the title
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| { | ||
| free_code_arenas(); | ||
| perf_trampoline_clear_code_watcher(); | ||
| // We do NOT clear code_watcher_id here. PyCode_ClearWatcher is not |
There was a problem hiding this comment.
This watcher remains registered, but _PyPerfTrampoline_Init() adds another one. A destroy event then decrements trampoline_refcount twice and can free active arenas. We need to reuse or safely clear it.
| // Call write_state outside the global lock to avoid holding it | ||
| // during JIT I/O. The default implementation | ||
| // (PyUnstable_WritePerfMapEntry) is internally thread-safe. | ||
| trampoline_api.write_state(trampoline_api.state, new_trampoline, |
There was a problem hiding this comment.
This can race with _PyPerfTrampoline_Fini in free-threaded builds: another thread can run free_state between the unlock and this call, and perf_map_jit_write_entry does not re-check perf_map != NULL under its own lock, so we can end up writing to a closed jitdump file. Do we need to re-check the state in the jit callback, or keep write_state under the lock for that backend?
| #endif | ||
| return -1; | ||
| } | ||
| code_watcher_id = PyCode_AddWatcher(perf_trampoline_code_watcher); |
There was a problem hiding this comment.
Now that perf_trampoline_reset_state never calls PyCode_ClearWatcher, every re-activation registers a second copy of perf_trampoline_code_watcher (nothing outside the fork handler ever clears one). Each code destruction then decrements trampoline_refcount once per registered copy, so we can free the arenas while trampolines are still live, and after 8 activations PyCode_AddWatcher fails and the trampoline cannot be activated again in the process. I know part of this pre-dates the PR, but we should reuse code_watcher_id when it is still >= 0 instead of registering a new one.
| return 0; | ||
| } | ||
| PyThreadState *tstate = _PyThreadState_GET(); | ||
| if (code_watcher_id == 0) { |
There was a problem hiding this comment.
Now that code_watcher_id is initialized to -1 in _PyEval_RUNTIME_PERF_INIT, this check should be removed: with the new reset behavior it can throw away a valid watcher id 0 that is still registered.
| if (ret != 0 || f == NULL) { | ||
| new_trampoline = compile_trampoline(); | ||
| if (new_trampoline != NULL) { | ||
| _PyCode_SetExtra((PyObject *)co, index, |
There was a problem hiding this comment.
_PyCode_SetExtra() can fail here. We then increment the refcount and execute with an exception set. Please only publish and count the trampoline on success.
| // Initialize to -1 since 0 is a valid watcher ID | ||
| code_watcher_id = -1; | ||
| } | ||
| if (!activate) { |
There was a problem hiding this comment.
Nit: PEP 7 , this block (and the fork-restart block and the default_eval fallback) is still using 2-space indentation.
| PyMutex_Lock(&perf_trampoline_mutex); | ||
| #endif | ||
| if (ATOMIC_LOAD_STATUS() == PERF_STATUS_OK) { | ||
| ret = _PyCode_GetExtra((PyObject *)co, index, (void **)&f); |
There was a problem hiding this comment.
index was loaded before taking the lock, so it can be stale here if another thread did a Fini/Init cycle in between: we would set the extra at the old index and bump the new trampoline_refcount for a trampoline the watcher will never see, so the arenas never get freed. Can we reload extra_code_index after acquiring the mutex? Same applies to PyUnstable_PerfTrampoline_CompileCode.
| @@ -0,0 +1 @@ | |||
| Make perf_trampoline thread safe by fixing critical data races, post-fork deadlocks, memory leaks, and edge-case exception handling issues. | |||
There was a problem hiding this comment.
Nit: this is a change in Python/perf_trampoline.c, so the NEWS entry should go in Core and Builtins, not Library.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
2 participants
Summary of Changes
Fixes multiple critical data races, post-fork deadlocks, memory leaks, and edge-case exception handling issues in
perf_trampoline.cunder Free Threading and Python 3.14.perf_trampoline_mutex): Introduced a dedicated mutex to guard global state mutations and JIT arena allocations (compile_trampoline(), initialization, and teardown). Hot-path executions remain lock-free.perf_status,extra_code_index, andpersist_after_forkto atomic operations.py_trampoline_evaluatorto loadextra_code_indexatomically. If deactivation has started (returning-1), evaluating threads cleanly bypass the JIT trampoline and fall back to_PyEval_EvalFrameDefaultinstead of hitting assertion failures.write_statewriting to/tmp/perf-PID.map) from the critical section, ensuring slow disk I/O does not hold the compiler lock.perf_trampoline_mutexto an unlocked state, preventing deadlocks if a thread forks while another thread holds the lock.PyErr_Clear()) and the trampoline is disabled gracefully rather than leaking unhandled exceptions or crashing.pycore_ceval_state.hby initializing the global watcher ID sentinel to-1instead of0. This allows0to be correctly recognized as a valid active watcher ID.Fixes gh-152330