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

Use MPI_Init_thread if OpenMP is enabled #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cniethammer
Copy link
Contributor

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.

@cniethammer cniethammer force-pushed the fix_mpi_init branch 3 times, most recently from 3236f17 to 3dca287 Compare May 21, 2024 08:32
@cniethammer cniethammer requested review from FG-TUM and HomesGH May 22, 2024 08:44
Copy link
Member

@FG-TUM FG-TUM left a 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

src/MarDyn.cpp Outdated Show resolved Hide resolved
src/MarDyn.cpp Outdated Show resolved Hide resolved
src/MarDyn.cpp Outdated Show resolved Hide resolved
src/MarDyn.cpp Outdated Show resolved Hide resolved
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]>
@cniethammer
Copy link
Contributor Author

Based on the feedback and diving deeper into the MPI standard, I refactored this PR.

Note:

  • The logging infrastructure is not available at this point in the code. Therefore mardyn_exit() cannot be used, because it uses the Logger internally.
  • I replaced MPI_Init() with MPI_Init_thread() entirely now. The MPI standard says here 'A call to MPI_INIT has the same effect as a call to MPI_INIT_THREAD with a required = MPI_THREAD_SINGLE." (MPI 4.1 p.486)

@cniethammer
Copy link
Contributor Author

@HomesGH Can we merge this after the taking into account our comment and changes?

@HomesGH
Copy link
Contributor

HomesGH commented Nov 26, 2024

@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 .

@cniethammer cniethammer requested review from HomesGH and FG-TUM December 6, 2024 18:38
Copy link
Member

@FG-TUM FG-TUM left a 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.

Comment on lines +144 to +146
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);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested 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);
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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

-> here you go :)

Copy link
Contributor Author

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.

src/MarDyn.cpp Show resolved Hide resolved
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.

3 participants