Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mini] Add missing membars when initializing rgctx entries #16904

Merged
merged 2 commits into from
Sep 18, 2019

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Sep 17, 2019

Whenever we are publishing changes to the rgctx arrays, we are racing with rgctx_lazy_fetch_trampoline, so we need to make sure any pointers we set there must have their contents initialized.

Whenever we are publishing changes to the rgctx arrays, we are racing with rgctx_lazy_fetch_trampoline, so we need to make sure any pointers we set there must have their contents initialized.
@BrzVlad
Copy link
Member Author

BrzVlad commented Sep 17, 2019

This fixes very random crashes on arm. The offending missing membar is the one needed when storing the newly allocated rgctx_array. This was a little bit confusing because alloc_rgctx_array allocates the array using mono_domain_alloc0 which clears the allocated array and then releases the domain lock, which one can expect to have a release semantic. It turns out in this particular case, we were already holding the domain lock multiple times (doing only a nest decrement) and only the last release has release semantics.

Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just make the rgctx array alloc have a memory barrier inside? It seems like we would always want a writer barrier to avoid making a partially initialized array visible to anyone

@BrzVlad
Copy link
Member Author

BrzVlad commented Sep 18, 2019

I added the memory barrier outside the allocation because I feel it is the responsibility of the user of the array to add memory barrier if it publishes it to lock free data structures. The allocator just allocates. I wouldn't want to fall on the path of adding memory barriers everywhere, just in case.

@vargaz
Copy link
Contributor

vargaz commented Sep 18, 2019

Nice work.

@marek-safar
Copy link
Member

@monojenkins backport 2019-08

@lewurm
Copy link
Contributor

lewurm commented Sep 18, 2019

@monojenkins squash

@monojenkins monojenkins merged commit b2739b5 into mono:master Sep 18, 2019
monojenkins added a commit that referenced this pull request Sep 18, 2019
…16909)

[2019-08] [mini] Add missing membars when initializing rgctx entries

Whenever we are publishing changes to the rgctx arrays, we are racing with rgctx_lazy_fetch_trampoline, so we need to make sure any pointers we set there must have their contents initialized.


Backport of #16904.

/cc @marek-safar @BrzVlad
ManickaP pushed a commit to ManickaP/runtime that referenced this pull request Jan 20, 2020
…#16904)

[mini] Add missing membars when initializing rgctx entries

Whenever we are publishing changes to the rgctx arrays, we are racing with rgctx_lazy_fetch_trampoline, so we need to make sure any pointers we set there must have their contents initialized.



Commit migrated from mono/mono@b2739b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants