gh-127266: avoid data races when updating type slots v2 #133177
Conversation
In the free-threaded build, avoid data races caused by updating type slots or type flags after the type was initially created. For those (typically rare) cases, use the stop-the-world mechanism. Remove the use of atomics when reading or writing type flags. The use of atomics is not sufficient to avoid races (since flags are sometimes read without a lock and without atomics) and are no longer required.
To avoid deadlocks while the world is stopped, we need to avoid calling APIs like _PyObject_HashFast() and _PyDict_GetItemRef_KnownHash(). Collect the slot updates to be done and then apply them all at once. This reduces the amount of code running in the stop-the-world condition.
|
🤖 New build scheduled with the buildbot fleet by @nascheme for commit d511ca6 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133177%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Now that stop-the-world is used to do the slot update, these tests are a lot slower in the free-threaded build. Test with fewer items, which will still hopefully be enough to find bugs in the specializer.
The clearing of Py_TPFLAGS_HAVE_VECTORCALL must be done when the world is stopped too.
|
🤖 New build scheduled with the buildbot fleet by @nascheme for commit 3cb2256 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133177%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Since we stack allocate one chunk, we need to check 'n' to see if there are actually any updates to make. It's pretty common that no updates are actually needed.
| @@ -576,6 +576,7 @@ class TestRacesDoNotCrash(TestBase): | |||
| # Careful with these. Bigger numbers have a higher chance of catching bugs, | |||
| # but you can also burn through a *ton* of type/dict/function versions: | |||
| ITEMS = 1000 | |||
| SMALL_ITEMS = 100 | |||
There was a problem hiding this comment.
It might be worth investigating this further. I'm surprised there's a large slowdown in this PR, but not in the earlier version. What's the relevant difference?
| race:_Py_slot_tp_getattr_hook |
There was a problem hiding this comment.
Hmm.. seems like we should fix this in a follow up PR
| PyObject *old_bases = lookup_tp_bases(type); | ||
| assert(old_bases != NULL); | ||
| PyTypeObject *old_base = type->tp_base; | ||
|
|
||
| set_tp_bases(type, Py_NewRef(new_bases), 0); | ||
| type->tp_base = (PyTypeObject *)Py_NewRef(new_base); | ||
| type->tp_base = (PyTypeObject *)Py_NewRef(best_base); |
There was a problem hiding this comment.
Should we be setting type->tp_base in a stop the world pause?
Doesn't have to be part of this PR if it complicates things.
|
@colesbury Not sure if you would like this approach but it is a variation on your idea to modify the critical section to prevent release of the mutex. I changed it to work with a PyCriticalSection2 as well. https://github.com/nascheme/cpython/tree/type-slot-ts-release-hack Not using a critical section for the type dict looks kind of difficult to do. For example, |
This is an updated version of GH-131174, which was reverted. I figured the cleanest thing to do is make a new PR.
This is the same as the previous PR with the following additional change. The
update_all_slots()andtype_setattro()functions are now more careful when the world is stopped. Instead of doing the MRO lookups while the world is stopped, we do them all first and collect the slot pointers to be updated. Then, we stop the world and do those updates. This makes it much easier to confirm the code running during the stop-the-world is safe and that should avoid the deadlocks.The
test_opcachetest has become quite a bit slower. It seems to be due to mutex contention in the__getitem__and__getattribute__method assignment tests. I reduced the items count from 1000 to 100 to keep the test from becoming much slower.