Description
Bug report
Bug description:
First of all, I'm not a threading expert and my understanding of the memory-ordering model may be wrong. So, if I'm wrong I will be happy to fix my knowledge lacoons.
I saw some inconsistency (from my sight) of loading and writing of managed dict pointer.
Non-atomic loads:
PyObject_VisitManagedDict
Line 7202 in 9ad0c7b
PyObject_ClearManagedDict
Line 7462 in 9ad0c7b
_PyObject_GetDictPtr
Line 1541 in 9ad0c7b
Non-atomic stores:
_PyObject_InitInlineValues
Lines 6787 to 6791 in 9ad0c7b
IIUC mixing of non-atomic loads/stores with atomic ones may lead to data races.
memory_order_acquire
loads:
_PyObject_GetManagedDict
cpython/Include/internal/pycore_object.h
Lines 932 to 936 in 9ad0c7b
memory_order_release
stores:
_PyObject_MaterializeManagedDict_LockHeld
Lines 6827 to 6829 in 9ad0c7b
store_instance_attr_lock_held
Lines 6925 to 6927 in 9ad0c7b
ensure_managed_dict
Lines 7494 to 7495 in 9ad0c7b
memory_order_seq_cst
stores:
set_dict_inline_values
Line 7225 in 9ad0c7b
try_set_dict_inline_only_or_other_dict
Lines 7252 to 7253 in 9ad0c7b
replace_dict_probably_inline_materialized
Lines 7287 to 7289 in 9ad0c7b
IIUC mixing acquire/release with seq_cst may break total order of seq_cst operations.
Mixing with memory_order_seq_cst
stores
_PyObject_SetManagedDict
Lines 7356 to 7365 in 9ad0c7b
_PyObject_SetManagedDict
uses non-atomic load but stores with seq_cst
mode so it is OK (IIUC), but following store without fence may lead to data race.
Are my findings valid or am I completely wrong?
CPython versions tested on:
CPython main branch
Operating systems tested on:
No response
Activity
colesbury commentedon May 14, 2025
I think these are fine:
1: GC traverse is called during stop the world (no other threads are running) so it doesn't need atomics
2: GC clear is called when the object is not accessible so it doesn't need atomics (same logic applies to
tp_dealloc
)3: This constructs a pointer to the dictionary; it doesn't load the dictionary
_PyObject_InitInlineValues: object initialization doesn't need atomics (no other threads have access to those fields)
The
memory_order_seq_cst
stores can bememory_order_release
, but it doesn't matter (except possibly for perf).memory_order_seq_cst
is strictly stronger thanmemory_order_seq_cst
. We don't care about a total order formemory_order_seq_cst
across stores to other memory locations here._PyObject_SetManagedDict: this is code in an
#ifdef
that only runs in the default GIL-enabled build (not the free threaded build)sergey-miryanov commentedon May 14, 2025
Thanks for explanation!
About
_PyObject_GetDictPtr
it is called fromPyObject_GenericSetDict
where store to ptr is made. It is guarded by CS, but I believe it should use atomic store too. WDYT?cpython/Objects/object.c
Lines 1933 to 1935 in 9ad0c7b
PyObject_GenericSetDict
#133988kumaraditya303 commentedon May 14, 2025
Yes, it should use atomic store with release memory order, created #133988 to fix it.
Reproducer:
sergey-miryanov commentedon May 14, 2025
@kumaraditya303 Thanks!
gh-133980: use atomic store in `PyObject_GenericSetDict` (#133988)
pythongh-133980: use atomic store in `PyObject_GenericSetDict` (pytho…
PyObject_GenericSetDict
(GH-133988) #134354[3.14] gh-133980: use atomic store in `PyObject_GenericSetDict` (GH-1…
1 remaining item