Skip to content

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

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 <niethammer@hlrs.de>
@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 FG-TUM and HomesGH 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cniethammer What is the current state of this PR? #359 was merged, so I think we can continue working on this PR, i.e. fix ::exit(EXIT_FAILURE);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not find time yet but will see to do so in the next weeks.

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