Skip to content

gh-116818: Make sys.settrace, sys.setprofile, and monitoring thread-safe #116775

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 19, 2024
Merged

gh-116818: Make sys.settrace, sys.setprofile, and monitoring thread-safe #116775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2f6ca58
tracing thread safety
DinoV Mar 12, 2024
fe19d59
Lock on allocation, use version as sync point
DinoV Mar 25, 2024
210b802
Lock in non-debug builds
DinoV Mar 28, 2024
597f40d
Move CHECK_EVAL_BREAKER in ENTER_EXECUTOR
DinoV Mar 29, 2024
8393bfe
Don't potentially leak executor
DinoV Apr 2, 2024
65ff505
Just use _Py_atomic_exchange_ptr instead of FT macro
DinoV Apr 4, 2024
6e19601
Use FT_ATOMIC_LOAD_UINTPTR_ACQUIRE for INSTRUMENTED_RESUME too
DinoV Apr 18, 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
8 changes: 8 additions & 0 deletions Include/cpython/pyatomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,10 +465,16 @@ _Py_atomic_store_ullong_relaxed(unsigned long long *obj,
static inline void *
_Py_atomic_load_ptr_acquire(const void *obj);

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj);

// Stores `*obj = value` (release operation)
static inline void
_Py_atomic_store_ptr_release(void *obj, void *value);

static inline void
_Py_atomic_store_uintptr_release(uintptr_t *obj, uintptr_t value);

static inline void
_Py_atomic_store_ssize_release(Py_ssize_t *obj, Py_ssize_t value);

Expand All @@ -491,6 +497,8 @@ static inline Py_ssize_t
_Py_atomic_load_ssize_acquire(const Py_ssize_t *obj);




// --- _Py_atomic_fence ------------------------------------------------------

// Sequential consistency fence. C11 fences have complex semantics. When
Expand Down
8 changes: 8 additions & 0 deletions Include/cpython/pyatomic_gcc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,10 +492,18 @@ static inline void *
_Py_atomic_load_ptr_acquire(const void *obj)
{ return (void *)__atomic_load_n((void **)obj, __ATOMIC_ACQUIRE); }

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj)
{ return (uintptr_t)__atomic_load_n((uintptr_t *)obj, __ATOMIC_ACQUIRE); }

static inline void
_Py_atomic_store_ptr_release(void *obj, void *value)
{ __atomic_store_n((void **)obj, value, __ATOMIC_RELEASE); }

static inline void
_Py_atomic_store_uintptr_release(uintptr_t *obj, uintptr_t value)
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

static inline void
_Py_atomic_store_int_release(int *obj, int value)
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
Expand Down
25 changes: 25 additions & 0 deletions Include/cpython/pyatomic_msc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,18 @@ _Py_atomic_load_ptr_acquire(const void *obj)
#endif
}

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj)
{
#if defined(_M_X64) || defined(_M_IX86)
return *(uintptr_t volatile *)obj;
#elif defined(_M_ARM64)
return (uintptr_t)__ldar64((unsigned __int64 volatile *)obj);
#else
# error "no implementation of _Py_atomic_load_uintptr_acquire"
#endif
}

static inline void
_Py_atomic_store_ptr_release(void *obj, void *value)
{
Expand All @@ -926,6 +938,19 @@ _Py_atomic_store_ptr_release(void *obj, void *value)
#endif
}

static inline void
_Py_atomic_store_uintptr_release(uintptr_t *obj, uintptr_t value)
{
#if defined(_M_X64) || defined(_M_IX86)
*(uintptr_t volatile *)obj = value;
#elif defined(_M_ARM64)
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int64);
__stlr64((unsigned __int64 volatile *)obj, (unsigned __int64)value);
#else
# error "no implementation of _Py_atomic_store_uintptr_release"
#endif
}

static inline void
_Py_atomic_store_int_release(int *obj, int value)
{
Expand Down
16 changes: 16 additions & 0 deletions Include/cpython/pyatomic_std.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,14 @@ _Py_atomic_load_ptr_acquire(const void *obj)
memory_order_acquire);
}

