Skip to content

gh-148613: Fix race in gc_set_threshold and gc_get_threshold#150356

Open
LindaSummer wants to merge 4 commits into
python:mainfrom
LindaSummer:gc_state_tsan
Open

gh-148613: Fix race in gc_set_threshold and gc_get_threshold#150356
LindaSummer wants to merge 4 commits into
python:mainfrom
LindaSummer:gc_state_tsan

Conversation

@LindaSummer
Copy link
Copy Markdown
Contributor

Issue

gh-148613

Root Cause

In free-threading, the gc_generation.threshold races in threads when one thread has objects triggered the GC.

if (gc->alloc_count >= LOCAL_ALLOC_COUNT_THRESHOLD) {
// TODO: Use Py_ssize_t for the generation count.
GCState *gcstate = &tstate->interp->gc;
_Py_atomic_add_int(&gcstate->young.count, (int)gc->alloc_count);
gc->alloc_count = 0;
if (gc_should_collect(gcstate) &&

Inside gc_should_collect we read the gcstate->young.threshold and gcstate->old[0].threshold without thread syncing.

gc_should_collect(GCState *gcstate)
{
int count = _Py_atomic_load_int_relaxed(&gcstate->young.count);
int threshold = gcstate->young.threshold;
int gc_enabled = _Py_atomic_load_int_relaxed(&gcstate->enabled);
if (count <= threshold || threshold == 0 || !gc_enabled) {
return false;
}
if (gcstate->old[0].threshold == 0) {

At the same time, the threshold setting also has no syncing protection.

cpython/Modules/gcmodule.c

Lines 170 to 175 in cb72193

gcstate->young.threshold = threshold0;
if (group_right_1) {
gcstate->old[0].threshold = threshold1;
}
if (group_right_2) {
gcstate->old[1].threshold = threshold2;

This explains why a cyclic referenced object caused this TSAN report.
The cyclic object couldn't make ref count to zero in scoped call stack, and it increments the _gc_thread_state.alloc_count to LOCAL_ALLOC_COUNT_THRESHOLD.
Then the GC collect triggered in this thread and races with another thread's update of gc_generation.threshold.

Proposed Changes

Add relaxed atomic load/store protection for the gc_generation.threshold setter and getter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant