From 861f3a640fac626492cec78f1b77f7ceae0414f7 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 21 May 2024 19:34:22 +0000 Subject: [PATCH 01/11] gh-119344: Make critical section API public --- Include/Python.h | 1 + Include/cpython/critical_section.h | 75 ++++++++++++++++++ Include/critical_section.h | 16 ++++ Include/internal/pycore_critical_section.h | 78 ++++++++----------- Makefile.pre.in | 2 + ...-05-21-19-41-41.gh-issue-119344.QKvzQb.rst | 1 + Objects/typeobject.c | 10 +-- PCbuild/pythoncore.vcxproj | 2 + PCbuild/pythoncore.vcxproj.filters | 6 ++ Python/critical_section.c | 44 +++++++++++ 10 files changed, 184 insertions(+), 51 deletions(-) create mode 100644 Include/cpython/critical_section.h create mode 100644 Include/critical_section.h create mode 100644 Misc/NEWS.d/next/C API/2024-05-21-19-41-41.gh-issue-119344.QKvzQb.rst diff --git a/Include/Python.h b/Include/Python.h index e05901b9e52b5a..1520453873af3f 100644 --- a/Include/Python.h +++ b/Include/Python.h @@ -119,6 +119,7 @@ #include "import.h" #include "abstract.h" #include "bltinmodule.h" +#include "critical_section.h" #include "cpython/pyctype.h" #include "pystrtod.h" #include "pystrcmp.h" diff --git a/Include/cpython/critical_section.h b/Include/cpython/critical_section.h new file mode 100644 index 00000000000000..38b2917bad8f5d --- /dev/null +++ b/Include/cpython/critical_section.h @@ -0,0 +1,75 @@ +#ifndef Py_CPYTHON_CRITICAL_SECTION_H +# error "this header file must not be included directly" +#endif + +// Python critical sections. +// +// These operations are no-ops in the default build. See +// pycore_critical_section.h for details. + +typedef struct _PyCriticalSection _PyCriticalSection; +typedef struct _PyCriticalSection2 _PyCriticalSection2; + +#ifndef Py_GIL_DISABLED +# define Py_BEGIN_CRITICAL_SECTION(op) \ + { +# define Py_END_CRITICAL_SECTION() \ + } +# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ + { +# define Py_END_CRITICAL_SECTION2() \ + } +#else /* !Py_GIL_DISABLED */ + +// (private) +struct _PyCriticalSection { + // Tagged pointer to an outer active critical section (or 0). + uintptr_t prev; + + // Mutex used to protect critical section + struct _PyMutex *mutex; +}; + +// (private) A critical section protected by two mutexes. Use +// Py_BEGIN_CRITICAL_SECTION2 and Py_END_CRITICAL_SECTION2. +struct _PyCriticalSection2 { + struct _PyCriticalSection base; + + struct _PyMutex *mutex2; +}; + +# define Py_BEGIN_CRITICAL_SECTION(op) \ + { \ + _PyCriticalSection _cs; \ + _PyCriticalSection_Begin(&_cs, _PyObject_CAST(op)) + +# define Py_END_CRITICAL_SECTION() \ + _PyCriticalSection_End(&_cs); \ + } + +# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ + { \ + _PyCriticalSection2 _cs2; \ + _PyCriticalSection2_Begin(&_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) + +# define Py_END_CRITICAL_SECTION2() \ + _PyCriticalSection2_End(&_cs2); \ + } + +#endif + +// (private) +PyAPI_FUNC(void) +_PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op); + +// (private) +PyAPI_FUNC(void) +_PyCriticalSection_End(_PyCriticalSection *c); + +// CPython internals should use pycore_critical_section.h instead. +#ifdef Py_BUILD_CORE +# undef Py_BEGIN_CRITICAL_SECTION +# undef Py_END_CRITICAL_SECTION +# undef Py_BEGIN_CRITICAL_SECTION2 +# undef Py_END_CRITICAL_SECTION2 +#endif diff --git a/Include/critical_section.h b/Include/critical_section.h new file mode 100644 index 00000000000000..3b37615a8b17e2 --- /dev/null +++ b/Include/critical_section.h @@ -0,0 +1,16 @@ +#ifndef Py_CRITICAL_SECTION_H +#define Py_CRITICAL_SECTION_H +#ifdef __cplusplus +extern "C" { +#endif + +#ifndef Py_LIMITED_API +# define Py_CPYTHON_CRITICAL_SECTION_H +# include "cpython/critical_section.h" +# undef Py_CPYTHON_CRITICAL_SECTION_H +#endif + +#ifdef __cplusplus +} +#endif +#endif /* !Py_CRITICAL_SECTION_H */ diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 573d09a09683ef..afb92361cea352 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -90,28 +90,30 @@ extern "C" { # define Py_BEGIN_CRITICAL_SECTION_MUT(mutex) \ { \ _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, mutex) + _PyCriticalSection_BeginInline(&_cs, mutex) # define Py_BEGIN_CRITICAL_SECTION(op) \ Py_BEGIN_CRITICAL_SECTION_MUT(&_PyObject_CAST(op)->ob_mutex) # define Py_END_CRITICAL_SECTION() \ - _PyCriticalSection_End(&_cs); \ + _PyCriticalSection_EndInline(&_cs); \ } # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ _PyCriticalSection2 _cs2; \ - _PyCriticalSection2_Begin(&_cs2, &_PyObject_CAST(a)->ob_mutex, &_PyObject_CAST(b)->ob_mutex) + _PyCriticalSection2_BeginInline(&_cs2, \ + &_PyObject_CAST(a)->ob_mutex, \ + &_PyObject_CAST(b)->ob_mutex) # define Py_END_CRITICAL_SECTION2() \ - _PyCriticalSection2_End(&_cs2); \ + _PyCriticalSection2_EndInline(&_cs2); \ } // Asserts that the mutex is locked. The mutex must be held by the // top-most critical section otherwise there's the possibility // that the mutex would be swalled out in some code paths. -#define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) \ +#define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) \ _PyCriticalSection_AssertHeld(mutex) // Asserts that the mutex for the given object is locked. The mutex must @@ -119,51 +121,29 @@ extern "C" { // possibility that the mutex would be swalled out in some code paths. #ifdef Py_DEBUG -#define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) \ +# define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) \ if (Py_REFCNT(op) != 1) { \ _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(&_PyObject_CAST(op)->ob_mutex); \ } #else /* Py_DEBUG */ -#define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) +# define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) #endif /* Py_DEBUG */ #else /* !Py_GIL_DISABLED */ // The critical section APIs are no-ops with the GIL. -# define Py_BEGIN_CRITICAL_SECTION_MUT(mut) -# define Py_BEGIN_CRITICAL_SECTION(op) -# define Py_END_CRITICAL_SECTION() -# define Py_BEGIN_CRITICAL_SECTION2(a, b) -# define Py_END_CRITICAL_SECTION2() +# define Py_BEGIN_CRITICAL_SECTION_MUT(mut) { +# define Py_BEGIN_CRITICAL_SECTION(op) { +# define Py_END_CRITICAL_SECTION() } +# define Py_BEGIN_CRITICAL_SECTION2(a, b) { +# define Py_END_CRITICAL_SECTION2() } # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) # define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) #endif /* !Py_GIL_DISABLED */ -typedef struct { - // Tagged pointer to an outer active critical section (or 0). - // The two least-significant-bits indicate whether the pointed-to critical - // section is inactive and whether it is a _PyCriticalSection2 object. - uintptr_t prev; - - // Mutex used to protect critical section - PyMutex *mutex; -} _PyCriticalSection; - -// A critical section protected by two mutexes. Use -// _PyCriticalSection2_Begin and _PyCriticalSection2_End. -typedef struct { - _PyCriticalSection base; - - PyMutex *mutex2; -} _PyCriticalSection2; - -static inline int -_PyCriticalSection_IsActive(uintptr_t tag) -{ - return tag != 0 && (tag & _Py_CRITICAL_SECTION_INACTIVE) == 0; -} +typedef struct _PyCriticalSection2 _PyCriticalSection2; // Resumes the top-most critical section. PyAPI_FUNC(void) @@ -177,8 +157,19 @@ PyAPI_FUNC(void) _PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, int is_m1_locked); +PyAPI_FUNC(void) +_PyCriticalSection_SuspendAll(PyThreadState *tstate); + +#ifdef Py_GIL_DISABLED + +static inline int +_PyCriticalSection_IsActive(uintptr_t tag) +{ + return tag != 0 && (tag & _Py_CRITICAL_SECTION_INACTIVE) == 0; +} + static inline void -_PyCriticalSection_Begin(_PyCriticalSection *c, PyMutex *m) +_PyCriticalSection_BeginInline(_PyCriticalSection *c, PyMutex *m) { if (PyMutex_LockFast(&m->v)) { PyThreadState *tstate = _PyThreadState_GET(); @@ -207,20 +198,20 @@ _PyCriticalSection_Pop(_PyCriticalSection *c) } static inline void -_PyCriticalSection_End(_PyCriticalSection *c) +_PyCriticalSection_EndInline(_PyCriticalSection *c) { PyMutex_Unlock(c->mutex); _PyCriticalSection_Pop(c); } static inline void -_PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) +_PyCriticalSection2_BeginInline(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) { if (m1 == m2) { // If the two mutex arguments are the same, treat this as a critical // section with a single mutex. c->mutex2 = NULL; - _PyCriticalSection_Begin(&c->base, m1); + _PyCriticalSection_BeginInline(&c->base, m1); return; } @@ -253,7 +244,7 @@ _PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) } static inline void -_PyCriticalSection2_End(_PyCriticalSection2 *c) +_PyCriticalSection2_EndInline(_PyCriticalSection2 *c) { if (c->mutex2) { PyMutex_Unlock(c->mutex2); @@ -262,11 +253,6 @@ _PyCriticalSection2_End(_PyCriticalSection2 *c) _PyCriticalSection_Pop(&c->base); } -PyAPI_FUNC(void) -_PyCriticalSection_SuspendAll(PyThreadState *tstate); - -#ifdef Py_GIL_DISABLED - static inline void _PyCriticalSection_AssertHeld(PyMutex *mutex) { @@ -285,7 +271,7 @@ _PyCriticalSection_AssertHeld(PyMutex *mutex) #endif } -#endif +#endif /* Py_GIL_DISABLED */ #ifdef __cplusplus } diff --git a/Makefile.pre.in b/Makefile.pre.in index 9e99c95e2af042..54a1dc15fc4e62 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -1002,6 +1002,7 @@ PYTHON_HEADERS= \ $(srcdir)/Include/codecs.h \ $(srcdir)/Include/compile.h \ $(srcdir)/Include/complexobject.h \ + $(srcdir)/Include/critical_section.h \ $(srcdir)/Include/descrobject.h \ $(srcdir)/Include/dictobject.h \ $(srcdir)/Include/dynamic_annotations.h \ @@ -1078,6 +1079,7 @@ PYTHON_HEADERS= \ $(srcdir)/Include/cpython/compile.h \ $(srcdir)/Include/cpython/complexobject.h \ $(srcdir)/Include/cpython/context.h \ + $(srcdir)/Include/cpython/critical_section.h \ $(srcdir)/Include/cpython/descrobject.h \ $(srcdir)/Include/cpython/dictobject.h \ $(srcdir)/Include/cpython/fileobject.h \ diff --git a/Misc/NEWS.d/next/C API/2024-05-21-19-41-41.gh-issue-119344.QKvzQb.rst b/Misc/NEWS.d/next/C API/2024-05-21-19-41-41.gh-issue-119344.QKvzQb.rst new file mode 100644 index 00000000000000..5a2e4d980b59be --- /dev/null +++ b/Misc/NEWS.d/next/C API/2024-05-21-19-41-41.gh-issue-119344.QKvzQb.rst @@ -0,0 +1 @@ +The critical section API is now public as part of the non-limited C API. diff --git a/Objects/typeobject.c b/Objects/typeobject.c index b7c3fcf47f23fc..c5e378bdb63693 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -65,20 +65,20 @@ class object "PyObject *" "&PyBaseObject_Type" #define BEGIN_TYPE_LOCK() \ { \ _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, TYPE_LOCK); \ + _PyCriticalSection_BeginInline(&_cs, TYPE_LOCK); \ #define END_TYPE_LOCK() \ - _PyCriticalSection_End(&_cs); \ + _PyCriticalSection_EndInline(&_cs); \ } #define BEGIN_TYPE_DICT_LOCK(d) \ { \ _PyCriticalSection2 _cs; \ - _PyCriticalSection2_Begin(&_cs, TYPE_LOCK, \ - &_PyObject_CAST(d)->ob_mutex); \ + _PyCriticalSection2_BeginInline(&_cs, TYPE_LOCK, \ + &_PyObject_CAST(d)->ob_mutex); \ #define END_TYPE_DICT_LOCK() \ - _PyCriticalSection2_End(&_cs); \ + _PyCriticalSection2_EndInline(&_cs); \ } #define ASSERT_TYPE_LOCK_HELD() \ diff --git a/PCbuild/pythoncore.vcxproj b/PCbuild/pythoncore.vcxproj index 16fb424b11c6a8..1053d282d54688 100644 --- a/PCbuild/pythoncore.vcxproj +++ b/PCbuild/pythoncore.vcxproj @@ -135,6 +135,7 @@ + @@ -145,6 +146,7 @@ + diff --git a/PCbuild/pythoncore.vcxproj.filters b/PCbuild/pythoncore.vcxproj.filters index cf9bc0f4bc1c70..2853a65d2a5ee8 100644 --- a/PCbuild/pythoncore.vcxproj.filters +++ b/PCbuild/pythoncore.vcxproj.filters @@ -72,6 +72,9 @@ Include + + Include + Include @@ -372,6 +375,9 @@ Include\cpython + + Include\cpython + Include\cpython diff --git a/Python/critical_section.c b/Python/critical_section.c index 2214d80eeb297b..2336121f1771f3 100644 --- a/Python/critical_section.c +++ b/Python/critical_section.c @@ -3,12 +3,47 @@ #include "pycore_lock.h" #include "pycore_critical_section.h" +#ifdef Py_GIL_DISABLED static_assert(_Alignof(_PyCriticalSection) >= 4, "critical section must be aligned to at least 4 bytes"); +#endif + +void +_PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection_BeginInline(c, &op->ob_mutex); +#endif +} + +void +_PyCriticalSection_End(_PyCriticalSection *c) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection_EndInline(c); +#endif +} + +void +_PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyObject *a, PyObject *b) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection2_BeginInline(c, &a->ob_mutex, &b->ob_mutex); +#endif +} + +void +_PyCriticalSection2_End(_PyCriticalSection2 *c) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection2_EndInline(c); +#endif +} void _PyCriticalSection_BeginSlow(_PyCriticalSection *c, PyMutex *m) { +#ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); c->mutex = NULL; c->prev = (uintptr_t)tstate->critical_section; @@ -16,12 +51,14 @@ _PyCriticalSection_BeginSlow(_PyCriticalSection *c, PyMutex *m) _PyMutex_LockSlow(m); c->mutex = m; +#endif } void _PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, int is_m1_locked) { +#ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); c->base.mutex = NULL; c->mutex2 = NULL; @@ -34,19 +71,23 @@ _PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, PyMutex_Lock(m2); c->base.mutex = m1; c->mutex2 = m2; +#endif } +#ifdef Py_GIL_DISABLED static _PyCriticalSection * untag_critical_section(uintptr_t tag) { return (_PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK); } +#endif // Release all locks held by critical sections. This is called by // _PyThreadState_Detach. void _PyCriticalSection_SuspendAll(PyThreadState *tstate) { +#ifdef Py_GIL_DISABLED uintptr_t *tagptr = &tstate->critical_section; while (_PyCriticalSection_IsActive(*tagptr)) { _PyCriticalSection *c = untag_critical_section(*tagptr); @@ -64,11 +105,13 @@ _PyCriticalSection_SuspendAll(PyThreadState *tstate) *tagptr |= _Py_CRITICAL_SECTION_INACTIVE; tagptr = &c->prev; } +#endif } void _PyCriticalSection_Resume(PyThreadState *tstate) { +#ifdef Py_GIL_DISABLED uintptr_t p = tstate->critical_section; _PyCriticalSection *c = untag_critical_section(p); assert(!_PyCriticalSection_IsActive(p)); @@ -97,4 +140,5 @@ _PyCriticalSection_Resume(PyThreadState *tstate) } tstate->critical_section &= ~_Py_CRITICAL_SECTION_INACTIVE; +#endif } From 0bc4a93a9b27e2effeb02c2054c5fcd499053a20 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 31 May 2024 15:41:28 +0000 Subject: [PATCH 02/11] Add missing functions and public C API test --- Include/cpython/critical_section.h | 8 ++++++++ Modules/_testcapimodule.c | 13 +++++++++++++ 2 files changed, 21 insertions(+) diff --git a/Include/cpython/critical_section.h b/Include/cpython/critical_section.h index 38b2917bad8f5d..89cf39f82bb042 100644 --- a/Include/cpython/critical_section.h +++ b/Include/cpython/critical_section.h @@ -66,6 +66,14 @@ _PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op); PyAPI_FUNC(void) _PyCriticalSection_End(_PyCriticalSection *c); +// (private) +PyAPI_FUNC(void) +_PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyObject *a, PyObject *b); + +// (private) +PyAPI_FUNC(void) +_PyCriticalSection2_End(_PyCriticalSection2 *c); + // CPython internals should use pycore_critical_section.h instead. #ifdef Py_BUILD_CORE # undef Py_BEGIN_CRITICAL_SECTION diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index f99ebf0dde4f9e..dd108ada5ef736 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3312,6 +3312,18 @@ function_set_warning(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) Py_RETURN_NONE; } +static PyObject * +test_critical_sections(PyObject *module, PyObject *Py_UNUSED(args)) +{ + Py_BEGIN_CRITICAL_SECTION(module); + Py_END_CRITICAL_SECTION(); + + Py_BEGIN_CRITICAL_SECTION2(module, module); + Py_END_CRITICAL_SECTION2(); + + Py_RETURN_NONE; +} + static PyMethodDef TestMethods[] = { {"set_errno", set_errno, METH_VARARGS}, {"test_config", test_config, METH_NOARGS}, @@ -3454,6 +3466,7 @@ static PyMethodDef TestMethods[] = { {"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS}, {"test_weakref_capi", test_weakref_capi, METH_NOARGS}, {"function_set_warning", function_set_warning, METH_NOARGS}, + {"test_critical_sections", test_critical_sections, METH_NOARGS}, {NULL, NULL} /* sentinel */ }; From 75c96fe2324fec6b5d764f47fb6779581d1c82ff Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 20 Jun 2024 17:10:22 -0400 Subject: [PATCH 03/11] Remove leading underscore before _PyCriticalSection --- Include/cpython/critical_section.h | 37 ++++++++++++---------- Include/internal/pycore_critical_section.h | 30 ++++++++---------- Objects/typeobject.c | 4 +-- Python/critical_section.c | 28 ++++++++-------- 4 files changed, 50 insertions(+), 49 deletions(-) diff --git a/Include/cpython/critical_section.h b/Include/cpython/critical_section.h index 50baf97590a3f3..a451f232c6f32d 100644 --- a/Include/cpython/critical_section.h +++ b/Include/cpython/critical_section.h @@ -7,8 +7,8 @@ // These operations are no-ops in the default build. See // pycore_critical_section.h for details. -typedef struct _PyCriticalSection _PyCriticalSection; -typedef struct _PyCriticalSection2 _PyCriticalSection2; +typedef struct PyCriticalSection PyCriticalSection; +typedef struct PyCriticalSection2 PyCriticalSection2; #ifndef Py_GIL_DISABLED # define Py_BEGIN_CRITICAL_SECTION(op) \ @@ -21,8 +21,9 @@ typedef struct _PyCriticalSection2 _PyCriticalSection2; } #else /* !Py_GIL_DISABLED */ -// (private) -struct _PyCriticalSection { +// NOTE: the contents of this struct are private and may change betweeen +// Python releases without a deprecation period. +struct PyCriticalSection { // Tagged pointer to an outer active critical section (or 0). uintptr_t prev; @@ -30,49 +31,51 @@ struct _PyCriticalSection { PyMutex *mutex; }; -// (private) A critical section protected by two mutexes. Use +// A critical section protected by two mutexes. Use // Py_BEGIN_CRITICAL_SECTION2 and Py_END_CRITICAL_SECTION2. -struct _PyCriticalSection2 { - struct _PyCriticalSection base; +// NOTE: the contents of this struct are private and may change betweeen +// Python releases without a deprecation period. +struct PyCriticalSection2 { + PyCriticalSection base; PyMutex *mutex2; }; # define Py_BEGIN_CRITICAL_SECTION(op) \ { \ - _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, _PyObject_CAST(op)) + PyCriticalSection _cs; \ + PyCriticalSection_Begin(&_cs, _PyObject_CAST(op)) # define Py_END_CRITICAL_SECTION() \ - _PyCriticalSection_End(&_cs); \ + PyCriticalSection_End(&_cs); \ } # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ - _PyCriticalSection2 _cs2; \ - _PyCriticalSection2_Begin(&_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) + PyCriticalSection2 _cs2; \ + PyCriticalSection2_Begin(&_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) # define Py_END_CRITICAL_SECTION2() \ - _PyCriticalSection2_End(&_cs2); \ + PyCriticalSection2_End(&_cs2); \ } #endif // (private) PyAPI_FUNC(void) -_PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op); +PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op); // (private) PyAPI_FUNC(void) -_PyCriticalSection_End(_PyCriticalSection *c); +PyCriticalSection_End(PyCriticalSection *c); // (private) PyAPI_FUNC(void) -_PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyObject *a, PyObject *b); +PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b); // (private) PyAPI_FUNC(void) -_PyCriticalSection2_End(_PyCriticalSection2 *c); +PyCriticalSection2_End(PyCriticalSection2 *c); // CPython internals should use pycore_critical_section.h instead. #ifdef Py_BUILD_CORE diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 6183e3e7341270..8d3731219c6f3e 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -81,7 +81,7 @@ extern "C" { // Tagged pointers to critical sections use the two least significant bits to // mark if the pointed-to critical section is inactive and whether it is a -// _PyCriticalSection2 object. +// PyCriticalSection2 object. #define _Py_CRITICAL_SECTION_INACTIVE 0x1 #define _Py_CRITICAL_SECTION_TWO_MUTEXES 0x2 #define _Py_CRITICAL_SECTION_MASK 0x3 @@ -89,7 +89,7 @@ extern "C" { #ifdef Py_GIL_DISABLED # define Py_BEGIN_CRITICAL_SECTION_MUT(mutex) \ { \ - _PyCriticalSection _cs; \ + PyCriticalSection _cs; \ _PyCriticalSection_BeginInline(&_cs, mutex) # define Py_BEGIN_CRITICAL_SECTION(op) \ @@ -101,7 +101,7 @@ extern "C" { # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ - _PyCriticalSection2 _cs2; \ + PyCriticalSection2 _cs2; \ _PyCriticalSection2_BeginInline(&_cs2, \ &_PyObject_CAST(a)->ob_mutex, \ &_PyObject_CAST(b)->ob_mutex) @@ -119,14 +119,14 @@ extern "C" { { \ PyObject *_orig_seq = _PyObject_CAST(original); \ const bool _should_lock_cs = PyList_CheckExact(_orig_seq); \ - _PyCriticalSection _cs; \ + PyCriticalSection _cs; \ if (_should_lock_cs) { \ _PyCriticalSection_BeginInline(&_cs, &_orig_seq->ob_mutex); \ } # define Py_END_CRITICAL_SECTION_SEQUENCE_FAST() \ if (_should_lock_cs) { \ - _PyCriticalSection_End(&_cs); \ + PyCriticalSection_End(&_cs); \ } \ } @@ -165,18 +165,16 @@ extern "C" { # define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) #endif /* !Py_GIL_DISABLED */ -typedef struct _PyCriticalSection2 _PyCriticalSection2; - // Resumes the top-most critical section. PyAPI_FUNC(void) _PyCriticalSection_Resume(PyThreadState *tstate); // (private) slow path for locking the mutex PyAPI_FUNC(void) -_PyCriticalSection_BeginSlow(_PyCriticalSection *c, PyMutex *m); +_PyCriticalSection_BeginSlow(PyCriticalSection *c, PyMutex *m); PyAPI_FUNC(void) -_PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, +_PyCriticalSection2_BeginSlow(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, int is_m1_locked); PyAPI_FUNC(void) @@ -191,7 +189,7 @@ _PyCriticalSection_IsActive(uintptr_t tag) } static inline void -_PyCriticalSection_BeginInline(_PyCriticalSection *c, PyMutex *m) +_PyCriticalSection_BeginInline(PyCriticalSection *c, PyMutex *m) { if (PyMutex_LockFast(&m->_bits)) { PyThreadState *tstate = _PyThreadState_GET(); @@ -208,7 +206,7 @@ _PyCriticalSection_BeginInline(_PyCriticalSection *c, PyMutex *m) // sections. If the new top-most critical section is inactive, then it is // resumed. static inline void -_PyCriticalSection_Pop(_PyCriticalSection *c) +_PyCriticalSection_Pop(PyCriticalSection *c) { PyThreadState *tstate = _PyThreadState_GET(); uintptr_t prev = c->prev; @@ -220,14 +218,14 @@ _PyCriticalSection_Pop(_PyCriticalSection *c) } static inline void -_PyCriticalSection_EndInline(_PyCriticalSection *c) +_PyCriticalSection_EndInline(PyCriticalSection *c) { PyMutex_Unlock(c->mutex); _PyCriticalSection_Pop(c); } static inline void -_PyCriticalSection2_BeginInline(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) +_PyCriticalSection2_BeginInline(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) { if (m1 == m2) { // If the two mutex arguments are the same, treat this as a critical @@ -266,7 +264,7 @@ _PyCriticalSection2_BeginInline(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2 } static inline void -_PyCriticalSection2_EndInline(_PyCriticalSection2 *c) +_PyCriticalSection2_EndInline(PyCriticalSection2 *c) { if (c->mutex2) { PyMutex_Unlock(c->mutex2); @@ -282,11 +280,11 @@ _PyCriticalSection_AssertHeld(PyMutex *mutex) PyThreadState *tstate = _PyThreadState_GET(); uintptr_t prev = tstate->critical_section; if (prev & _Py_CRITICAL_SECTION_TWO_MUTEXES) { - _PyCriticalSection2 *cs = (_PyCriticalSection2 *)(prev & ~_Py_CRITICAL_SECTION_MASK); + PyCriticalSection2 *cs = (PyCriticalSection2 *)(prev & ~_Py_CRITICAL_SECTION_MASK); assert(cs != NULL && (cs->base.mutex == mutex || cs->mutex2 == mutex)); } else { - _PyCriticalSection *cs = (_PyCriticalSection *)(tstate->critical_section & ~_Py_CRITICAL_SECTION_MASK); + PyCriticalSection *cs = (PyCriticalSection *)(tstate->critical_section & ~_Py_CRITICAL_SECTION_MASK); assert(cs != NULL && cs->mutex == mutex); } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 16096ee13297b9..e01c568f73fcfd 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -64,7 +64,7 @@ class object "PyObject *" "&PyBaseObject_Type" #define TYPE_LOCK &PyInterpreterState_Get()->types.mutex #define BEGIN_TYPE_LOCK() \ { \ - _PyCriticalSection _cs; \ + PyCriticalSection _cs; \ _PyCriticalSection_BeginInline(&_cs, TYPE_LOCK); \ #define END_TYPE_LOCK() \ @@ -73,7 +73,7 @@ class object "PyObject *" "&PyBaseObject_Type" #define BEGIN_TYPE_DICT_LOCK(d) \ { \ - _PyCriticalSection2 _cs; \ + PyCriticalSection2 _cs; \ _PyCriticalSection2_BeginInline(&_cs, TYPE_LOCK, \ &_PyObject_CAST(d)->ob_mutex); \ diff --git a/Python/critical_section.c b/Python/critical_section.c index 0d60e12dfd9cb5..466f815d41c8d1 100644 --- a/Python/critical_section.c +++ b/Python/critical_section.c @@ -4,12 +4,12 @@ #include "pycore_critical_section.h" #ifdef Py_GIL_DISABLED -static_assert(_Alignof(_PyCriticalSection) >= 4, +static_assert(_Alignof(PyCriticalSection) >= 4, "critical section must be aligned to at least 4 bytes"); #endif void -_PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op) +PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op) { #ifdef Py_GIL_DISABLED _PyCriticalSection_BeginInline(c, &op->ob_mutex); @@ -17,7 +17,7 @@ _PyCriticalSection_Begin(_PyCriticalSection *c, PyObject *op) } void -_PyCriticalSection_End(_PyCriticalSection *c) +PyCriticalSection_End(PyCriticalSection *c) { #ifdef Py_GIL_DISABLED _PyCriticalSection_EndInline(c); @@ -25,7 +25,7 @@ _PyCriticalSection_End(_PyCriticalSection *c) } void -_PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyObject *a, PyObject *b) +PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b) { #ifdef Py_GIL_DISABLED _PyCriticalSection2_BeginInline(c, &a->ob_mutex, &b->ob_mutex); @@ -33,7 +33,7 @@ _PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyObject *a, PyObject *b) } void -_PyCriticalSection2_End(_PyCriticalSection2 *c) +PyCriticalSection2_End(PyCriticalSection2 *c) { #ifdef Py_GIL_DISABLED _PyCriticalSection2_EndInline(c); @@ -41,7 +41,7 @@ _PyCriticalSection2_End(_PyCriticalSection2 *c) } void -_PyCriticalSection_BeginSlow(_PyCriticalSection *c, PyMutex *m) +_PyCriticalSection_BeginSlow(PyCriticalSection *c, PyMutex *m) { #ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); @@ -55,7 +55,7 @@ _PyCriticalSection_BeginSlow(_PyCriticalSection *c, PyMutex *m) } void -_PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, +_PyCriticalSection2_BeginSlow(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, int is_m1_locked) { #ifdef Py_GIL_DISABLED @@ -75,10 +75,10 @@ _PyCriticalSection2_BeginSlow(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, } #ifdef Py_GIL_DISABLED -static _PyCriticalSection * +static PyCriticalSection * untag_critical_section(uintptr_t tag) { - return (_PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK); + return (PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK); } #endif @@ -90,12 +90,12 @@ _PyCriticalSection_SuspendAll(PyThreadState *tstate) #ifdef Py_GIL_DISABLED uintptr_t *tagptr = &tstate->critical_section; while (_PyCriticalSection_IsActive(*tagptr)) { - _PyCriticalSection *c = untag_critical_section(*tagptr); + PyCriticalSection *c = untag_critical_section(*tagptr); if (c->mutex) { PyMutex_Unlock(c->mutex); if ((*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { - _PyCriticalSection2 *c2 = (_PyCriticalSection2 *)c; + PyCriticalSection2 *c2 = (PyCriticalSection2 *)c; if (c2->mutex2) { PyMutex_Unlock(c2->mutex2); } @@ -113,16 +113,16 @@ _PyCriticalSection_Resume(PyThreadState *tstate) { #ifdef Py_GIL_DISABLED uintptr_t p = tstate->critical_section; - _PyCriticalSection *c = untag_critical_section(p); + PyCriticalSection *c = untag_critical_section(p); assert(!_PyCriticalSection_IsActive(p)); PyMutex *m1 = c->mutex; c->mutex = NULL; PyMutex *m2 = NULL; - _PyCriticalSection2 *c2 = NULL; + PyCriticalSection2 *c2 = NULL; if ((p & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { - c2 = (_PyCriticalSection2 *)c; + c2 = (PyCriticalSection2 *)c; m2 = c2->mutex2; c2->mutex2 = NULL; } From 3941794239e73273365436ae837ab71c04b7ca2a Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 20 Jun 2024 17:40:41 -0400 Subject: [PATCH 04/11] Clean up function definitions --- Include/cpython/critical_section.h | 122 ++++++++++++----- Include/internal/pycore_critical_section.h | 150 ++++++--------------- Objects/typeobject.c | 75 +++++------ Python/critical_section.c | 106 ++++++++------- 4 files changed, 214 insertions(+), 239 deletions(-) diff --git a/Include/cpython/critical_section.h b/Include/cpython/critical_section.h index a451f232c6f32d..72db930cc1fbbb 100644 --- a/Include/cpython/critical_section.h +++ b/Include/cpython/critical_section.h @@ -2,14 +2,86 @@ # error "this header file must not be included directly" #endif -// Python critical sections. +// Python critical sections // -// These operations are no-ops in the default build. See -// pycore_critical_section.h for details. +// Conceptually, critical sections are a deadlock avoidance layer on top of +// per-object locks. These helpers, in combination with those locks, replace +// our usage of the global interpreter lock to provide thread-safety for +// otherwise thread-unsafe objects, such as dict. +// +// NOTE: These APIs are no-ops in non-free-threaded builds. +// +// Straightforward per-object locking could introduce deadlocks that were not +// present when running with the GIL. Threads may hold locks for multiple +// objects simultaneously because Python operations can nest. If threads were +// to acquire the same locks in different orders, they would deadlock. +// +// One way to avoid deadlocks is to allow threads to hold only the lock (or +// locks) for a single operation at a time (typically a single lock, but some +// operations involve two locks). When a thread begins a nested operation it +// could suspend the locks for any outer operation: before beginning the nested +// operation, the locks for the outer operation are released and when the +// nested operation completes, the locks for the outer operation are +// reacquired. +// +// To improve performance, this API uses a variation of the above scheme. +// Instead of immediately suspending locks any time a nested operation begins, +// locks are only suspended if the thread would block. This reduces the number +// of lock acquisitions and releases for nested operations, while still +// avoiding deadlocks. +// +// Additionally, the locks for any active operation are suspended around +// other potentially blocking operations, such as I/O. This is because the +// interaction between locks and blocking operations can lead to deadlocks in +// the same way as the interaction between multiple locks. +// +// Each thread's critical sections and their corresponding locks are tracked in +// a stack in `PyThreadState.critical_section`. When a thread calls +// `_PyThreadState_Detach()`, such as before a blocking I/O operation or when +// waiting to acquire a lock, the thread suspends all of its active critical +// sections, temporarily releasing the associated locks. When the thread calls +// `_PyThreadState_Attach()`, it resumes the top-most (i.e., most recent) +// critical section by reacquiring the associated lock or locks. See +// `_PyCriticalSection_Resume()`. +// +// NOTE: Only the top-most critical section is guaranteed to be active. +// Operations that need to lock two objects at once must use +// `Py_BEGIN_CRITICAL_SECTION2()`. You *CANNOT* use nested critical sections +// to lock more than one object at once, because the inner critical section +// may suspend the outer critical sections. This API does not provide a way +// to lock more than two objects at once (though it could be added later +// if actually needed). +// +// NOTE: Critical sections implicitly behave like reentrant locks because +// attempting to acquire the same lock will suspend any outer (earlier) +// critical sections. However, they are less efficient for this use case than +// purposefully designed reentrant locks. +// +// Example usage: +// Py_BEGIN_CRITICAL_SECTION(op); +// ... +// Py_END_CRITICAL_SECTION(); +// +// To lock two objects at once: +// Py_BEGIN_CRITICAL_SECTION2(op1, op2); +// ... +// Py_END_CRITICAL_SECTION2(); typedef struct PyCriticalSection PyCriticalSection; typedef struct PyCriticalSection2 PyCriticalSection2; +PyAPI_FUNC(void) +PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op); + +PyAPI_FUNC(void) +PyCriticalSection_End(PyCriticalSection *c); + +PyAPI_FUNC(void) +PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b); + +PyAPI_FUNC(void) +PyCriticalSection2_End(PyCriticalSection2 *c); + #ifndef Py_GIL_DISABLED # define Py_BEGIN_CRITICAL_SECTION(op) \ { @@ -25,10 +97,10 @@ typedef struct PyCriticalSection2 PyCriticalSection2; // Python releases without a deprecation period. struct PyCriticalSection { // Tagged pointer to an outer active critical section (or 0). - uintptr_t prev; + uintptr_t _prev; // Mutex used to protect critical section - PyMutex *mutex; + PyMutex *_mutex; }; // A critical section protected by two mutexes. Use @@ -36,51 +108,27 @@ struct PyCriticalSection { // NOTE: the contents of this struct are private and may change betweeen // Python releases without a deprecation period. struct PyCriticalSection2 { - PyCriticalSection base; + PyCriticalSection _base; - PyMutex *mutex2; + PyMutex *_mutex2; }; # define Py_BEGIN_CRITICAL_SECTION(op) \ { \ - PyCriticalSection _cs; \ - PyCriticalSection_Begin(&_cs, _PyObject_CAST(op)) + PyCriticalSection _py_cs; \ + PyCriticalSection_Begin(&_py_cs, _PyObject_CAST(op)) # define Py_END_CRITICAL_SECTION() \ - PyCriticalSection_End(&_cs); \ + PyCriticalSection_End(&_py_cs); \ } # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ - PyCriticalSection2 _cs2; \ - PyCriticalSection2_Begin(&_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) + PyCriticalSection2 _py_cs2; \ + PyCriticalSection2_Begin(&_py_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) # define Py_END_CRITICAL_SECTION2() \ - PyCriticalSection2_End(&_cs2); \ + PyCriticalSection2_End(&_py_cs2); \ } #endif - -// (private) -PyAPI_FUNC(void) -PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op); - -// (private) -PyAPI_FUNC(void) -PyCriticalSection_End(PyCriticalSection *c); - -// (private) -PyAPI_FUNC(void) -PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b); - -// (private) -PyAPI_FUNC(void) -PyCriticalSection2_End(PyCriticalSection2 *c); - -// CPython internals should use pycore_critical_section.h instead. -#ifdef Py_BUILD_CORE -# undef Py_BEGIN_CRITICAL_SECTION -# undef Py_END_CRITICAL_SECTION -# undef Py_BEGIN_CRITICAL_SECTION2 -# undef Py_END_CRITICAL_SECTION2 -#endif diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 8d3731219c6f3e..0dc42ef4ec5d34 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -13,72 +13,6 @@ extern "C" { #endif -// Implementation of Python critical sections -// -// Conceptually, critical sections are a deadlock avoidance layer on top of -// per-object locks. These helpers, in combination with those locks, replace -// our usage of the global interpreter lock to provide thread-safety for -// otherwise thread-unsafe objects, such as dict. -// -// NOTE: These APIs are no-ops in non-free-threaded builds. -// -// Straightforward per-object locking could introduce deadlocks that were not -// present when running with the GIL. Threads may hold locks for multiple -// objects simultaneously because Python operations can nest. If threads were -// to acquire the same locks in different orders, they would deadlock. -// -// One way to avoid deadlocks is to allow threads to hold only the lock (or -// locks) for a single operation at a time (typically a single lock, but some -// operations involve two locks). When a thread begins a nested operation it -// could suspend the locks for any outer operation: before beginning the nested -// operation, the locks for the outer operation are released and when the -// nested operation completes, the locks for the outer operation are -// reacquired. -// -// To improve performance, this API uses a variation of the above scheme. -// Instead of immediately suspending locks any time a nested operation begins, -// locks are only suspended if the thread would block. This reduces the number -// of lock acquisitions and releases for nested operations, while still -// avoiding deadlocks. -// -// Additionally, the locks for any active operation are suspended around -// other potentially blocking operations, such as I/O. This is because the -// interaction between locks and blocking operations can lead to deadlocks in -// the same way as the interaction between multiple locks. -// -// Each thread's critical sections and their corresponding locks are tracked in -// a stack in `PyThreadState.critical_section`. When a thread calls -// `_PyThreadState_Detach()`, such as before a blocking I/O operation or when -// waiting to acquire a lock, the thread suspends all of its active critical -// sections, temporarily releasing the associated locks. When the thread calls -// `_PyThreadState_Attach()`, it resumes the top-most (i.e., most recent) -// critical section by reacquiring the associated lock or locks. See -// `_PyCriticalSection_Resume()`. -// -// NOTE: Only the top-most critical section is guaranteed to be active. -// Operations that need to lock two objects at once must use -// `Py_BEGIN_CRITICAL_SECTION2()`. You *CANNOT* use nested critical sections -// to lock more than one object at once, because the inner critical section -// may suspend the outer critical sections. This API does not provide a way -// to lock more than two objects at once (though it could be added later -// if actually needed). -// -// NOTE: Critical sections implicitly behave like reentrant locks because -// attempting to acquire the same lock will suspend any outer (earlier) -// critical sections. However, they are less efficient for this use case than -// purposefully designed reentrant locks. -// -// Example usage: -// Py_BEGIN_CRITICAL_SECTION(op); -// ... -// Py_END_CRITICAL_SECTION(); -// -// To lock two objects at once: -// Py_BEGIN_CRITICAL_SECTION2(op1, op2); -// ... -// Py_END_CRITICAL_SECTION2(); - - // Tagged pointers to critical sections use the two least significant bits to // mark if the pointed-to critical section is inactive and whether it is a // PyCriticalSection2 object. @@ -89,26 +23,13 @@ extern "C" { #ifdef Py_GIL_DISABLED # define Py_BEGIN_CRITICAL_SECTION_MUT(mutex) \ { \ - PyCriticalSection _cs; \ - _PyCriticalSection_BeginInline(&_cs, mutex) - -# define Py_BEGIN_CRITICAL_SECTION(op) \ - Py_BEGIN_CRITICAL_SECTION_MUT(&_PyObject_CAST(op)->ob_mutex) + PyCriticalSection _py_cs; \ + _PyCriticalSection_BeginMutex(&_py_cs, mutex) -# define Py_END_CRITICAL_SECTION() \ - _PyCriticalSection_EndInline(&_cs); \ - } - -# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ +# define Py_BEGIN_CRITICAL_SECTION2_MUT(m1, m2) \ { \ - PyCriticalSection2 _cs2; \ - _PyCriticalSection2_BeginInline(&_cs2, \ - &_PyObject_CAST(a)->ob_mutex, \ - &_PyObject_CAST(b)->ob_mutex) - -# define Py_END_CRITICAL_SECTION2() \ - _PyCriticalSection2_EndInline(&_cs2); \ - } + PyCriticalSection2 _py_cs2; \ + _PyCriticalSection2_BeginMutex(&_py_cs2, m1, m2) // Specialized version of critical section locking to safely use // PySequence_Fast APIs without the GIL. For performance, the argument *to* @@ -121,7 +42,7 @@ extern "C" { const bool _should_lock_cs = PyList_CheckExact(_orig_seq); \ PyCriticalSection _cs; \ if (_should_lock_cs) { \ - _PyCriticalSection_BeginInline(&_cs, &_orig_seq->ob_mutex); \ + _PyCriticalSection_Begin(&_cs, _orig_seq); \ } # define Py_END_CRITICAL_SECTION_SEQUENCE_FAST() \ @@ -155,10 +76,7 @@ extern "C" { #else /* !Py_GIL_DISABLED */ // The critical section APIs are no-ops with the GIL. # define Py_BEGIN_CRITICAL_SECTION_MUT(mut) { -# define Py_BEGIN_CRITICAL_SECTION(op) { -# define Py_END_CRITICAL_SECTION() } -# define Py_BEGIN_CRITICAL_SECTION2(a, b) { -# define Py_END_CRITICAL_SECTION2() } +# define Py_BEGIN_CRITICAL_SECTION2_MUT(m1, m2) { # define Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST(original) { # define Py_END_CRITICAL_SECTION_SEQUENCE_FAST() } # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) @@ -189,12 +107,12 @@ _PyCriticalSection_IsActive(uintptr_t tag) } static inline void -_PyCriticalSection_BeginInline(PyCriticalSection *c, PyMutex *m) +_PyCriticalSection_BeginMutex(PyCriticalSection *c, PyMutex *m) { if (PyMutex_LockFast(&m->_bits)) { PyThreadState *tstate = _PyThreadState_GET(); - c->mutex = m; - c->prev = tstate->critical_section; + c->_mutex = m; + c->_prev = tstate->critical_section; tstate->critical_section = (uintptr_t)c; } else { @@ -202,6 +120,13 @@ _PyCriticalSection_BeginInline(PyCriticalSection *c, PyMutex *m) } } +static inline void +_PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op) +{ + _PyCriticalSection_BeginMutex(c, &op->ob_mutex); +} +#define PyCriticalSection_Begin _PyCriticalSection_Begin + // Removes the top-most critical section from the thread's stack of critical // sections. If the new top-most critical section is inactive, then it is // resumed. @@ -209,7 +134,7 @@ static inline void _PyCriticalSection_Pop(PyCriticalSection *c) { PyThreadState *tstate = _PyThreadState_GET(); - uintptr_t prev = c->prev; + uintptr_t prev = c->_prev; tstate->critical_section = prev; if ((prev & _Py_CRITICAL_SECTION_INACTIVE) != 0) { @@ -218,20 +143,21 @@ _PyCriticalSection_Pop(PyCriticalSection *c) } static inline void -_PyCriticalSection_EndInline(PyCriticalSection *c) +_PyCriticalSection_End(PyCriticalSection *c) { - PyMutex_Unlock(c->mutex); + PyMutex_Unlock(c->_mutex); _PyCriticalSection_Pop(c); } +#define PyCriticalSection_End _PyCriticalSection_End static inline void -_PyCriticalSection2_BeginInline(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) +_PyCriticalSection2_BeginMutex(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) { if (m1 == m2) { // If the two mutex arguments are the same, treat this as a critical // section with a single mutex. - c->mutex2 = NULL; - _PyCriticalSection_BeginInline(&c->base, m1); + c->_mutex2 = NULL; + _PyCriticalSection_BeginMutex(&c->_base, m1); return; } @@ -247,9 +173,9 @@ _PyCriticalSection2_BeginInline(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) if (PyMutex_LockFast(&m1->_bits)) { if (PyMutex_LockFast(&m2->_bits)) { PyThreadState *tstate = _PyThreadState_GET(); - c->base.mutex = m1; - c->mutex2 = m2; - c->base.prev = tstate->critical_section; + c->_base._mutex = m1; + c->_mutex2 = m2; + c->_base._prev = tstate->critical_section; uintptr_t p = (uintptr_t)c | _Py_CRITICAL_SECTION_TWO_MUTEXES; tstate->critical_section = p; @@ -264,14 +190,22 @@ _PyCriticalSection2_BeginInline(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) } static inline void -_PyCriticalSection2_EndInline(PyCriticalSection2 *c) +_PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b) +{ + _PyCriticalSection2_BeginMutex(c, &a->ob_mutex, &b->ob_mutex); +} +#define PyCriticalSection2_Begin _PyCriticalSection2_Begin + +static inline void +_PyCriticalSection2_End(PyCriticalSection2 *c) { - if (c->mutex2) { - PyMutex_Unlock(c->mutex2); + if (c->_mutex2) { + PyMutex_Unlock(c->_mutex2); } - PyMutex_Unlock(c->base.mutex); - _PyCriticalSection_Pop(&c->base); + PyMutex_Unlock(c->_base._mutex); + _PyCriticalSection_Pop(&c->_base); } +#define PyCriticalSection2_End _PyCriticalSection2_End static inline void _PyCriticalSection_AssertHeld(PyMutex *mutex) @@ -281,11 +215,11 @@ _PyCriticalSection_AssertHeld(PyMutex *mutex) uintptr_t prev = tstate->critical_section; if (prev & _Py_CRITICAL_SECTION_TWO_MUTEXES) { PyCriticalSection2 *cs = (PyCriticalSection2 *)(prev & ~_Py_CRITICAL_SECTION_MASK); - assert(cs != NULL && (cs->base.mutex == mutex || cs->mutex2 == mutex)); + assert(cs != NULL && (cs->_base._mutex == mutex || cs->_mutex2 == mutex)); } else { PyCriticalSection *cs = (PyCriticalSection *)(tstate->critical_section & ~_Py_CRITICAL_SECTION_MASK); - assert(cs != NULL && cs->mutex == mutex); + assert(cs != NULL && cs->_mutex == mutex); } #endif diff --git a/Objects/typeobject.c b/Objects/typeobject.c index e01c568f73fcfd..e29482de1a8a3b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -62,24 +62,13 @@ class object "PyObject *" "&PyBaseObject_Type" // be released and reacquired during a subclass update if there's contention // on the subclass lock. #define TYPE_LOCK &PyInterpreterState_Get()->types.mutex -#define BEGIN_TYPE_LOCK() \ - { \ - PyCriticalSection _cs; \ - _PyCriticalSection_BeginInline(&_cs, TYPE_LOCK); \ +#define BEGIN_TYPE_LOCK() Py_BEGIN_CRITICAL_SECTION_MUT(TYPE_LOCK) +#define END_TYPE_LOCK() Py_END_CRITICAL_SECTION() -#define END_TYPE_LOCK() \ - _PyCriticalSection_EndInline(&_cs); \ - } - -#define BEGIN_TYPE_DICT_LOCK(d) \ - { \ - PyCriticalSection2 _cs; \ - _PyCriticalSection2_BeginInline(&_cs, TYPE_LOCK, \ - &_PyObject_CAST(d)->ob_mutex); \ +#define BEGIN_TYPE_DICT_LOCK(d) \ + Py_BEGIN_CRITICAL_SECTION2_MUT(TYPE_LOCK, &_PyObject_CAST(d)->ob_mutex) -#define END_TYPE_DICT_LOCK() \ - _PyCriticalSection2_EndInline(&_cs); \ - } +#define END_TYPE_DICT_LOCK() Py_END_CRITICAL_SECTION2() #define ASSERT_TYPE_LOCK_HELD() \ _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(TYPE_LOCK) @@ -442,7 +431,7 @@ _PyType_GetBases(PyTypeObject *self) BEGIN_TYPE_LOCK(); res = lookup_tp_bases(self); Py_INCREF(res); - END_TYPE_LOCK() + END_TYPE_LOCK(); return res; } @@ -513,7 +502,7 @@ _PyType_GetMRO(PyTypeObject *self) BEGIN_TYPE_LOCK(); mro = lookup_tp_mro(self); Py_XINCREF(mro); - END_TYPE_LOCK() + END_TYPE_LOCK(); return mro; #else return Py_XNewRef(lookup_tp_mro(self)); @@ -951,10 +940,10 @@ PyType_Watch(int watcher_id, PyObject* obj) return -1; } // ensure we will get a callback on the next modification - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); assign_version_tag(interp, type); type->tp_watched |= (1 << watcher_id); - END_TYPE_LOCK() + END_TYPE_LOCK(); return 0; } @@ -1080,9 +1069,9 @@ PyType_Modified(PyTypeObject *type) return; } - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); type_modified_unlocked(type); - END_TYPE_LOCK() + END_TYPE_LOCK(); } static int @@ -1161,9 +1150,9 @@ void _PyType_SetVersion(PyTypeObject *tp, unsigned int version) { - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); set_version_unlocked(tp, version); - END_TYPE_LOCK() + END_TYPE_LOCK(); } PyTypeObject * @@ -1245,9 +1234,9 @@ int PyUnstable_Type_AssignVersionTag(PyTypeObject *type) { PyInterpreterState *interp = _PyInterpreterState_GET(); int assigned; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); assigned = assign_version_tag(interp, type); - END_TYPE_LOCK() + END_TYPE_LOCK(); return assigned; } @@ -1530,7 +1519,7 @@ type_get_mro(PyTypeObject *type, void *context) { PyObject *mro; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); mro = lookup_tp_mro(type); if (mro == NULL) { mro = Py_None; @@ -1538,7 +1527,7 @@ type_get_mro(PyTypeObject *type, void *context) Py_INCREF(mro); } - END_TYPE_LOCK() + END_TYPE_LOCK(); return mro; } @@ -3119,9 +3108,9 @@ static PyObject * mro_implementation(PyTypeObject *type) { PyObject *mro; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); mro = mro_implementation_unlocked(type); - END_TYPE_LOCK() + END_TYPE_LOCK(); return mro; } @@ -3308,9 +3297,9 @@ static int mro_internal(PyTypeObject *type, PyObject **p_old_mro) { int res; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); res = mro_internal_unlocked(type, 0, p_old_mro); - END_TYPE_LOCK() + END_TYPE_LOCK(); return res; } @@ -5171,7 +5160,7 @@ get_module_by_def(PyTypeObject *type, PyModuleDef *def) } PyObject *res = NULL; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); PyObject *mro = lookup_tp_mro(type); // The type must be ready @@ -5198,7 +5187,7 @@ get_module_by_def(PyTypeObject *type, PyModuleDef *def) break; } } - END_TYPE_LOCK() + END_TYPE_LOCK(); return res; } @@ -5456,13 +5445,13 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) int has_version = 0; int version = 0; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); res = find_name_in_mro(type, name, &error); if (MCACHE_CACHEABLE_NAME(name)) { has_version = assign_version_tag(interp, type); version = type->tp_version_tag; } - END_TYPE_LOCK() + END_TYPE_LOCK(); /* Only put NULL results into cache if there was no error. */ if (error) { @@ -8452,14 +8441,14 @@ PyType_Ready(PyTypeObject *type) } int res; - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); if (!(type->tp_flags & Py_TPFLAGS_READY)) { res = type_ready(type, 1); } else { res = 0; assert(_PyType_CheckConsistency(type)); } - END_TYPE_LOCK() + END_TYPE_LOCK(); return res; } @@ -8493,7 +8482,7 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, int res; BEGIN_TYPE_LOCK(); res = type_ready(self, initial); - END_TYPE_LOCK() + END_TYPE_LOCK(); if (res < 0) { _PyStaticType_ClearWeakRefs(interp, self); managed_static_type_state_clear(interp, self, isbuiltin, initial); @@ -8965,7 +8954,7 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) int res; BEGIN_TYPE_LOCK(); res = hackcheck_unlocked(self, func, what); - END_TYPE_LOCK() + END_TYPE_LOCK(); return res; } @@ -10894,14 +10883,14 @@ fixup_slot_dispatchers(PyTypeObject *type) // This lock isn't strictly necessary because the type has not been // exposed to anyone else yet, but update_ont_slot calls find_name_in_mro // where we'd like to assert that the type is locked. - BEGIN_TYPE_LOCK() + BEGIN_TYPE_LOCK(); assert(!PyErr_Occurred()); for (pytype_slotdef *p = slotdefs; p->name; ) { p = update_one_slot(type, p); } - END_TYPE_LOCK() + END_TYPE_LOCK(); } static void @@ -11190,7 +11179,7 @@ _super_lookup_descr(PyTypeObject *su_type, PyTypeObject *su_obj_type, PyObject * another thread can modify it after we end the critical section below */ Py_XINCREF(mro); - END_TYPE_LOCK() + END_TYPE_LOCK(); if (mro == NULL) return NULL; diff --git a/Python/critical_section.c b/Python/critical_section.c index 466f815d41c8d1..698d354c191649 100644 --- a/Python/critical_section.c +++ b/Python/critical_section.c @@ -8,49 +8,17 @@ static_assert(_Alignof(PyCriticalSection) >= 4, "critical section must be aligned to at least 4 bytes"); #endif -void -PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op) -{ -#ifdef Py_GIL_DISABLED - _PyCriticalSection_BeginInline(c, &op->ob_mutex); -#endif -} - -void -PyCriticalSection_End(PyCriticalSection *c) -{ -#ifdef Py_GIL_DISABLED - _PyCriticalSection_EndInline(c); -#endif -} - -void -PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b) -{ -#ifdef Py_GIL_DISABLED - _PyCriticalSection2_BeginInline(c, &a->ob_mutex, &b->ob_mutex); -#endif -} - -void -PyCriticalSection2_End(PyCriticalSection2 *c) -{ -#ifdef Py_GIL_DISABLED - _PyCriticalSection2_EndInline(c); -#endif -} - void _PyCriticalSection_BeginSlow(PyCriticalSection *c, PyMutex *m) { #ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); - c->mutex = NULL; - c->prev = (uintptr_t)tstate->critical_section; + c->_mutex = NULL; + c->_prev = (uintptr_t)tstate->critical_section; tstate->critical_section = (uintptr_t)c; PyMutex_Lock(m); - c->mutex = m; + c->_mutex = m; #endif } @@ -60,17 +28,17 @@ _PyCriticalSection2_BeginSlow(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, { #ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); - c->base.mutex = NULL; - c->mutex2 = NULL; - c->base.prev = tstate->critical_section; + c->_base._mutex = NULL; + c->_mutex2 = NULL; + c->_base._prev = tstate->critical_section; tstate->critical_section = (uintptr_t)c | _Py_CRITICAL_SECTION_TWO_MUTEXES; if (!is_m1_locked) { PyMutex_Lock(m1); } PyMutex_Lock(m2); - c->base.mutex = m1; - c->mutex2 = m2; + c->_base._mutex = m1; + c->_mutex2 = m2; #endif } @@ -92,18 +60,18 @@ _PyCriticalSection_SuspendAll(PyThreadState *tstate) while (_PyCriticalSection_IsActive(*tagptr)) { PyCriticalSection *c = untag_critical_section(*tagptr); - if (c->mutex) { - PyMutex_Unlock(c->mutex); + if (c->_mutex) { + PyMutex_Unlock(c->_mutex); if ((*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { PyCriticalSection2 *c2 = (PyCriticalSection2 *)c; - if (c2->mutex2) { - PyMutex_Unlock(c2->mutex2); + if (c2->_mutex2) { + PyMutex_Unlock(c2->_mutex2); } } } *tagptr |= _Py_CRITICAL_SECTION_INACTIVE; - tagptr = &c->prev; + tagptr = &c->_prev; } #endif } @@ -116,15 +84,15 @@ _PyCriticalSection_Resume(PyThreadState *tstate) PyCriticalSection *c = untag_critical_section(p); assert(!_PyCriticalSection_IsActive(p)); - PyMutex *m1 = c->mutex; - c->mutex = NULL; + PyMutex *m1 = c->_mutex; + c->_mutex = NULL; PyMutex *m2 = NULL; PyCriticalSection2 *c2 = NULL; if ((p & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { c2 = (PyCriticalSection2 *)c; - m2 = c2->mutex2; - c2->mutex2 = NULL; + m2 = c2->_mutex2; + c2->_mutex2 = NULL; } if (m1) { @@ -134,11 +102,47 @@ _PyCriticalSection_Resume(PyThreadState *tstate) PyMutex_Lock(m2); } - c->mutex = m1; + c->_mutex = m1; if (m2) { - c2->mutex2 = m2; + c2->_mutex2 = m2; } tstate->critical_section &= ~_Py_CRITICAL_SECTION_INACTIVE; #endif } + +#undef PyCriticalSection_Begin +void +PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection_Begin(c, op); +#endif +} + +#undef PyCriticalSection_End +void +PyCriticalSection_End(PyCriticalSection *c) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection_End(c); +#endif +} + +#undef PyCriticalSection2_Begin +void +PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection2_Begin(c, a, b); +#endif +} + +#undef PyCriticalSection2_End +void +PyCriticalSection2_End(PyCriticalSection2 *c) +{ +#ifdef Py_GIL_DISABLED + _PyCriticalSection2_End(c); +#endif +} From 0e7873b7fbb0a08dab353c279f1f83a50ab8b04e Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 20 Jun 2024 18:59:00 -0400 Subject: [PATCH 05/11] Fix C syntax error --- Objects/dictobject.c | 2 +- Objects/listobject.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 32799bf5210fc3..55bef8da1bc1eb 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3116,7 +3116,7 @@ _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value) goto dict_iter_exit; } } -dict_iter_exit: +dict_iter_exit:; Py_END_CRITICAL_SECTION(); } else { while ((key = PyIter_Next(it)) != NULL) { diff --git a/Objects/listobject.c b/Objects/listobject.c index 6829d5d28656cf..ed19a5df563379 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -452,7 +452,7 @@ PyList_SetItem(PyObject *op, Py_ssize_t i, p = self->ob_item + i; Py_XSETREF(*p, newitem); ret = 0; -end: +end:; Py_END_CRITICAL_SECTION(); return ret; } From 4c7df8dba7fc480849dba02a4e10dba87ae7f571 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 20 Jun 2024 19:06:14 -0400 Subject: [PATCH 06/11] Add documentation --- Doc/c-api/init.rst | 97 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 58c79031de5320..b1a06b0043cc17 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -2195,3 +2195,100 @@ The C-API provides a basic mutual exclusion lock. issue a fatal error. .. versionadded:: 3.13 + +Python Critical Section API +--------------------------- + +The critical section API provides a deadlock avoidance layer on top of +per-object locks for :term:`free-threaded ` CPython. They are +intended to replace reliance on the :term:`global interpreter lock`, and are +no-ops in versions of Python with the global interpreter lock. + +Critical sections avoid deadlocks by implicitly suspending active critical +sections and releasing the locks during calls to :c:func:`PyEval_SaveThread`. +When :c:func:`PyEval_RestoreThread` is called, the most recent critical section +is resumed, and its locks reacquired. This means the critical section API +provides weaker guarantees than traditional locks -- they are useful because +their behavior is similar to the :term:`GIL`. + +.. note:: + + Operations that need to lock two objects at once must use + :c:macro:`Py_BEGIN_CRITICAL_SECTION2()`. You *cannot* use nested critical + sections to lock more than one object at once, because the inner critical + section may suspend the outer critical sections. This API does not provide + a way to lock more than two objects at once. + +Example usage:: + + static PyObject * + set_field(MyObject *self, PyObject *value) + { + Py_BEGIN_CRITICAL_SECTION(self); + Py_SETREF(self->field, value); + Py_END_CRITICAL_SECTION(); + Py_RETURN_NONE; + } + +In the above example, :c:macro:`Py_SETREF` calls :c:macro:`Py_DECREF`, which +can call arbitrary code through an object's deallocation function. The critical +section API avoids potentital deadlocks due to reentrancy and lock ordering +by allowing the runtime to temporarily suspend the critical section if the +code triggered by the finalizer blocks and calls :c:func:`PyEval_SaveThread`. + +.. c:macro:: Py_BEGIN_CRITICAL_SECTION(op) + + Acquires the per-object lock for the object *op* and begins a + critical section. + + In the free-threaded build, this macro expands to:: + + { + PyCriticalSection _py_cs; + PyCriticalSection_Begin(&_py_cs, _PyObject_CAST(op)) + + In the default build, this macro expands to ``{``. + + .. versionadded:: 3.13 + +.. c:macro:: Py_END_CRITICAL_SECTION() + + Ends the critical section and releases the per-object lock. + + In the free-threaded build, this macro expands to:: + + PyCriticalSection_End(&_py_cs); + } + + In the default build, this macro expands to ``}``. + + .. versionadded:: 3.13 + +.. c:macro:: Py_BEGIN_CRITICAL_SECTION2(a, b) + + Acquires the per-objects locks for the objects *a* and *b* and begins a + critical section. The locks are acquired in a consistent order (lowest + address first) to avoid lock ordering deadlocks. + + In the free-threaded build, this macro expands to:: + + { + PyCriticalSection2 _py_cs2; + PyCriticalSection_Begin2(&_py_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) + + In the default build, this macro expands to ``{``. + + .. versionadded:: 3.13 + +.. c:macro:: Py_END_CRITICAL_SECTION2() + + Ends the critical section and releases the per-object locks. + + In the free-threaded build, this macro expands to:: + + PyCriticalSection_End2(&_py_cs2); + } + + In the default build, this macro expands to ``}``. + + .. versionadded:: 3.13 From c61ba72b3d1c7f41957f88c2bf47aa0d3133a3a1 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 20 Jun 2024 19:12:10 -0400 Subject: [PATCH 07/11] Fix syntax error --- Modules/_sre/sre.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sre/sre.c b/Modules/_sre/sre.c index 0c656b47991c2f..0a888af31b0497 100644 --- a/Modules/_sre/sre.c +++ b/Modules/_sre/sre.c @@ -2371,7 +2371,7 @@ _sre_SRE_Match_groupdict_impl(MatchObject *self, PyObject *default_value) goto exit; } } -exit: +exit:; Py_END_CRITICAL_SECTION(); return result; From 89405ca8cd51cd9c7a4172d0d2725b8446d7a659 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 10:52:49 -0400 Subject: [PATCH 08/11] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Petr Viktorin Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Victor Stinner --- Doc/c-api/init.rst | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index b1a06b0043cc17..787c6a094bb31e 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -2211,10 +2211,15 @@ is resumed, and its locks reacquired. This means the critical section API provides weaker guarantees than traditional locks -- they are useful because their behavior is similar to the :term:`GIL`. +The functions and structs used by the macros are exposed for cases +where C macros are not available. They should only be used as in the +given macro expansions. Note that the sizes of the structures may +change in future Python versions. + .. note:: Operations that need to lock two objects at once must use - :c:macro:`Py_BEGIN_CRITICAL_SECTION2()`. You *cannot* use nested critical + :c:macro:`Py_BEGIN_CRITICAL_SECTION2`. You *cannot* use nested critical sections to lock more than one object at once, because the inner critical section may suspend the outer critical sections. This API does not provide a way to lock more than two objects at once. @@ -2225,7 +2230,7 @@ Example usage:: set_field(MyObject *self, PyObject *value) { Py_BEGIN_CRITICAL_SECTION(self); - Py_SETREF(self->field, value); + Py_SETREF(self->field, Py_XNewRef(value)); Py_END_CRITICAL_SECTION(); Py_RETURN_NONE; } @@ -2245,7 +2250,7 @@ code triggered by the finalizer blocks and calls :c:func:`PyEval_SaveThread`. { PyCriticalSection _py_cs; - PyCriticalSection_Begin(&_py_cs, _PyObject_CAST(op)) + PyCriticalSection_Begin(&_py_cs, (PyObject*)(op)) In the default build, this macro expands to ``{``. @@ -2274,7 +2279,7 @@ code triggered by the finalizer blocks and calls :c:func:`PyEval_SaveThread`. { PyCriticalSection2 _py_cs2; - PyCriticalSection_Begin2(&_py_cs2, _PyObject_CAST(a), _PyObject_CAST(b)) + PyCriticalSection_Begin2(&_py_cs2, (PyObject*)(a), (PyObject*)(b)) In the default build, this macro expands to ``{``. From dc50fff9093dc15349a2a67818fd9faf2117ed77 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 10:53:38 -0400 Subject: [PATCH 09/11] Sizes and contents --- Doc/c-api/init.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 787c6a094bb31e..59ee6a0163d5ce 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -2213,7 +2213,7 @@ their behavior is similar to the :term:`GIL`. The functions and structs used by the macros are exposed for cases where C macros are not available. They should only be used as in the -given macro expansions. Note that the sizes of the structures may +given macro expansions. Note that the sizes and contents of the structures may change in future Python versions. .. note:: From c3073b0cb12229a1d09cf3619e6825fa58754d09 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 10:57:06 -0400 Subject: [PATCH 10/11] Rename PyCriticalSection fields --- Include/cpython/critical_section.h | 8 ++--- Include/internal/pycore_critical_section.h | 30 ++++++++--------- Python/critical_section.c | 38 +++++++++++----------- 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/Include/cpython/critical_section.h b/Include/cpython/critical_section.h index 72db930cc1fbbb..35db3fb6a59ce6 100644 --- a/Include/cpython/critical_section.h +++ b/Include/cpython/critical_section.h @@ -97,10 +97,10 @@ PyCriticalSection2_End(PyCriticalSection2 *c); // Python releases without a deprecation period. struct PyCriticalSection { // Tagged pointer to an outer active critical section (or 0). - uintptr_t _prev; + uintptr_t _cs_prev; // Mutex used to protect critical section - PyMutex *_mutex; + PyMutex *_cs_mutex; }; // A critical section protected by two mutexes. Use @@ -108,9 +108,9 @@ struct PyCriticalSection { // NOTE: the contents of this struct are private and may change betweeen // Python releases without a deprecation period. struct PyCriticalSection2 { - PyCriticalSection _base; + PyCriticalSection _cs_base; - PyMutex *_mutex2; + PyMutex *_cs_mutex2; }; # define Py_BEGIN_CRITICAL_SECTION(op) \ diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 0dc42ef4ec5d34..78cd0d54972660 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -111,8 +111,8 @@ _PyCriticalSection_BeginMutex(PyCriticalSection *c, PyMutex *m) { if (PyMutex_LockFast(&m->_bits)) { PyThreadState *tstate = _PyThreadState_GET(); - c->_mutex = m; - c->_prev = tstate->critical_section; + c->_cs_mutex = m; + c->_cs_prev = tstate->critical_section; tstate->critical_section = (uintptr_t)c; } else { @@ -134,7 +134,7 @@ static inline void _PyCriticalSection_Pop(PyCriticalSection *c) { PyThreadState *tstate = _PyThreadState_GET(); - uintptr_t prev = c->_prev; + uintptr_t prev = c->_cs_prev; tstate->critical_section = prev; if ((prev & _Py_CRITICAL_SECTION_INACTIVE) != 0) { @@ -145,7 +145,7 @@ _PyCriticalSection_Pop(PyCriticalSection *c) static inline void _PyCriticalSection_End(PyCriticalSection *c) { - PyMutex_Unlock(c->_mutex); + PyMutex_Unlock(c->_cs_mutex); _PyCriticalSection_Pop(c); } #define PyCriticalSection_End _PyCriticalSection_End @@ -156,8 +156,8 @@ _PyCriticalSection2_BeginMutex(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) if (m1 == m2) { // If the two mutex arguments are the same, treat this as a critical // section with a single mutex. - c->_mutex2 = NULL; - _PyCriticalSection_BeginMutex(&c->_base, m1); + c->_cs_mutex2 = NULL; + _PyCriticalSection_BeginMutex(&c->_cs_base, m1); return; } @@ -173,9 +173,9 @@ _PyCriticalSection2_BeginMutex(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) if (PyMutex_LockFast(&m1->_bits)) { if (PyMutex_LockFast(&m2->_bits)) { PyThreadState *tstate = _PyThreadState_GET(); - c->_base._mutex = m1; - c->_mutex2 = m2; - c->_base._prev = tstate->critical_section; + c->_cs_base._cs_mutex = m1; + c->_cs_mutex2 = m2; + c->_cs_base._cs_prev = tstate->critical_section; uintptr_t p = (uintptr_t)c | _Py_CRITICAL_SECTION_TWO_MUTEXES; tstate->critical_section = p; @@ -199,11 +199,11 @@ _PyCriticalSection2_Begin(PyCriticalSection2 *c, PyObject *a, PyObject *b) static inline void _PyCriticalSection2_End(PyCriticalSection2 *c) { - if (c->_mutex2) { - PyMutex_Unlock(c->_mutex2); + if (c->_cs_mutex2) { + PyMutex_Unlock(c->_cs_mutex2); } - PyMutex_Unlock(c->_base._mutex); - _PyCriticalSection_Pop(&c->_base); + PyMutex_Unlock(c->_cs_base._cs_mutex); + _PyCriticalSection_Pop(&c->_cs_base); } #define PyCriticalSection2_End _PyCriticalSection2_End @@ -215,11 +215,11 @@ _PyCriticalSection_AssertHeld(PyMutex *mutex) uintptr_t prev = tstate->critical_section; if (prev & _Py_CRITICAL_SECTION_TWO_MUTEXES) { PyCriticalSection2 *cs = (PyCriticalSection2 *)(prev & ~_Py_CRITICAL_SECTION_MASK); - assert(cs != NULL && (cs->_base._mutex == mutex || cs->_mutex2 == mutex)); + assert(cs != NULL && (cs->_cs_base._cs_mutex == mutex || cs->_cs_mutex2 == mutex)); } else { PyCriticalSection *cs = (PyCriticalSection *)(tstate->critical_section & ~_Py_CRITICAL_SECTION_MASK); - assert(cs != NULL && cs->_mutex == mutex); + assert(cs != NULL && cs->_cs_mutex == mutex); } #endif diff --git a/Python/critical_section.c b/Python/critical_section.c index 698d354c191649..62ed25523fd6dc 100644 --- a/Python/critical_section.c +++ b/Python/critical_section.c @@ -13,12 +13,12 @@ _PyCriticalSection_BeginSlow(PyCriticalSection *c, PyMutex *m) { #ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); - c->_mutex = NULL; - c->_prev = (uintptr_t)tstate->critical_section; + c->_cs_mutex = NULL; + c->_cs_prev = (uintptr_t)tstate->critical_section; tstate->critical_section = (uintptr_t)c; PyMutex_Lock(m); - c->_mutex = m; + c->_cs_mutex = m; #endif } @@ -28,17 +28,17 @@ _PyCriticalSection2_BeginSlow(PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2, { #ifdef Py_GIL_DISABLED PyThreadState *tstate = _PyThreadState_GET(); - c->_base._mutex = NULL; - c->_mutex2 = NULL; - c->_base._prev = tstate->critical_section; + c->_cs_base._cs_mutex = NULL; + c->_cs_mutex2 = NULL; + c->_cs_base._cs_prev = tstate->critical_section; tstate->critical_section = (uintptr_t)c | _Py_CRITICAL_SECTION_TWO_MUTEXES; if (!is_m1_locked) { PyMutex_Lock(m1); } PyMutex_Lock(m2); - c->_base._mutex = m1; - c->_mutex2 = m2; + c->_cs_base._cs_mutex = m1; + c->_cs_mutex2 = m2; #endif } @@ -60,18 +60,18 @@ _PyCriticalSection_SuspendAll(PyThreadState *tstate) while (_PyCriticalSection_IsActive(*tagptr)) { PyCriticalSection *c = untag_critical_section(*tagptr); - if (c->_mutex) { - PyMutex_Unlock(c->_mutex); + if (c->_cs_mutex) { + PyMutex_Unlock(c->_cs_mutex); if ((*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { PyCriticalSection2 *c2 = (PyCriticalSection2 *)c; - if (c2->_mutex2) { - PyMutex_Unlock(c2->_mutex2); + if (c2->_cs_mutex2) { + PyMutex_Unlock(c2->_cs_mutex2); } } } *tagptr |= _Py_CRITICAL_SECTION_INACTIVE; - tagptr = &c->_prev; + tagptr = &c->_cs_prev; } #endif } @@ -84,15 +84,15 @@ _PyCriticalSection_Resume(PyThreadState *tstate) PyCriticalSection *c = untag_critical_section(p); assert(!_PyCriticalSection_IsActive(p)); - PyMutex *m1 = c->_mutex; - c->_mutex = NULL; + PyMutex *m1 = c->_cs_mutex; + c->_cs_mutex = NULL; PyMutex *m2 = NULL; PyCriticalSection2 *c2 = NULL; if ((p & _Py_CRITICAL_SECTION_TWO_MUTEXES)) { c2 = (PyCriticalSection2 *)c; - m2 = c2->_mutex2; - c2->_mutex2 = NULL; + m2 = c2->_cs_mutex2; + c2->_cs_mutex2 = NULL; } if (m1) { @@ -102,9 +102,9 @@ _PyCriticalSection_Resume(PyThreadState *tstate) PyMutex_Lock(m2); } - c->_mutex = m1; + c->_cs_mutex = m1; if (m2) { - c2->_mutex2 = m2; + c2->_cs_mutex2 = m2; } tstate->critical_section &= ~_Py_CRITICAL_SECTION_INACTIVE; From 493d062bfd0376e79034e1dda90521e9ed6aeb5c Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 10:58:31 -0400 Subject: [PATCH 11/11] Add section label --- Doc/c-api/init.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 59ee6a0163d5ce..10a2aa57d5f57e 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -2196,6 +2196,8 @@ The C-API provides a basic mutual exclusion lock. .. versionadded:: 3.13 +.. _python-critical-section-api: + Python Critical Section API ---------------------------