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

ompi: always enable MPI_THREAD_MULTIPLE support #1397

Merged
merged 1 commit into from
Apr 23, 2016

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Feb 23, 2016

This commit removes the --with-mpi-thread-multiple option and forces
MPI_THREAD_MULTIPLE support. This cleans up an abstration violation
in opal where OMPI_ENABLE_THREAD_MULTIPLE determines whether the
opal_using_threads is meaningful. To reduce the performance hit on
MPI_THREAD_SINGLE programs an OPAL_UNLIKELY is used for the
check on opal_using_threads in OPAL_THREAD_* macros.

This commit does not clean up the arguments to the various functions
that take whether muti-threading support is enabled. That should be
done at a later time.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

This commit removes the --with-mpi-thread-multiple option and forces
MPI_THREAD_MULTIPLE support. This cleans up an abstration violation
in opal where OMPI_ENABLE_THREAD_MULTIPLE determines whether the
opal_using_threads is meaningful. To reduce the performance hit on
MPI_THREAD_SINGLE programs an OPAL_UNLIKELY is used for the
check on opal_using_threads in OPAL_THREAD_* macros.

This commit does not clean up the arguments to the various functions
that take whether muti-threading support is enabled. That should be
done at a later time.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@hjelmn
Copy link
Member Author

hjelmn commented Feb 23, 2016

@bwbarrett Do you remember why the checks for OMPI_ENABLE_THREAD_MULTIPLE were left in opal/thread/mutex.h?

@jsquyres, @bosilca, @rhc54, @gpaulsen This is the PR proposed on 2/23 at the developer meeting. The hit of this change is minimal on x86 but should be verified on power.

@hjelmn hjelmn added this to the v2.0.1 milestone Feb 23, 2016
@hjelmn hjelmn self-assigned this Feb 23, 2016
@hjelmn
Copy link
Member Author

hjelmn commented Feb 23, 2016

:bot:retest:

@gpaulsen
Copy link
Member

@nysal - We don't yet have Power 8 setup for Jenkins testing. Do you mind checking?

@bwbarrett
Copy link
Member

I think we were worried about the performance impact, but I honestly don't remember. Whatever it is, seems not worth it if MPI_THREAD_MULTIPLE is the default...

@hjelmn
Copy link
Member Author

hjelmn commented Apr 19, 2016

@jsquyres, @bosilca Should the be merged?

@bosilca
Copy link
Member

bosilca commented Apr 19, 2016

This patch completely removes the non thread-safe case, which, based on my understanding of the outcome of the discussion at the developer's meeting, is an extension of what we were originally looking for.

@jsquyres
Copy link
Member

@hjelmn Is this PR ready to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants