From 84305083c7f69f46013c729faf69588469935162 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 00:09:29 +0900 Subject: [PATCH 1/8] gh-120837: Update _Py_DumpExtensionModules to be async-signal-safe --- Python/pylifecycle.c | 116 ++++++++++++++++++++++++++++--------------- 1 file changed, 76 insertions(+), 40 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index cae1f8b61c8182..fa9a7373db357e 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2897,6 +2897,30 @@ fatal_error_exit(int status) } } +static inline int +acquire_dict_lock_for_dump(PyObject *obj) { +#ifdef Py_GIL_DISABLED + PyMutex *mutex = &obj->ob_mutex; + if (_PyMutex_LockTimed(mutex, 0, 0) == PY_LOCK_ACQUIRED) { + return 0; + } + return -1; +#else + return 0; +#endif +} + +static inline void +release_dict_lock_for_dump(PyObject *obj) { +#ifdef Py_GIL_DISABLED + PyMutex *mutex = &obj->ob_mutex; + uint8_t expected = _Py_UNLOCKED; + // Do not wake up other threads. + _Py_atomic_compare_exchange_uint8(&mutex->_bits, &expected, _Py_UNLOCKED); +#else + return; +#endif +} // Dump the list of extension modules of sys.modules, excluding stdlib modules // (sys.stdlib_module_names), into fd file descriptor. @@ -2924,12 +2948,18 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) PyObject *stdlib_module_names = NULL; if (interp->sysdict != NULL) { pos = 0; - while (PyDict_Next(interp->sysdict, &pos, &key, &value)) { - if (PyUnicode_Check(key) - && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { - stdlib_module_names = value; - break; + if (acquire_dict_lock_for_dump(interp->sysdict) == 0) { + while (_PyDict_Next(interp->sysdict, &pos, &key, &value, NULL)) { + if (PyUnicode_Check(key) + && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { + stdlib_module_names = value; + break; + } } + release_dict_lock_for_dump(interp->sysdict); + } + else { + return; } } // If we failed to get sys.stdlib_module_names or it's not a frozenset, @@ -2942,46 +2972,52 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) int header = 1; Py_ssize_t count = 0; pos = 0; - while (PyDict_Next(modules, &pos, &key, &value)) { - if (!PyUnicode_Check(key)) { - continue; - } - if (!_PyModule_IsExtension(value)) { - continue; - } - // Use the module name from the sys.modules key, - // don't attempt to get the module object name. - if (stdlib_module_names != NULL) { - int is_stdlib_ext = 0; - - Py_ssize_t i = 0; - PyObject *item; - Py_hash_t hash; - // if stdlib_module_names is not NULL, it is always a frozenset. - while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { - if (PyUnicode_Check(item) - && PyUnicode_Compare(key, item) == 0) - { - is_stdlib_ext = 1; - break; - } + if (acquire_dict_lock_for_dump(modules) == 0) { + while (_PyDict_Next(modules, &pos, &key, &value, NULL)) { + if (!PyUnicode_Check(key)) { + continue; } - if (is_stdlib_ext) { - // Ignore stdlib extension + if (!_PyModule_IsExtension(value)) { continue; } - } + // Use the module name from the sys.modules key, + // don't attempt to get the module object name. + if (stdlib_module_names != NULL) { + int is_stdlib_ext = 0; + + Py_ssize_t i = 0; + PyObject *item; + Py_hash_t hash; + // if stdlib_module_names is not NULL, it is always a frozenset. + while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { + if (PyUnicode_Check(item) + && PyUnicode_Compare(key, item) == 0) + { + is_stdlib_ext = 1; + break; + } + } + if (is_stdlib_ext) { + // Ignore stdlib extension + continue; + } + } - if (header) { - PUTS(fd, "\nExtension modules: "); - header = 0; - } - else { - PUTS(fd, ", "); - } + if (header) { + PUTS(fd, "\nExtension modules: "); + header = 0; + } + else { + PUTS(fd, ", "); + } - _Py_DumpASCII(fd, key); - count++; + _Py_DumpASCII(fd, key); + count++; + } + release_dict_lock_for_dump(modules); + } + else { + return; } if (count) { From 07962e5e0b4eb82eba0920f1ba78c906ccf70988 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 00:13:14 +0900 Subject: [PATCH 2/8] nit --- Python/pylifecycle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index fa9a7373db357e..e195b8b9a40c32 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2951,7 +2951,7 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) if (acquire_dict_lock_for_dump(interp->sysdict) == 0) { while (_PyDict_Next(interp->sysdict, &pos, &key, &value, NULL)) { if (PyUnicode_Check(key) - && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { + && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { stdlib_module_names = value; break; } From 1f56fcc54f9f4029e85881a4661202927388b55b Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 00:26:37 +0900 Subject: [PATCH 3/8] fix --- Python/pylifecycle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index e195b8b9a40c32..cb81ab790c66a5 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2914,7 +2914,7 @@ static inline void release_dict_lock_for_dump(PyObject *obj) { #ifdef Py_GIL_DISABLED PyMutex *mutex = &obj->ob_mutex; - uint8_t expected = _Py_UNLOCKED; + uint8_t expected = _Py_LOCKED; // Do not wake up other threads. _Py_atomic_compare_exchange_uint8(&mutex->_bits, &expected, _Py_UNLOCKED); #else From e7f24af4c642a1ee49dd8f9ada6f16fb6818b0b1 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 20:10:03 +0900 Subject: [PATCH 4/8] Address code review --- Python/pylifecycle.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index cb81ab790c66a5..a989e5b542fc5d 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2915,7 +2915,8 @@ release_dict_lock_for_dump(PyObject *obj) { #ifdef Py_GIL_DISABLED PyMutex *mutex = &obj->ob_mutex; uint8_t expected = _Py_LOCKED; - // Do not wake up other threads. + // We can not call PyMutex_Unlock because it's not async-signal-safe. + // So not to wake up other threads, we just use a simple CAS in here. _Py_atomic_compare_exchange_uint8(&mutex->_bits, &expected, _Py_UNLOCKED); #else return; From ed9bb3b11c76c9c0eea89596c63f179076977dcf Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 20:32:45 +0900 Subject: [PATCH 5/8] Address code review --- Python/pylifecycle.c | 102 +++++++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 52 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index a989e5b542fc5d..b71ab931352012 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2949,19 +2949,18 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) PyObject *stdlib_module_names = NULL; if (interp->sysdict != NULL) { pos = 0; - if (acquire_dict_lock_for_dump(interp->sysdict) == 0) { - while (_PyDict_Next(interp->sysdict, &pos, &key, &value, NULL)) { - if (PyUnicode_Check(key) - && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { - stdlib_module_names = value; - break; - } - } - release_dict_lock_for_dump(interp->sysdict); - } - else { + if (acquire_dict_lock_for_dump(interp->sysdict) < 0) { + // If we cannot acquire the lock, just don't dump the list of extension modules. return; } + while (_PyDict_Next(interp->sysdict, &pos, &key, &value, NULL)) { + if (PyUnicode_Check(key) + && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { + stdlib_module_names = value; + break; + } + } + release_dict_lock_for_dump(interp->sysdict); } // If we failed to get sys.stdlib_module_names or it's not a frozenset, // don't exclude stdlib modules. @@ -2973,53 +2972,52 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) int header = 1; Py_ssize_t count = 0; pos = 0; - if (acquire_dict_lock_for_dump(modules) == 0) { - while (_PyDict_Next(modules, &pos, &key, &value, NULL)) { - if (!PyUnicode_Check(key)) { - continue; - } - if (!_PyModule_IsExtension(value)) { - continue; - } - // Use the module name from the sys.modules key, - // don't attempt to get the module object name. - if (stdlib_module_names != NULL) { - int is_stdlib_ext = 0; - - Py_ssize_t i = 0; - PyObject *item; - Py_hash_t hash; - // if stdlib_module_names is not NULL, it is always a frozenset. - while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { - if (PyUnicode_Check(item) - && PyUnicode_Compare(key, item) == 0) - { - is_stdlib_ext = 1; - break; - } - } - if (is_stdlib_ext) { - // Ignore stdlib extension - continue; + if (acquire_dict_lock_for_dump(modules) < 0) { + // If we cannot acquire the lock, just don't dump the list of extension modules. + return; + } + while (_PyDict_Next(modules, &pos, &key, &value, NULL)) { + if (!PyUnicode_Check(key)) { + continue; + } + if (!_PyModule_IsExtension(value)) { + continue; + } + // Use the module name from the sys.modules key, + // don't attempt to get the module object name. + if (stdlib_module_names != NULL) { + int is_stdlib_ext = 0; + + Py_ssize_t i = 0; + PyObject *item; + Py_hash_t hash; + // if stdlib_module_names is not NULL, it is always a frozenset. + while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { + if (PyUnicode_Check(item) + && PyUnicode_Compare(key, item) == 0) + { + is_stdlib_ext = 1; + break; } } - - if (header) { - PUTS(fd, "\nExtension modules: "); - header = 0; - } - else { - PUTS(fd, ", "); + if (is_stdlib_ext) { + // Ignore stdlib extension + continue; } + } - _Py_DumpASCII(fd, key); - count++; + if (header) { + PUTS(fd, "\nExtension modules: "); + header = 0; } - release_dict_lock_for_dump(modules); - } - else { - return; + else { + PUTS(fd, ", "); + } + + _Py_DumpASCII(fd, key); + count++; } + release_dict_lock_for_dump(modules); if (count) { PUTS(fd, " (total: "); From 136e8d13fdf49a503233576587a8deaf41da8907 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 27 Jun 2024 20:33:39 +0900 Subject: [PATCH 6/8] nit --- Python/pylifecycle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index b71ab931352012..0abfdacd4c72ae 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2955,7 +2955,7 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) } while (_PyDict_Next(interp->sysdict, &pos, &key, &value, NULL)) { if (PyUnicode_Check(key) - && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { + && PyUnicode_CompareWithASCIIString(key, "stdlib_module_names") == 0) { stdlib_module_names = value; break; } From 61de424504a6b44e2901468df3852c244de35a06 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 28 Jun 2024 05:48:45 +0900 Subject: [PATCH 7/8] Address code review --- Python/pylifecycle.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 0abfdacd4c72ae..db5b2e6df34471 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2898,26 +2898,27 @@ fatal_error_exit(int status) } static inline int -acquire_dict_lock_for_dump(PyObject *obj) { +acquire_dict_lock_for_dump(PyObject *obj) +{ #ifdef Py_GIL_DISABLED PyMutex *mutex = &obj->ob_mutex; if (_PyMutex_LockTimed(mutex, 0, 0) == PY_LOCK_ACQUIRED) { - return 0; + return 1; } - return -1; -#else return 0; +#else + return 1; #endif } static inline void -release_dict_lock_for_dump(PyObject *obj) { +release_dict_lock_for_dump(PyObject *obj) +{ #ifdef Py_GIL_DISABLED PyMutex *mutex = &obj->ob_mutex; - uint8_t expected = _Py_LOCKED; // We can not call PyMutex_Unlock because it's not async-signal-safe. - // So not to wake up other threads, we just use a simple CAS in here. - _Py_atomic_compare_exchange_uint8(&mutex->_bits, &expected, _Py_UNLOCKED); + // So not to wake up other threads, we just use a simple atomic store in here. + _Py_atomic_store_uint8(&mutex->_bits, _Py_UNLOCKED); #else return; #endif @@ -2949,7 +2950,7 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) PyObject *stdlib_module_names = NULL; if (interp->sysdict != NULL) { pos = 0; - if (acquire_dict_lock_for_dump(interp->sysdict) < 0) { + if (!acquire_dict_lock_for_dump(interp->sysdict)) { // If we cannot acquire the lock, just don't dump the list of extension modules. return; } @@ -2972,7 +2973,7 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) int header = 1; Py_ssize_t count = 0; pos = 0; - if (acquire_dict_lock_for_dump(modules) < 0) { + if (!acquire_dict_lock_for_dump(modules)) { // If we cannot acquire the lock, just don't dump the list of extension modules. return; } From 2d3178089290a75f0cdad5813f0e90f06b784763 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 28 Jun 2024 06:20:24 +0900 Subject: [PATCH 8/8] Address code review --- Python/pylifecycle.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index db5b2e6df34471..31144f4027140e 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2919,8 +2919,6 @@ release_dict_lock_for_dump(PyObject *obj) // We can not call PyMutex_Unlock because it's not async-signal-safe. // So not to wake up other threads, we just use a simple atomic store in here. _Py_atomic_store_uint8(&mutex->_bits, _Py_UNLOCKED); -#else - return; #endif }