From 752139564607a316f7dc82454cce2d190be934d2 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 16 Feb 2024 17:26:48 +0900 Subject: [PATCH 1/6] gh-112087: Make list_concat to be thread-safe --- .../internal/pycore_pyatomic_ft_wrappers.h | 3 + Objects/listobject.c | 60 ++++++++++--------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index cbbe90e009c8d2..6286b090ec9039 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -23,11 +23,14 @@ extern "C" { #define FT_ATOMIC_LOAD_SSIZE(value) _Py_atomic_load_ssize(&value) #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) \ _Py_atomic_load_ssize_relaxed(&value) +#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \ + _Py_atomic_store_ptr_relaxed(&value, new_value) #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \ _Py_atomic_store_ssize_relaxed(&value, new_value) #else #define FT_ATOMIC_LOAD_SSIZE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value +#define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value #endif diff --git a/Objects/listobject.c b/Objects/listobject.c index eb466260318ec1..3ae6a0f9c781a3 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3,6 +3,7 @@ #include "Python.h" #include "pycore_abstract.h" // _PyIndex_Check() #include "pycore_ceval.h" // _PyEval_GetBuiltin() +#include "pycore_pyatomic_ft_wrappers.h" #include "pycore_interp.h" // PyInterpreterState.list #include "pycore_list.h" // struct _Py_list_freelist, _PyListIterObject #include "pycore_long.h" // _PyLong_DigitCount @@ -20,14 +21,6 @@ class list "PyListObject *" "&PyList_Type" _Py_DECLARE_STR(list_err, "list index out of range"); -#ifdef Py_GIL_DISABLED -# define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value) -# define STORE_SSIZE(value, new_value) _Py_atomic_store_ssize_relaxed(&value, new_value) -#else -# define LOAD_SSIZE(value) value -# define STORE_SSIZE(value, new_value) value = new_value -#endif - #ifdef WITH_FREELISTS static struct _Py_list_freelist * get_list_freelist(void) @@ -563,20 +556,12 @@ PyList_GetSlice(PyObject *a, Py_ssize_t ilow, Py_ssize_t ihigh) } static PyObject * -list_concat(PyObject *aa, PyObject *bb) +list_concat_locked(PyListObject *a, PyListObject *b) { - PyListObject *a = (PyListObject *)aa; Py_ssize_t size; Py_ssize_t i; PyObject **src, **dest; PyListObject *np; - if (!PyList_Check(bb)) { - PyErr_Format(PyExc_TypeError, - "can only concatenate list (not \"%.200s\") to list", - Py_TYPE(bb)->tp_name); - return NULL; - } -#define b ((PyListObject *)bb) assert((size_t)Py_SIZE(a) + (size_t)Py_SIZE(b) < PY_SSIZE_T_MAX); size = Py_SIZE(a) + Py_SIZE(b); if (size == 0) { @@ -590,17 +575,34 @@ list_concat(PyObject *aa, PyObject *bb) dest = np->ob_item; for (i = 0; i < Py_SIZE(a); i++) { PyObject *v = src[i]; - dest[i] = Py_NewRef(v); + FT_ATOMIC_STORE_PTR_RELAXED(dest[i], Py_NewRef(v)); } src = b->ob_item; dest = np->ob_item + Py_SIZE(a); for (i = 0; i < Py_SIZE(b); i++) { PyObject *v = src[i]; - dest[i] = Py_NewRef(v); + FT_ATOMIC_STORE_PTR_RELAXED(dest[i], Py_NewRef(v)); } Py_SET_SIZE(np, size); return (PyObject *)np; -#undef b +} + +static PyObject * +list_concat(PyObject *aa, PyObject *bb) +{ + if (!PyList_Check(bb)) { + PyErr_Format(PyExc_TypeError, + "can only concatenate list (not \"%.200s\") to list", + Py_TYPE(bb)->tp_name); + return NULL; + } + PyListObject *a = (PyListObject *)aa; + PyListObject *b = (PyListObject *)bb; + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION2(a, b); + ret = list_concat_locked(a, b); + Py_END_CRITICAL_SECTION2(); + return ret; } static PyObject * @@ -2979,7 +2981,7 @@ list___sizeof___impl(PyListObject *self) /*[clinic end generated code: output=3417541f95f9a53e input=b8030a5d5ce8a187]*/ { size_t res = _PyObject_SIZE(Py_TYPE(self)); - Py_ssize_t allocated = LOAD_SSIZE(self->allocated); + Py_ssize_t allocated = FT_ATOMIC_LOAD_SSIZE_RELAXED(self->allocated); res += (size_t)allocated * sizeof(void*); return PyLong_FromSize_t(res); } @@ -3382,7 +3384,7 @@ static PyObject * listiter_next(PyObject *self) { _PyListIterObject *it = (_PyListIterObject *)self; - Py_ssize_t index = LOAD_SSIZE(it->it_index); + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); if (index < 0) { return NULL; } @@ -3390,7 +3392,7 @@ listiter_next(PyObject *self) PyObject *item = list_get_item_ref(it->it_seq, index); if (item == NULL) { // out-of-bounds - STORE_SSIZE(it->it_index, -1); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, -1); #ifndef Py_GIL_DISABLED PyListObject *seq = it->it_seq; it->it_seq = NULL; @@ -3398,7 +3400,7 @@ listiter_next(PyObject *self) #endif return NULL; } - STORE_SSIZE(it->it_index, index + 1); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1); return item; } @@ -3407,7 +3409,7 @@ listiter_len(PyObject *self, PyObject *Py_UNUSED(ignored)) { assert(self != NULL); _PyListIterObject *it = (_PyListIterObject *)self; - Py_ssize_t index = LOAD_SSIZE(it->it_index); + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); if (index >= 0) { Py_ssize_t len = PyList_GET_SIZE(it->it_seq) - index; if (len >= 0) @@ -3540,16 +3542,16 @@ listreviter_next(PyObject *self) PyListObject *seq = it->it_seq; assert(PyList_Check(seq)); - Py_ssize_t index = LOAD_SSIZE(it->it_index); + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); if (index < 0) { return NULL; } PyObject *item = list_get_item_ref(seq, index); if (item != NULL) { - STORE_SSIZE(it->it_index, index - 1); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index - 1); return item; } - STORE_SSIZE(it->it_index, -1); + FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, -1); #ifndef Py_GIL_DISABLED it->it_seq = NULL; Py_DECREF(seq); @@ -3561,7 +3563,7 @@ static PyObject * listreviter_len(PyObject *self, PyObject *Py_UNUSED(ignored)) { listreviterobject *it = (listreviterobject *)self; - Py_ssize_t index = LOAD_SSIZE(it->it_index); + Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index); Py_ssize_t len = index + 1; if (it->it_seq == NULL || PyList_GET_SIZE(it->it_seq) < len) len = 0; From 4bbd4fec015b0a36c68c6bfffacca91b217c938d Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 16 Feb 2024 17:44:01 +0900 Subject: [PATCH 2/6] Make list_repeat/clear to be thread-safe --- .../internal/pycore_pyatomic_ft_wrappers.h | 3 +++ Objects/listobject.c | 21 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index 6286b090ec9039..e441600d54e1aa 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -25,12 +25,15 @@ extern "C" { _Py_atomic_load_ssize_relaxed(&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_SSIZE_RELAXED(value, new_value) \ _Py_atomic_store_ssize_relaxed(&value, new_value) #else #define FT_ATOMIC_LOAD_SSIZE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(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_SSIZE_RELAXED(value, new_value) value = new_value #endif diff --git a/Objects/listobject.c b/Objects/listobject.c index 3ae6a0f9c781a3..95bb4eadb7e9b3 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -606,9 +606,8 @@ list_concat(PyObject *aa, PyObject *bb) } static PyObject * -list_repeat(PyObject *aa, Py_ssize_t n) +list_repeat_locked(PyListObject *a, Py_ssize_t n) { - PyListObject *a = (PyListObject *)aa; const Py_ssize_t input_size = Py_SIZE(a); if (input_size == 0 || n <= 0) return PyList_New(0); @@ -628,7 +627,7 @@ list_repeat(PyObject *aa, Py_ssize_t n) _Py_RefcntAdd(elem, n); PyObject **dest_end = dest + output_size; while (dest < dest_end) { - *dest++ = elem; + FT_ATOMIC_STORE_PTR_RELAXED(*dest++, elem); } } else { @@ -636,7 +635,7 @@ list_repeat(PyObject *aa, Py_ssize_t n) PyObject **src_end = src + input_size; while (src < src_end) { _Py_RefcntAdd(*src, n); - *dest++ = *src++; + FT_ATOMIC_STORE_PTR_RELAXED(*dest++, *src++); } _Py_memory_repeat((char *)np->ob_item, sizeof(PyObject *)*output_size, @@ -647,6 +646,17 @@ list_repeat(PyObject *aa, Py_ssize_t n) return (PyObject *) np; } +static PyObject * +list_repeat(PyObject *aa, Py_ssize_t n) +{ + PyObject *ret; + PyListObject *a = (PyListObject *)aa; + Py_BEGIN_CRITICAL_SECTION(a); + ret = list_repeat_locked(a, n); + Py_END_CRITICAL_SECTION(); + return ret; +} + static void list_clear(PyListObject *a) { @@ -659,11 +669,12 @@ list_clear(PyListObject *a) this list, we make it empty first. */ Py_ssize_t i = Py_SIZE(a); Py_SET_SIZE(a, 0); - a->ob_item = NULL; + FT_ATOMIC_STORE_PTR_RELEASE(a->ob_item, NULL); a->allocated = 0; while (--i >= 0) { Py_XDECREF(items[i]); } + // TODO: Use QSBR to free the memory in the background PyMem_Free(items); // Note that there is no guarantee that the list is actually empty From dc4e52fbf310df2f087f826ac7d484eac3735f5a Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 16 Feb 2024 17:53:40 +0900 Subject: [PATCH 3/6] Make list_inplace_repeat to be thread-safe --- Objects/listobject.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 95bb4eadb7e9b3..9ef93b1d586356 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -811,9 +811,8 @@ PyList_SetSlice(PyObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyObject *v) } static PyObject * -list_inplace_repeat(PyObject *_self, Py_ssize_t n) +list_inplace_repeat_locked(PyListObject *self, Py_ssize_t n) { - PyListObject *self = (PyListObject *)_self; Py_ssize_t input_size = PyList_GET_SIZE(self); if (input_size == 0 || n == 1) { return Py_NewRef(self); @@ -842,6 +841,17 @@ list_inplace_repeat(PyObject *_self, Py_ssize_t n) return Py_NewRef(self); } +static PyObject * +list_inplace_repeat(PyObject *_self, Py_ssize_t n) +{ + PyObject *ret; + PyListObject *self = (PyListObject *) _self; + Py_BEGIN_CRITICAL_SECTION(self); + ret = list_inplace_repeat_locked(self, n); + Py_END_CRITICAL_SECTION(); + return ret; +} + static int list_ass_item(PyObject *aa, Py_ssize_t i, PyObject *v) { From b1293edc8ed18893734440677e84ca8f5530b95c Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 17 Feb 2024 21:50:59 +0900 Subject: [PATCH 4/6] Make list_ass_item to be thread-safe --- Objects/listobject.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 9ef93b1d586356..f299e2738074b1 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -853,20 +853,39 @@ list_inplace_repeat(PyObject *_self, Py_ssize_t n) } static int -list_ass_item(PyObject *aa, Py_ssize_t i, PyObject *v) +list_ass_item_locked(PyListObject *a, Py_ssize_t i, PyObject *v) { - PyListObject *a = (PyListObject *)aa; if (!valid_index(i, Py_SIZE(a))) { PyErr_SetString(PyExc_IndexError, "list assignment index out of range"); return -1; } - if (v == NULL) - return list_ass_slice(a, i, i+1, v); - Py_SETREF(a->ob_item[i], Py_NewRef(v)); + PyObject *tmp = a->ob_item[i]; + if (v == NULL) { + Py_ssize_t size = Py_SIZE(a); + for (Py_ssize_t idx = i; idx < size - 1; idx++) { + FT_ATOMIC_STORE_PTR_RELAXED(a->ob_item[idx], a->ob_item[idx + 1]); + } + Py_SET_SIZE(a, size - 1); + } + else { + FT_ATOMIC_STORE_PTR_RELEASE(a->ob_item[i], Py_NewRef(v)); + } + Py_DECREF(tmp); return 0; } +static int +list_ass_item(PyObject *aa, Py_ssize_t i, PyObject *v) +{ + int ret; + PyListObject *a = (PyListObject *)aa; + Py_BEGIN_CRITICAL_SECTION(a); + ret = list_ass_item_locked(a, i, v); + Py_END_CRITICAL_SECTION(); + return ret; +} + /*[clinic input] @critical_section list.insert From 6b8eaa2824f571337d0875e1df6189e85c9ce010 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 17 Feb 2024 22:14:18 +0900 Subject: [PATCH 5/6] nit --- Objects/listobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index f299e2738074b1..24333c4e252969 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -674,7 +674,7 @@ list_clear(PyListObject *a) while (--i >= 0) { Py_XDECREF(items[i]); } - // TODO: Use QSBR to free the memory in the background + // TODO: Use QSBR technique, if the list is shared between threads, PyMem_Free(items); // Note that there is no guarantee that the list is actually empty From a7441468ca72105c3355c4260e2c8aec0cca5435 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 21 Feb 2024 10:03:17 +0900 Subject: [PATCH 6/6] Follow _lock_held suffix convention --- Objects/listobject.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 24333c4e252969..58e6e31f1cf355 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -556,7 +556,7 @@ PyList_GetSlice(PyObject *a, Py_ssize_t ilow, Py_ssize_t ihigh) } static PyObject * -list_concat_locked(PyListObject *a, PyListObject *b) +list_concat_lock_held(PyListObject *a, PyListObject *b) { Py_ssize_t size; Py_ssize_t i; @@ -600,13 +600,13 @@ list_concat(PyObject *aa, PyObject *bb) PyListObject *b = (PyListObject *)bb; PyObject *ret; Py_BEGIN_CRITICAL_SECTION2(a, b); - ret = list_concat_locked(a, b); + ret = list_concat_lock_held(a, b); Py_END_CRITICAL_SECTION2(); return ret; } static PyObject * -list_repeat_locked(PyListObject *a, Py_ssize_t n) +list_repeat_lock_held(PyListObject *a, Py_ssize_t n) { const Py_ssize_t input_size = Py_SIZE(a); if (input_size == 0 || n <= 0) @@ -652,7 +652,7 @@ list_repeat(PyObject *aa, Py_ssize_t n) PyObject *ret; PyListObject *a = (PyListObject *)aa; Py_BEGIN_CRITICAL_SECTION(a); - ret = list_repeat_locked(a, n); + ret = list_repeat_lock_held(a, n); Py_END_CRITICAL_SECTION(); return ret; } @@ -811,7 +811,7 @@ PyList_SetSlice(PyObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyObject *v) } static PyObject * -list_inplace_repeat_locked(PyListObject *self, Py_ssize_t n) +list_inplace_repeat_lock_held(PyListObject *self, Py_ssize_t n) { Py_ssize_t input_size = PyList_GET_SIZE(self); if (input_size == 0 || n == 1) { @@ -847,13 +847,13 @@ list_inplace_repeat(PyObject *_self, Py_ssize_t n) PyObject *ret; PyListObject *self = (PyListObject *) _self; Py_BEGIN_CRITICAL_SECTION(self); - ret = list_inplace_repeat_locked(self, n); + ret = list_inplace_repeat_lock_held(self, n); Py_END_CRITICAL_SECTION(); return ret; } static int -list_ass_item_locked(PyListObject *a, Py_ssize_t i, PyObject *v) +list_ass_item_lock_held(PyListObject *a, Py_ssize_t i, PyObject *v) { if (!valid_index(i, Py_SIZE(a))) { PyErr_SetString(PyExc_IndexError, @@ -881,7 +881,7 @@ list_ass_item(PyObject *aa, Py_ssize_t i, PyObject *v) int ret; PyListObject *a = (PyListObject *)aa; Py_BEGIN_CRITICAL_SECTION(a); - ret = list_ass_item_locked(a, i, v); + ret = list_ass_item_lock_held(a, i, v); Py_END_CRITICAL_SECTION(); return ret; }