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

Fix possible data race in MsgStack::push() #1955

Merged
merged 3 commits into from
Mar 10, 2020
Merged

Conversation

johnomotani
Copy link
Contributor

Fix for issue noted by @d7919 here #1803 (comment)

Would it be better with a BOUT_OMP(single) as suggested here #1803 (comment)?

@johnomotani johnomotani added bugfix small-change Changes less than 100 lines - should be quick to review labels Mar 8, 2020
ZedThree
ZedThree previously approved these changes Mar 9, 2020
@ZedThree
Copy link
Member

ZedThree commented Mar 9, 2020

single means we'd only get the message stack from one thread, which implies they should all be doing the same thing and precludes different tasks.

We currently only apply OpenMP to loops, so this would be fine. I don't believe we have plans to do any other kind of task sharing, so I would be happy making it single.

single means we only get the message stack from one thread, which
implies they should all be doing the same thing and precludes different
tasks. We currently only apply OpenMP to loops, so this is fine.
@ZedThree
Copy link
Member

ZedThree commented Mar 9, 2020

I think we also need single in MsgStack::pop

@johnomotani
Copy link
Contributor Author

It should be single in both overloads of MsgStack::pop right?

Needed to be consistent with using BOUT_OMP(single) in MsgStack::push().
@ZedThree
Copy link
Member

ZedThree commented Mar 9, 2020

Yep

@ZedThree ZedThree merged commit 4eb064f into next Mar 10, 2020
@ZedThree ZedThree deleted the MsgStack-data-race-fix branch March 10, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix small-change Changes less than 100 lines - should be quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants