From df767e58c97f074c6391a228ff3098cabbbdde29 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Thu, 6 Jun 2024 23:49:38 +0800 Subject: [PATCH 1/8] Make Py_TYPE and Py_SET_TYPE thread safe --- Include/object.h | 8 ++++++++ Tools/tsan/suppressions_free_threading.txt | 2 -- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Include/object.h b/Include/object.h index c8c63b9b2b1450..bbc17f574b341b 100644 --- a/Include/object.h +++ b/Include/object.h @@ -246,7 +246,11 @@ _Py_IsOwnedByCurrentThread(PyObject *ob) // bpo-39573: The Py_SET_TYPE() function must be used to set an object type. static inline PyTypeObject* Py_TYPE(PyObject *ob) { +#ifdef Py_GIL_DISABLED + return _Py_atomic_load_ptr_relaxed(&ob->ob_type); +#else return ob->ob_type; +#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_TYPE(ob) Py_TYPE(_PyObject_CAST(ob)) @@ -274,7 +278,11 @@ static inline int Py_IS_TYPE(PyObject *ob, PyTypeObject *type) { static inline void Py_SET_TYPE(PyObject *ob, PyTypeObject *type) { +#ifdef Py_GIL_DISABLED + _Py_atomic_store_ptr(&ob->ob_type, type); +#else ob->ob_type = type; +#endif } #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000 # define Py_SET_TYPE(ob, type) Py_SET_TYPE(_PyObject_CAST(ob), type) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 8b64d1ff321858..4324f9db8922dc 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -39,7 +39,6 @@ race_top:set_contains_key # https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35 race_top:set_discard_entry race_top:set_inheritable -race_top:Py_SET_TYPE race_top:_PyDict_CheckConsistency race_top:_PyImport_AcquireLock race_top:_Py_dict_lookup_threadsafe @@ -62,7 +61,6 @@ race_top:_PyFrame_Initialize race_top:PyInterpreterState_ThreadHead race_top:_PyObject_TryGetInstanceAttribute race_top:PyThreadState_Next -race_top:Py_TYPE race_top:PyUnstable_InterpreterFrame_GetLine race_top:sock_close race_top:tstate_delete_common From 2e9fda6d71bd90a16bdee8ed57b109d2eb3cfc6d Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 00:05:33 +0800 Subject: [PATCH 2/8] fix Windows --- Include/object.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/object.h b/Include/object.h index bbc17f574b341b..4a39ada8c7daa4 100644 --- a/Include/object.h +++ b/Include/object.h @@ -247,7 +247,7 @@ _Py_IsOwnedByCurrentThread(PyObject *ob) // bpo-39573: The Py_SET_TYPE() function must be used to set an object type. static inline PyTypeObject* Py_TYPE(PyObject *ob) { #ifdef Py_GIL_DISABLED - return _Py_atomic_load_ptr_relaxed(&ob->ob_type); + return (PyTypeObject *)_Py_atomic_load_ptr_relaxed(&ob->ob_type); #else return ob->ob_type; #endif From bda9545ac413922c49b91fad870e12990258ffac Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:28:11 +0800 Subject: [PATCH 3/8] add test --- Include/internal/pycore_interp.h | 7 ++++-- Lib/test/test_free_threading/test_type.py | 27 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 86dada5061e7b5..2fecc08c2facee 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -400,8 +400,11 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( #define RARE_EVENT_INTERP_INC(interp, name) \ do { \ - /* saturating add */ \ - if (interp->rare_events.name < UINT8_MAX) interp->rare_events.name++; \ + /* saturating add */ \ + int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ + if (val < UINT8_MAX) { \ + FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ + } \ RARE_EVENT_STAT_INC(name); \ } while (0); \ diff --git a/Lib/test/test_free_threading/test_type.py b/Lib/test/test_free_threading/test_type.py index 6eead198deed46..1e84b2db2d4882 100644 --- a/Lib/test/test_free_threading/test_type.py +++ b/Lib/test/test_free_threading/test_type.py @@ -1,3 +1,4 @@ +import threading import unittest from concurrent.futures import ThreadPoolExecutor @@ -95,6 +96,32 @@ def reader_func(): self.run_one(writer_func, reader_func) + def test___class___modification(self): + class Foo: + pass + + class Bar: + pass + + thing = Foo() + def work(): + foo = thing + for _ in range(10000): + foo.__class__ = Bar + type(foo) + foo.__class__ = Foo + type(foo) + + + threads = [] + for i in range(NTHREADS): + thread = threading.Thread(target=work) + thread.start() + threads.append(thread) + + for thread in threads: + thread.join() + def run_one(self, writer_func, reader_func): writer = Thread(target=writer_func) readers = [] From b49dc1a2ce41925a6b869e30453e36a868fd733d Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:34:14 +0800 Subject: [PATCH 4/8] style nits --- Include/internal/pycore_interp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2fecc08c2facee..2a3ae6be914a45 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -400,7 +400,7 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( #define RARE_EVENT_INTERP_INC(interp, name) \ do { \ - /* saturating add */ \ + /* saturating add */ \ int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ if (val < UINT8_MAX) { \ FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ From 07459a167cc61a727091b04fdd4f13ac51b0d628 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Sun, 9 Jun 2024 19:28:15 +0800 Subject: [PATCH 5/8] use stronger guarantees --- Include/internal/pycore_interp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2a3ae6be914a45..29c83789a90418 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -403,7 +403,7 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( /* saturating add */ \ int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ if (val < UINT8_MAX) { \ - FT_ATOMIC_STORE_UINT8_RELAXED(interp->rare_events.name, val+1); \ + FT_ATOMIC_STORE_UINT8(interp->rare_events.name, val+1); \ } \ RARE_EVENT_STAT_INC(name); \ } while (0); \ From 156908d731bba10c2364806108db4040777dd9da Mon Sep 17 00:00:00 2001 From: Ken Jin Date: Sun, 9 Jun 2024 20:53:14 +0800 Subject: [PATCH 6/8] Update Include/internal/pycore_interp.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> --- Include/internal/pycore_interp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 29c83789a90418..6b5f50b88f7b85 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -403,7 +403,7 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New( /* saturating add */ \ int val = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->rare_events.name); \ if (val < UINT8_MAX) { \ - FT_ATOMIC_STORE_UINT8(interp->rare_events.name, val+1); \ + FT_ATOMIC_STORE_UINT8(interp->rare_events.name, val + 1); \ } \ RARE_EVENT_STAT_INC(name); \ } while (0); \ From 9a33a780d5a87865b3e3f9061e4378c5d1b2da46 Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:56:01 +0800 Subject: [PATCH 7/8] Lock critical section --- Objects/typeobject.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 070e3d2f7bf2b4..86b9a33ad01e5b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6630,12 +6630,18 @@ object_set_class(PyObject *self, PyObject *value, void *closure) return -1; } } + Py_BEGIN_CRITICAL_SECTION(self); if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) { Py_INCREF(newto); } + // The real Py_TYPE(self) (`oldto`) may have changed from + // underneath us in another thread, so we re-fetch it here. + oldto = Py_TYPE(self); Py_SET_TYPE(self, newto); - if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) + if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) { Py_DECREF(oldto); + } + Py_END_CRITICAL_SECTION(); RARE_EVENT_INC(set_class); return 0; From e43acf0474be283d1623ca71a9588248a3da286f Mon Sep 17 00:00:00 2001 From: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Date: Wed, 12 Jun 2024 19:31:07 +0800 Subject: [PATCH 8/8] shrink critical section Co-Authored-By: Nadeshiko Manju --- Objects/typeobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 86b9a33ad01e5b..8ecab555454cdc 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6630,18 +6630,18 @@ object_set_class(PyObject *self, PyObject *value, void *closure) return -1; } } - Py_BEGIN_CRITICAL_SECTION(self); if (newto->tp_flags & Py_TPFLAGS_HEAPTYPE) { Py_INCREF(newto); } + Py_BEGIN_CRITICAL_SECTION(self); // The real Py_TYPE(self) (`oldto`) may have changed from // underneath us in another thread, so we re-fetch it here. oldto = Py_TYPE(self); Py_SET_TYPE(self, newto); + Py_END_CRITICAL_SECTION(); if (oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) { Py_DECREF(oldto); } - Py_END_CRITICAL_SECTION(); RARE_EVENT_INC(set_class); return 0;