static inline uintptr_t
_Py_atomic_load_uintptr_acquire(const uintptr_t *obj)
{
_Py_USING_STD;
return atomic_load_explicit((const _Atomic(uintptr_t)*)obj,
memory_order_acquire);
}

static inline void
_Py_atomic_store_ptr_release(void *obj, void *value)
{
Expand All @@ -871,6 +879,14 @@ _Py_atomic_store_ptr_release(void *obj, void *value)
memory_order_release);
}

static inline void
_Py_atomic_store_uintptr_release(uintptr_t *obj, uintptr_t value)
{
_Py_USING_STD;
atomic_store_explicit((_Atomic(uintptr_t)*)obj, value,
memory_order_release);
}

static inline void
_Py_atomic_store_int_release(int *obj, int value)
{
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_ceval_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ struct _ceval_runtime_state {
} perf;
/* Pending calls to be made only on the main thread. */
struct _pending_calls pending_mainthread;
PyMutex sys_trace_profile_mutex;
};

#ifdef PY_HAVE_PERF_TRAMPOLINE
Expand Down
2 changes: 1 addition & 1 deletion Include/internal/pycore_gc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ static inline void _PyObject_GC_SET_SHARED(PyObject *op) {
* threads and needs special purpose when freeing due to
* the possibility of in-flight lock-free reads occurring.
* Objects with this bit that are GC objects will automatically
* delay-freed by PyObject_GC_Del. */
* delay-freed by PyObject_GC_Del. */
static inline int _PyObject_GC_IS_SHARED_INLINE(PyObject *op) {
return (op->ob_gc_bits & _PyGC_BITS_SHARED_INLINE) != 0;
}
Expand Down
14 changes: 14 additions & 0 deletions Include/internal/pycore_pyatomic_ft_wrappers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,34 @@ extern "C" {
_Py_atomic_load_ssize_relaxed(&value)
#define FT_ATOMIC_STORE_PTR(value, new_value) \
_Py_atomic_store_ptr(&value, new_value)
#define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) \
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping things in macros often obfuscates the intent.
If I see _Py_atomic_load_ptr_acquire(&value) I know what it does (assuming I know how atomics work)
If I see FT_ATOMIC_LOAD_PTR_ACQUIRE(value) I need to scan the source code to find the macro definition.

Can we come up with a name that describes the semantics, not the context in which it should be used?
All these operations are thread safe on all platforms. Either the GIL makes them safe or we use hardware atomics.
So, maybe choose a name based on the semantics not the context, and drop the FT_ prefix?

OOI, do we have any evidence that relaxed/release/acquire gains us performance over sequential consistency which is a lot easier to reason about and thus safer.

_Py_atomic_load_ptr_acquire(&value)
#define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) \
_Py_atomic_load_uintptr_acquire(&value)
#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \
_Py_atomic_store_ptr_relaxed(&value, new_value)
#define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) \
_Py_atomic_store_ptr_release(&value, new_value)
#define FT_ATOMIC_STORE_UINTPTR_RELEASE(value, new_value) \
_Py_atomic_store_uintptr_release(&value, new_value)
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \
_Py_atomic_store_ssize_relaxed(&value, new_value)
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \
_Py_atomic_store_uint8_relaxed(&value, new_value)

#else
#define FT_ATOMIC_LOAD_PTR(value) value
#define FT_ATOMIC_LOAD_SSIZE(value) value
#define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value
#define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value
#define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) value
#define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value
#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value
#define FT_ATOMIC_STORE_UINTPTR_RELEASE(value, new_value) value = new_value
#define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value
#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value

#endif

#ifdef __cplusplus
Expand Down
7 changes: 7 additions & 0 deletions Lib/test/test_free_threading/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import os

from test import support


def load_tests(*args):
return support.load_package_tests(os.path.dirname(__file__), *args)
Loading
Loading