From 1f335cabb751cec9ee685778a5e5b9f2e70d54ef Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 12:55:20 +0900 Subject: [PATCH 1/7] gh-129533: Update PyGC_Enable/Disable/IsEnabled to use CAS operation --- .../2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst | 3 +++ Python/gc_free_threading.c | 14 +++++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst diff --git a/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst b/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst new file mode 100644 index 00000000000000..dcae222c849003 --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst @@ -0,0 +1,3 @@ +Update :c:func:`PyGC_Enable()`, :c:func:`PyGC_Disable()`, +:c:func:`PyGC_IsEnabled()` to use CAS operation for thread-safety at +free-threading build. Patch by Donghee Na. diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 5d264e407e1cc8..110ab90dab470d 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1952,8 +1952,10 @@ int PyGC_Enable(void) { GCState *gcstate = get_gc_state(); - int old_state = gcstate->enabled; - gcstate->enabled = 1; + int old_state; + do { + old_state = gcstate->enabled; + } while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 1)); return old_state; } @@ -1961,8 +1963,10 @@ int PyGC_Disable(void) { GCState *gcstate = get_gc_state(); - int old_state = gcstate->enabled; - gcstate->enabled = 0; + int old_state; + do { + old_state = gcstate->enabled; + } while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0)); return old_state; } @@ -1970,7 +1974,7 @@ int PyGC_IsEnabled(void) { GCState *gcstate = get_gc_state(); - return gcstate->enabled; + return _Py_atomic_load_int(&(gcstate->enabled)); } /* Public API to invoke gc.collect() from C */ From 06248094c913ad4093909968ba8d6da6c1ffac22 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 13:09:14 +0900 Subject: [PATCH 2/7] fix --- Python/gc_free_threading.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 110ab90dab470d..4b66ef8b816aff 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1952,10 +1952,8 @@ int PyGC_Enable(void) { GCState *gcstate = get_gc_state(); - int old_state; - do { - old_state = gcstate->enabled; - } while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 1)); + int old_state = _Py_atomic_load_int(&(gcstate->enabled)); + while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 1)); return old_state; } @@ -1963,10 +1961,8 @@ int PyGC_Disable(void) { GCState *gcstate = get_gc_state(); - int old_state; - do { - old_state = gcstate->enabled; - } while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0)); + int old_state = _Py_atomic_load_int(&(gcstate->enabled)); + while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0)); return old_state; } From 59ff5239d3a20616d28c9fd65e221f7b881d6070 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 13:30:02 +0900 Subject: [PATCH 3/7] nit --- Python/gc_free_threading.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 4b66ef8b816aff..52fd5d39e3de72 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1476,7 +1476,8 @@ gc_should_collect(GCState *gcstate) { int count = _Py_atomic_load_int_relaxed(&gcstate->young.count); int threshold = gcstate->young.threshold; - if (count <= threshold || threshold == 0 || !gcstate->enabled) { + int gc_enabled = _Py_atomic_load_int_relaxed(&gcstate->enabled); + if (count <= threshold || threshold == 0 || !gc_enabled) { return false; } // Avoid quadratic behavior by scaling threshold to the number of live @@ -1952,7 +1953,7 @@ int PyGC_Enable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_load_int(&(gcstate->enabled)); + int old_state = _Py_atomic_load_int_relaxed(&gcstate->enabled); while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 1)); return old_state; } @@ -1961,7 +1962,7 @@ int PyGC_Disable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_load_int(&(gcstate->enabled)); + int old_state = _Py_atomic_load_int_relaxed(&gcstate->enabled); while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0)); return old_state; } @@ -1970,7 +1971,7 @@ int PyGC_IsEnabled(void) { GCState *gcstate = get_gc_state(); - return _Py_atomic_load_int(&(gcstate->enabled)); + return _Py_atomic_load_int_relaxed(&gcstate->enabled); } /* Public API to invoke gc.collect() from C */ From e0669587f4f59d998895804e2b475b1291ddd0b8 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 13:34:37 +0900 Subject: [PATCH 4/7] nit --- Python/gc_free_threading.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 52fd5d39e3de72..6a452fb7677827 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1981,7 +1981,7 @@ PyGC_Collect(void) PyThreadState *tstate = _PyThreadState_GET(); GCState *gcstate = &tstate->interp->gc; - if (!gcstate->enabled) { + if (!_Py_atomic_load_int_relaxed(&gcstate->enabled)) { return 0; } @@ -2140,8 +2140,7 @@ _PyObject_GC_Link(PyObject *op) void _Py_RunGC(PyThreadState *tstate) { - GCState *gcstate = get_gc_state(); - if (!gcstate->enabled) { + if (!PyGC_IsEnabled()) { return; } gc_collect_main(tstate, 0, _Py_GC_REASON_HEAP); From 8a41f07364edb58fc2a95731778b9d08efdd3630 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 16:12:54 +0900 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: mpage --- Python/gc_free_threading.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 6a452fb7677827..a273ad8668e275 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1953,8 +1953,7 @@ int PyGC_Enable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_load_int_relaxed(&gcstate->enabled); - while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 1)); + int old_state = _Py_atomic_exchange_int(&gcstate->enabled, 1); return old_state; } @@ -1962,8 +1961,7 @@ int PyGC_Disable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_load_int_relaxed(&gcstate->enabled); - while (!_Py_atomic_compare_exchange_int(&gcstate->enabled, &old_state, 0)); + int old_state = _Py_atomic_exchange_int(&gcstate->enabled, 0); return old_state; } From 6c990f98f492e14e3c1656bc9b65dafd802484f7 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 16:16:10 +0900 Subject: [PATCH 6/7] Update NEWS.d --- .../next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst b/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst index dcae222c849003..20e93e2bcc1f47 100644 --- a/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst +++ b/Misc/NEWS.d/next/C_API/2025-02-02-12-58-21.gh-issue-129533.dFfqkT.rst @@ -1,3 +1,3 @@ Update :c:func:`PyGC_Enable()`, :c:func:`PyGC_Disable()`, -:c:func:`PyGC_IsEnabled()` to use CAS operation for thread-safety at -free-threading build. Patch by Donghee Na. +:c:func:`PyGC_IsEnabled()` to use atomic operation for thread-safety +at free-threading build. Patch by Donghee Na. From d36c349a6f7dac9d6a5105559591a2475a79b39e Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sun, 2 Feb 2025 18:55:26 +0900 Subject: [PATCH 7/7] Address code review --- Python/gc_free_threading.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index a273ad8668e275..14aeac785ead44 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1953,16 +1953,14 @@ int PyGC_Enable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_exchange_int(&gcstate->enabled, 1); - return old_state; + return _Py_atomic_exchange_int(&gcstate->enabled, 1); } int PyGC_Disable(void) { GCState *gcstate = get_gc_state(); - int old_state = _Py_atomic_exchange_int(&gcstate->enabled, 0); - return old_state; + return _Py_atomic_exchange_int(&gcstate->enabled, 0); } int