Split flags in struct MVMCollectable to avoid data races when setting MVM_CF_HAS_OBJECT_ID and MVM_CF_REF_FROM_GEN2
#1336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This permits us to avoid a data race when one thread needs to set MVM_CF_HAS_OBJECT_ID and a second needs to set MVM_CF_REF_FROM_GEN2. For now, don't change the flag bit values used in the two new members.
Change the values of the MVM_CF_* flags to sit in the range 1-128.
nwc10
added a commit
to nwc10/rakudo
that referenced
this pull request
Aug 11, 2020
As part of the fix for the data race described in MoarVM/MoarVM#1336 the flags member of struct MVMCollectable needs to be changed from a single MVMuint16 to two MVMuint8 values. As `p6setfirstflag` and `p6takefirstflag` directly manipulate this struct, they need to be updated to reflect the changes. Implemented with conditional compilation so that it will work with MoarVM before and after these changes. Once MoarVM is updated, the conditional compilation can be removed.
jnthn
approved these changes
Aug 13, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
"paranoia" checks added as part of work on the new hash tables reveal that there is a nasty data race when setting flag bits in
MVMCollectableSpecifically, if one thread calls
MVM_gc_object_idfor an object that does not yet have an ID, and another thread callsMVM_gc_write_barrier_hit_byfor the same object, then each function needs to perform a read, modify, write on the object's flags. If the data race is hit, then the setting of one of the flags bits is lost. IfMVM_CF_HAS_OBJECT_IDfails to be set, then we simply leak a little memory in the gen2 pools. IfMVM_CF_REF_FROM_GEN2fails to be set, then the GC doesn't function properly.A solution to this is to split the flags from a single
MVMuint16into twoMVMuint8s and partition the flags bits across the two, so that these two hot flags are not in the same byte. This way, each can be updated without causing a data race with the other. The proposed split turns out to be quite easily memorable - other thanMVM_CF_HAS_OBJECT_ID, all "gen2", "nursery" and GC related flags go intoflags2, all others intoflags1.Unfortunately rakudo's extension ops have an unhealthy amount of tight coupling with MoarVM, including (ab)using a free bit in the flags for
FIRSThandling. Hence rakudo needs to be changed before it can build with these changes. The proposed patch to rakudo makes it build with both MoarVMmasterand this branchSuggested actions
(intersperse with testing as necessary)(as expected, the automatic tests here fail currently, because Rakudo has not been patched)
Problem found by @dogbert17, cause diagnosed by @niner