-
Notifications
You must be signed in to change notification settings - Fork 15
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
Use MPI_Init_thread if OpenMP is enabled #306
base: master
Are you sure you want to change the base?
Conversation
3236f17
to
3dca287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bit of readability
If MPI is used in a multi-threaded environment the MPI implementation has to be aware of the calling context of MPI methods. Therefore, initialize the MPI environment with MPI_Init_thread requesting the desired thread support level. So far, ls1 built with OpenMP support makes MPI calls only from the main thread, i.e., uses MPI_THREAD_FUNNELED. For details see the MPI standard. Signed-off-by: Christoph Niethammer <[email protected]>
Based on the feedback and diving deeper into the MPI standard, I refactored this PR. Note:
|
@HomesGH Can we merge this after the taking into account our comment and changes? |
I am fine with this PR but please note this open conversation by @FG-TUM . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One style suggestion and one important change.
std::cerr << "ERROR: ls1 was build with OpenMP. However, the MPI implementation does not provide the necessary thread support level." << std::endl; | ||
MPI_Abort(MPI_COMM_WORLD, EXIT_FAILURE); | ||
::exit(EXIT_FAILURE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::cerr << "ERROR: ls1 was build with OpenMP. However, the MPI implementation does not provide the necessary thread support level." << std::endl; | |
MPI_Abort(MPI_COMM_WORLD, EXIT_FAILURE); | |
::exit(EXIT_FAILURE); | |
MARDYN_EXIT("ERROR: ls1 was build with OpenMP. However, the MPI implementation does not provide the necessary thread support level."); |
Please use the exit macro for consistent exit behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use the mardyn_exit
macro as it relies on the logger infrastructure, which is not initialized at this point in the program (and may also dependson the MPI part). However, IMHO, we are so early in the program, that we do not need the extensive logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, IMHO, we are so early in the program, that we do not need the extensive logger.
I respectfully disagree, especially when looking at your related PR #358. Adding exceptions to a pattern like this makes the code more confusing, prone to set bad precedent for new developers, and needs clear documentation.
I'll add a suggestion on how to tackle the issue in the mentioned PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then I'm looking forward to your suggestion to address the Logger issues in a cleaner way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> here you go :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the inspiration!
I will update this PR when #359 is merged.
If MPI is used in a multi-threaded environment the MPI implementation has to be aware of the calling context of MPI methods. Therefore, initialize the MPI environment with MPI_Init_thread requesting the desired thread support level. So far ls1 makes MPI calls only from the main thread, i.e., uses MPI_THREAD_FUNNELED. For details see the MPI standard.