Skip to content

init/finalize: extensions #1007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 15, 2015
Merged

init/finalize: extensions #1007

merged 3 commits into from
Oct 15, 2015

Conversation

jsquyres
Copy link
Member

Proposed extensions for Open MPI:

  • If MPI_INITLIZED is invoked and MPI is only partially initialized, wait until MPI is fully initialized before returning.
  • If MPI_FINALIZED is invoked and MPI is only partially finalized, wait until MPI is fully finalized before returning.
  • If the ompi_mpix_allow_multi_init MCA param is true, allow MPI_INIT and MPI_INIT_THREAD to be invoked multiple times without error (MPI will be safely initialized only the first time it is invoked).

@hppritcha @bosilca This is a bit more than we talked about on the phone last week. I ended up using a mutex instead of atomics because I have to check multiple values, so it made more sense to put all the checks within a mutex lock.

@jsquyres jsquyres added the RFC label Oct 11, 2015
bosilca
bosilca reviewed Oct 11, 2015
View reviewed changes
ompi/mpi/c/finalized.c
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
usleep(1);
opal_mutex_lock(&ompi_mpi_bootstrap_mutex);
}

Copy link
Member

Choose a reason for hiding this comment

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

The result of this loop is basically a continuous loop doing a "lock + unlock"every 1us, in all cases. As you are using a mutex, why don't you rely on the trigger of the mutex by the thread that succeeded to ensure the sequentiality of this code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I specifically didn't hold the lock for the duration of ompi_mpi_init() / ompi_mpi_finalize(). Hence, I can't have the thread here just block waiting for the lock (e.g., which would naturally wait until the thread in ompi_mpi_init() completed the function).

Instead, I'm only using the lock to access the variables. Hence, I have to loop around checking it, unlocking, delaying, and locking again.

Are you advocating that I should hold the lock through ompi_mpi_init() / ompi_mpi_finalize()? I could probably do that (and then get rid of this loop, and the similar one in INITIALIZED)

This was referenced Oct 12, 2015
Merged
Closed
@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from de8f493 to 58d6f86 Compare October 13, 2015 13:32
@jsquyres
Copy link
Member Author

@bosilca Code changed to hold the lock for the bulk of the duration of ompi_mpi_init() and ompi_mpi_finalize(), thereby removing the need to loop checking the values of variables.

@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from 58d6f86 to e0f49fa Compare October 13, 2015 14:04
@bosilca
Copy link
Member

bosilca commented Oct 13, 2015

Thanks Jeff. Also, adding the blurb of text to the man pages is a great idea. The code looks good 👍

@jsquyres
Copy link
Member Author

@hppritcha Does this look good to you?

@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from e0f49fa to b2a5f31 Compare October 13, 2015 16:37
@bosilca bosilca mentioned this pull request Oct 14, 2015
Merged
@jsquyres
Copy link
Member Author

The more I think about this, the less I think it makes sense to allow MPI_INIT[_THREAD] to be invoked multiple times safely, but only allow MPI_FINALIZE to be invoked once.

Should we give up on the idea of invoking MPI_INIT[_THREAD] multiple times, and just have this PR essentially be the bootstrap_mutex stuff?

jsquyres
jsquyres reviewed Oct 14, 2015
View reviewed changes
ompi/runtime/ompi_mpi_init.c
bool ompi_mpi_finalized = false;
bool ompi_rte_initialized = false;
int32_t ompi_mpi_finalize_started = false;
opal_mutex_t ompi_mpi_bootstrap_mutex;
Copy link
Member Author

Choose a reason for hiding this comment

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

@hjelmn @bosilca What is the Right way to initialize this mutex? I notice that it's a class -- not a plain pthread mutex. Should I use a simple pthread mutex instead, and then statically initialize it with PTHREAD_MUTEX_INITIALIZER?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the Right way is #1026.

@hppritcha
Copy link
Member

Looks excellent.

@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from 0b6b809 to d91b717 Compare October 15, 2015 02:12
@lanl-ompi
Copy link
Contributor

Test FAILed.

@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from d91b717 to 76ad8f2 Compare October 15, 2015 02:23
@lanl-ompi
Copy link
Contributor

Test FAILed.

@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from 76ad8f2 to b262382 Compare October 15, 2015 02:25
@lanl-ompi
Copy link
Contributor

Test FAILed.

3 similar comments
@lanl-ompi
Copy link
Contributor

Test FAILed.

@lanl-ompi
Copy link
Contributor

Test FAILed.

@lanl-ompi
Copy link
Contributor

Test FAILed.

@jsquyres
init/finalize: extensions
f5ad90c 
Proposed extensions for Open MPI:

- If MPI_INITLIZED is invoked and MPI is only partially initialized,
  wait until MPI is fully initialized before returning.
- If MPI_FINALIZED is invoked and MPI is only partially finalized,
  wait until MPI is fully finalized before returning.
- If the ompi_mpix_allow_multi_init MCA param is true, allow MPI_INIT
  and MPI_INIT_THREAD to be invoked multiple times without error (MPI
  will be safely initialized only the first time it is invoked).
@jsquyres
man: update man pages for Init*/Finalize*
338257a 
Update language surrounding initialization and finalization in
MPI_Init[_thread], MPI_Initialized, MPI_Finalize, and MPI_Finalized.
@jsquyres
help-mpi-api.txt: remove now-stale help messages
40b4d5d
@jsquyres jsquyres force-pushed the rfc/init-finalize-updates branch from b262382 to 40b4d5d Compare October 15, 2015 16:44
@jsquyres
Copy link
Member Author

I removed the MPIX MCA param and the ability to call MPI_INIT[_THREAD] multiple times. It's a can of worms. If someone else wants to tackle that issue, feel free to do so.

This PR is now just:

  • If MPI_INITLIZED is invoked and MPI is only partially initialized, wait until MPI is fully initialized before returning.
  • If MPI_FINALIZED is invoked and MPI is only partially finalized, wait until MPI is fully finalized before returning.

jsquyres added a commit that referenced this pull request Oct 15, 2015
@jsquyres
Merge pull request #1007 from jsquyres/rfc/init-finalize-updates
9fe4ebb 
init/finalize: extensions
@jsquyres jsquyres merged commit 9fe4ebb into open-mpi:master Oct 15, 2015
@jsquyres jsquyres deleted the rfc/init-finalize-updates branch October 15, 2015 17:55
@jsquyres jsquyres mentioned this pull request Oct 15, 2015
Merged
@jsquyres jsquyres mentioned this pull request Nov 2, 2015
Closed
jsquyres pushed a commit to jsquyres/ompi that referenced this pull request Aug 23, 2016
@hppritcha
Merge pull request open-mpi#1007 from jsquyres/pr/v2.0.0/fix-deprecat…
2d08dd1 
…ed-perl-construct

v2.0.0: autogen: fix deprecated construct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
MPI-3.1 RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsquyres @bosilca @hppritcha @lanl-ompi