From fb807e3e4de811d064fa8566ffe6a8b3a57af1ba Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 30 Aug 2023 12:26:46 -0600 Subject: [PATCH 01/11] Use the raw allocator. --- Modules/_xxsubinterpretersmodule.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index ea91e70cad991d..7dbd837398a1c5 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -168,16 +168,16 @@ typedef struct _sharedns { static _sharedns * _sharedns_new(Py_ssize_t len) { - _sharedns *shared = PyMem_NEW(_sharedns, 1); + _sharedns *shared = PyMem_RawCalloc(sizeof(_sharedns), 1); if (shared == NULL) { PyErr_NoMemory(); return NULL; } shared->len = len; - shared->items = PyMem_NEW(struct _sharednsitem, len); + shared->items = PyMem_RawCalloc(sizeof(struct _sharednsitem), len); if (shared->items == NULL) { PyErr_NoMemory(); - PyMem_Free(shared); + PyMem_RawFree(shared); return NULL; } return shared; @@ -189,8 +189,8 @@ _sharedns_free(_sharedns *shared) for (Py_ssize_t i=0; i < shared->len; i++) { _sharednsitem_clear(&shared->items[i]); } - PyMem_Free(shared->items); - PyMem_Free(shared); + PyMem_RawFree(shared->items); + PyMem_RawFree(shared); } static _sharedns * From e96eefcbcc68bf578edff475f6ee494102ba4b5f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 9 Sep 2023 12:26:23 -0600 Subject: [PATCH 02/11] Drop the unnecessary arg to _release_xid_data(). --- Modules/_xxsubinterpretersmodule.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index 7dbd837398a1c5..85dec17efe8c58 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -52,24 +52,17 @@ add_new_exception(PyObject *mod, const char *name, PyObject *base) add_new_exception(MOD, MODULE_NAME "." Py_STRINGIFY(NAME), BASE) static int -_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) +_release_xid_data(_PyCrossInterpreterData *data) { - PyObject *exc; - if (ignoreexc) { - exc = PyErr_GetRaisedException(); - } + PyObject *exc = PyErr_GetRaisedException(); int res = _PyCrossInterpreterData_Release(data); if (res < 0) { /* The owning interpreter is already destroyed. */ _PyCrossInterpreterData_Clear(NULL, data); - if (ignoreexc) { - // XXX Emit a warning? - PyErr_Clear(); - } - } - if (ignoreexc) { - PyErr_SetRaisedException(exc); + // XXX Emit a warning? + PyErr_Clear(); } + PyErr_SetRaisedException(exc); return res; } @@ -139,7 +132,7 @@ _sharednsitem_clear(struct _sharednsitem *item) PyMem_RawFree((void *)item->name); item->name = NULL; } - (void)_release_xid_data(&item->data, 1); + (void)_release_xid_data(&item->data); } static int From 53e102912576e847e7d1135fee4636fd66b8937c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 13:17:00 -0600 Subject: [PATCH 03/11] Pass a flag to _release_xid_data(). --- Modules/_xxinterpchannelsmodule.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index 1e418414767db8..0c9bd15a8a2ff4 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -160,9 +160,12 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared) return cls; } +#define XID_IGNORE_EXC 1 + static int -_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) +_release_xid_data(_PyCrossInterpreterData *data, int flags) { + int ignoreexc = flags & XID_IGNORE_EXC; PyObject *exc; if (ignoreexc) { exc = PyErr_GetRaisedException(); @@ -366,7 +369,7 @@ static void _channelitem_clear(_channelitem *item) { if (item->data != NULL) { - (void)_release_xid_data(item->data, 1); + (void)_release_xid_data(item->data, XID_IGNORE_EXC); // It was allocated in _channel_send(). GLOBAL_FREE(item->data); item->data = NULL; @@ -1439,7 +1442,7 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res) PyObject *obj = _PyCrossInterpreterData_NewObject(data); if (obj == NULL) { assert(PyErr_Occurred()); - (void)_release_xid_data(data, 1); + (void)_release_xid_data(data, XID_IGNORE_EXC); // It was allocated in _channel_send(). GLOBAL_FREE(data); return -1; From 8ef9b959d8059505b8a9ace8f654580a4ae743a3 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 14:18:42 -0600 Subject: [PATCH 04/11] Add the _Py_pending_call_func typedef. --- Include/internal/pycore_ceval.h | 2 +- Include/internal/pycore_ceval_state.h | 4 +++- Python/ceval_gil.c | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index e9535023cec46b..23d0fa399d7e6f 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -44,7 +44,7 @@ extern void _PyEval_SignalReceived(PyInterpreterState *interp); // Export for '_testinternalcapi' shared extension PyAPI_FUNC(int) _PyEval_AddPendingCall( PyInterpreterState *interp, - int (*func)(void *), + _Py_pending_call_func func, void *arg, int mainthreadonly); diff --git a/Include/internal/pycore_ceval_state.h b/Include/internal/pycore_ceval_state.h index 6e3d669dc646af..d0af5b542233e0 100644 --- a/Include/internal/pycore_ceval_state.h +++ b/Include/internal/pycore_ceval_state.h @@ -11,6 +11,8 @@ extern "C" { #include "pycore_gil.h" // struct _gil_runtime_state +typedef int (*_Py_pending_call_func)(void *); + struct _pending_calls { int busy; PyThread_type_lock lock; @@ -22,7 +24,7 @@ struct _pending_calls { int async_exc; #define NPENDINGCALLS 32 struct _pending_call { - int (*func)(void *); + _Py_pending_call_func func; void *arg; } calls[NPENDINGCALLS]; int first; diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 7c9ad07cc7207b..86fd0816c87bce 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -794,7 +794,7 @@ _PyEval_SignalReceived(PyInterpreterState *interp) /* Push one item onto the queue while holding the lock. */ static int _push_pending_call(struct _pending_calls *pending, - int (*func)(void *), void *arg) + _Py_pending_call_func func, void *arg) { int i = pending->last; int j = (i + 1) % NPENDINGCALLS; @@ -841,7 +841,7 @@ _pop_pending_call(struct _pending_calls *pending, int _PyEval_AddPendingCall(PyInterpreterState *interp, - int (*func)(void *), void *arg, + _Py_pending_call_func func, void *arg, int mainthreadonly) { assert(!mainthreadonly || _Py_IsMainInterpreter(interp)); @@ -865,7 +865,7 @@ _PyEval_AddPendingCall(PyInterpreterState *interp, } int -Py_AddPendingCall(int (*func)(void *), void *arg) +Py_AddPendingCall(_Py_pending_call_func func, void *arg) { /* Legacy users of this API will continue to target the main thread (of the main interpreter). */ @@ -909,7 +909,7 @@ _make_pending_calls(struct _pending_calls *pending) { /* perform a bounded number of calls, in case of recursion */ for (int i=0; i Date: Thu, 7 Sep 2023 12:18:21 -0600 Subject: [PATCH 05/11] Do not bother freeing the data if not set. --- Python/pystate.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 01651d79f9acc0..ce69536cf136a5 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2263,10 +2263,16 @@ _xidata_init(_PyCrossInterpreterData *data) static inline void _xidata_clear(_PyCrossInterpreterData *data) { - if (data->free != NULL) { - data->free(data->data); + // _PyCrossInterpreterData only has two members that need to be + // cleaned up, if set: "data" must be freed and "obj" must be decref'ed. + // In both cases the original (owning) interpreter must be used, + // which is the caller's responsibility to ensure. + if (data->data != NULL) { + if (data->free != NULL) { + data->free(data->data); + } + data->data = NULL; } - data->data = NULL; Py_CLEAR(data->obj); } @@ -2442,7 +2448,7 @@ _call_in_interpreter(PyInterpreterState *interp, releasefunc func, void *arg) int _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) { - if (data->free == NULL && data->obj == NULL) { + if ((data->data == NULL || data->free == NULL) && data->obj == NULL) { // Nothing to release! data->data = NULL; return 0; From 5d9445658c2647f25dfd9acee7170434720b1a55 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 12:50:18 -0600 Subject: [PATCH 06/11] Use pending calls when releasing cross-interpreter data. --- Python/pystate.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index ce69536cf136a5..5d2500b7967c44 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2417,32 +2417,11 @@ _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *data) return data->new_object(data); } -typedef void (*releasefunc)(PyInterpreterState *, void *); - -static void -_call_in_interpreter(PyInterpreterState *interp, releasefunc func, void *arg) +static int +_release_xidata_pending(void *data) { - /* We would use Py_AddPendingCall() if it weren't specific to the - * main interpreter (see bpo-33608). In the meantime we take a - * naive approach. - */ - _PyRuntimeState *runtime = interp->runtime; - PyThreadState *save_tstate = NULL; - if (interp != current_fast_get(runtime)->interp) { - // XXX Using the "head" thread isn't strictly correct. - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - // XXX Possible GILState issues? - save_tstate = _PyThreadState_Swap(runtime, tstate); - } - - // XXX Once the GIL is per-interpreter, this should be called with the - // calling interpreter's GIL released and the target interpreter's held. - func(interp, arg); - - // Switch back. - if (save_tstate != NULL) { - _PyThreadState_Swap(runtime, save_tstate); - } + _xidata_clear((_PyCrossInterpreterData *)data); + return 0; } int @@ -2465,8 +2444,13 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) } // "Release" the data and/or the object. - _call_in_interpreter(interp, - (releasefunc)_PyCrossInterpreterData_Clear, data); + if (interp == current_fast_get(interp->runtime)->interp) { + _xidata_clear(data); + } + else { + // XXX Emit a warning if this fails? + _PyEval_AddPendingCall(interp, _release_xidata_pending, data, 0); + } return 0; } From c610c41cfd9cb8f3439e0aa8779e17c9ea71fa51 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 14:30:57 -0600 Subject: [PATCH 07/11] Use _release_xidata_pending() indirectly. --- Python/pystate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 5d2500b7967c44..d750a6a24425f3 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2448,8 +2448,9 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) _xidata_clear(data); } else { + _Py_pending_call_func func = _release_xidata_pending; // XXX Emit a warning if this fails? - _PyEval_AddPendingCall(interp, _release_xidata_pending, data, 0); + _PyEval_AddPendingCall(interp, func, data, 0); } return 0; } From 1d125033320311fec845bf39fb47878fecc63a30 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 14:43:50 -0600 Subject: [PATCH 08/11] Optionally free the provided XID after releasing. --- Python/pystate.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index d750a6a24425f3..1f785ed57f3128 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2424,12 +2424,25 @@ _release_xidata_pending(void *data) return 0; } -int -_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) +static int +_xidata_release_and_rawfree_pending(void *data) +{ + _xidata_clear((_PyCrossInterpreterData *)data); + PyMem_RawFree(data); + return 0; +} + +static int +_xidata_release(_PyCrossInterpreterData *data, int rawfree) { if ((data->data == NULL || data->free == NULL) && data->obj == NULL) { // Nothing to release! - data->data = NULL; + if (rawfree) { + PyMem_RawFree(data); + } + else { + data->data = NULL; + } return 0; } @@ -2440,21 +2453,36 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) // This function shouldn't have been called. // XXX Someone leaked some memory... assert(PyErr_Occurred()); + if (rawfree) { + PyMem_RawFree(data); + } return -1; } // "Release" the data and/or the object. if (interp == current_fast_get(interp->runtime)->interp) { _xidata_clear(data); + if (rawfree) { + PyMem_RawFree(data); + } } else { _Py_pending_call_func func = _release_xidata_pending; + if (rawfree) { + func = _xidata_release_and_rawfree_pending; + } // XXX Emit a warning if this fails? _PyEval_AddPendingCall(interp, func, data, 0); } return 0; } +int +_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) +{ + return _xidata_release(data, 0); +} + /* registry of {type -> crossinterpdatafunc} */ /* For now we use a global registry of shareable classes. An From 0b5a84aa202a1ebaeeebcbc12e9ba0ad40cd609b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 7 Sep 2023 14:49:58 -0600 Subject: [PATCH 09/11] Add _PyCrossInterpreterData_ReleaseAndRawFree(). --- Include/cpython/pystate.h | 1 + Modules/_xxinterpchannelsmodule.c | 22 +++++++++++++++------- Python/pystate.c | 6 ++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index fc5f58db86dbe8..bb75dd1e849a21 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -310,6 +310,7 @@ PyAPI_FUNC(void) _PyCrossInterpreterData_Clear( PyAPI_FUNC(int) _PyObject_GetCrossInterpreterData(PyObject *, _PyCrossInterpreterData *); PyAPI_FUNC(PyObject *) _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyCrossInterpreterData_Release(_PyCrossInterpreterData *); +PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyObject_CheckCrossInterpreterData(PyObject *); diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index 0c9bd15a8a2ff4..bfc4f3126dda47 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -161,6 +161,7 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared) } #define XID_IGNORE_EXC 1 +#define XID_FREE 2 static int _release_xid_data(_PyCrossInterpreterData *data, int flags) @@ -170,7 +171,13 @@ _release_xid_data(_PyCrossInterpreterData *data, int flags) if (ignoreexc) { exc = PyErr_GetRaisedException(); } - int res = _PyCrossInterpreterData_Release(data); + int res; + if (flags & XID_FREE) { + res = _PyCrossInterpreterData_Release(data); + } + else { + res = _PyCrossInterpreterData_ReleaseAndRawFree(data); + } if (res < 0) { /* The owning interpreter is already destroyed. */ if (ignoreexc) { @@ -178,6 +185,9 @@ _release_xid_data(_PyCrossInterpreterData *data, int flags) PyErr_Clear(); } } + if (flags & XID_FREE) { + /* Either way, we free the data. */ + } if (ignoreexc) { PyErr_SetRaisedException(exc); } @@ -1442,14 +1452,12 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res) PyObject *obj = _PyCrossInterpreterData_NewObject(data); if (obj == NULL) { assert(PyErr_Occurred()); - (void)_release_xid_data(data, XID_IGNORE_EXC); - // It was allocated in _channel_send(). - GLOBAL_FREE(data); + // It was allocated in _channel_send(), so we free it. + (void)_release_xid_data(data, XID_IGNORE_EXC | XID_FREE); return -1; } - int release_res = _release_xid_data(data, 0); - // It was allocated in _channel_send(). - GLOBAL_FREE(data); + // It was allocated in _channel_send(), so we free it. + int release_res = _release_xid_data(data, XID_FREE); if (release_res < 0) { // The source interpreter has been destroyed already. assert(PyErr_Occurred()); diff --git a/Python/pystate.c b/Python/pystate.c index 1f785ed57f3128..daab6454e9d908 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2483,6 +2483,12 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) return _xidata_release(data, 0); } +int +_PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data) +{ + return _xidata_release(data, 1); +} + /* registry of {type -> crossinterpdatafunc} */ /* For now we use a global registry of shareable classes. An From ac9d0dbd0d0b9a0fefefae9a1f3da392ffa695ae Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 19 Sep 2023 11:09:55 -0600 Subject: [PATCH 10/11] Fix _release_xid_data(). --- Modules/_xxinterpchannelsmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index bfc4f3126dda47..ea7d7645ddc9b8 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -173,10 +173,10 @@ _release_xid_data(_PyCrossInterpreterData *data, int flags) } int res; if (flags & XID_FREE) { - res = _PyCrossInterpreterData_Release(data); + res = _PyCrossInterpreterData_ReleaseAndRawFree(data); } else { - res = _PyCrossInterpreterData_ReleaseAndRawFree(data); + res = _PyCrossInterpreterData_Release(data); } if (res < 0) { /* The owning interpreter is already destroyed. */ From f38f44b314afd0723b522fb1c66e83a9d56aaad5 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 19 Sep 2023 11:10:46 -0600 Subject: [PATCH 11/11] Do not directly free the XI data in _channelitem_clear(). --- Modules/_xxinterpchannelsmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index ea7d7645ddc9b8..ab87738be50d1a 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -379,9 +379,8 @@ static void _channelitem_clear(_channelitem *item) { if (item->data != NULL) { - (void)_release_xid_data(item->data, XID_IGNORE_EXC); // It was allocated in _channel_send(). - GLOBAL_FREE(item->data); + (void)_release_xid_data(item->data, XID_IGNORE_EXC & XID_FREE); item->data = NULL; } item->next = NULL;