Skip to content

Commit

Permalink
ompi_mpi_init: fix race condition
Browse files Browse the repository at this point in the history
There was a race condition in 35438ae: if multiple threads invoked
ompi_mpi_init() simultaneously (which could happen from both MPI and
OSHMEM), the code did not catch this condition -- Bad Things would
happen.

Now use an atomic cmp/set to ensure that only one thread is able to
advance ompi_mpi_init from NOT_INITIALIZED to INIT_STARTED.

Additionally, change the prototype of ompi_mpi_init() so that
oshmem_init() can safely invoke ompi_mpi_init() multiple times (as
long as MPI_FINALIZE has not started) without displaying an error.  If
multiple threads invoke oshmem_init() simultaneously, one of them will
actually do the initialization, and the rest will loop waiting for it
to complete.

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
  • Loading branch information
jsquyres committed Jun 6, 2018
1 parent 64a5baa commit 67ba8da
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 30 deletions.
6 changes: 3 additions & 3 deletions ompi/mpi/c/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2007-2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2007-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2007-2008 Sun Microsystems, Inc. All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
Expand Down Expand Up @@ -63,9 +63,9 @@ int MPI_Init(int *argc, char ***argv)
don't lose anything) */

if (NULL != argc && NULL != argv) {
err = ompi_mpi_init(*argc, *argv, required, &provided);
err = ompi_mpi_init(*argc, *argv, required, &provided, false);
} else {
err = ompi_mpi_init(0, NULL, required, &provided);
err = ompi_mpi_init(0, NULL, required, &provided, false);
}

/* Since we don't have a communicator to invoke an errorhandler on
Expand Down
6 changes: 3 additions & 3 deletions ompi/mpi/c/init_thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* Copyright (c) 2010 Oak Ridge National Labs. All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2015-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2016 Los Alamos National Security, LLC. All rights
* reserved.
* $COPYRIGHT$
Expand Down Expand Up @@ -63,9 +63,9 @@ int MPI_Init_thread(int *argc, char ***argv, int required,
don't lose anything) */

if (NULL != argc && NULL != argv) {
err = ompi_mpi_init(*argc, *argv, required, provided);
err = ompi_mpi_init(*argc, *argv, required, provided, false);
} else {
err = ompi_mpi_init(0, NULL, required, provided);
err = ompi_mpi_init(0, NULL, required, provided, false);
}

/* Since we don't have a communicator to invoke an errorhandler on
Expand Down
5 changes: 4 additions & 1 deletion ompi/runtime/mpiruntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ void ompi_mpi_thread_level(int requested, int *provided);
* @param argv argv, typically from main() (IN)
* @param requested Thread support that is requested (IN)
* @param provided Thread support that is provided (OUT)
* @param reinit_ok Return successfully (with no error) if someone has
* already called ompi_mpi_init().
*
* @returns MPI_SUCCESS if successful
* @returns Error code if unsuccessful
Expand All @@ -186,7 +188,8 @@ void ompi_mpi_thread_level(int requested, int *provided);
*
* It is permissable to pass in (0, NULL) for (argc, argv).
*/
int ompi_mpi_init(int argc, char **argv, int requested, int *provided);
int ompi_mpi_init(int argc, char **argv, int requested, int *provided,
bool reinit_ok);

/**
* Finalize the Open MPI MPI environment
Expand Down
47 changes: 26 additions & 21 deletions ompi/runtime/ompi_mpi_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,8 @@ static void fence_release(int status, void *cbdata)
OPAL_POST_OBJECT(active);
}

int ompi_mpi_init(int argc, char **argv, int requested, int *provided)
int ompi_mpi_init(int argc, char **argv, int requested, int *provided,
bool reinit_ok)
{
int ret;
ompi_proc_t** procs;
Expand All @@ -384,29 +385,33 @@ int ompi_mpi_init(int argc, char **argv, int requested, int *provided)

ompi_hook_base_mpi_init_top(argc, argv, requested, provided);

/* Ensure that we were not already initialized or finalized.
This lock is held for the duration of ompi_mpi_init() and
ompi_mpi_finalize(). Hence, if we get it, then no other thread
is inside the critical section (and we don't have to check the
*_started bool variables). */
opal_atomic_rmb();
int32_t state = ompi_mpi_state;
/* Ensure that we were not already initialized or finalized. */
int32_t expected = OMPI_MPI_STATE_NOT_INITIALIZED;
int32_t desired = OMPI_MPI_STATE_INIT_STARTED;
opal_atomic_wmb();
if (!opal_atomic_compare_exchange_strong_32(&ompi_mpi_state, &expected,
desired)) {
// If we failed to atomically transition ompi_mpi_state from
// NOT_INITIALIZED to INIT_STARTED, then someone else already
// did that, and we should return.
if (expected >= OMPI_MPI_STATE_FINALIZE_STARTED) {
opal_show_help("help-mpi-runtime.txt",
"mpi_init: already finalized", true);
return MPI_ERR_OTHER;
} else if (expected >= OMPI_MPI_STATE_INIT_STARTED) {
// In some cases (e.g., oshmem_shmem_init()), we may call
// ompi_mpi_init() multiple times. In such cases, just
// silently return successfully.
if (reinit_ok) {
return MPI_SUCCESS;
}

if (state >= OMPI_MPI_STATE_FINALIZE_STARTED) {
opal_show_help("help-mpi-runtime.txt",
"mpi_init: already finalized", true);
return MPI_ERR_OTHER;
} else if (state >= OMPI_MPI_STATE_INIT_STARTED) {
opal_show_help("help-mpi-runtime.txt",
"mpi_init: invoked multiple times", true);
return MPI_ERR_OTHER;
opal_show_help("help-mpi-runtime.txt",
"mpi_init: invoked multiple times", true);
return MPI_ERR_OTHER;
}
}

/* Indicate that we have *started* MPI_INIT* */
opal_atomic_wmb();
opal_atomic_swap_32(&ompi_mpi_state, OMPI_MPI_STATE_INIT_STARTED);

/* Figure out the final MPI thread levels. If we were not
compiled for support for MPI threads, then don't allow
MPI_THREAD_MULTIPLE. Set this stuff up here early in the
Expand Down
9 changes: 7 additions & 2 deletions oshmem/runtime/oshmem_shmem_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,13 @@ int oshmem_shmem_init(int argc, char **argv, int requested, int *provided)
OMPI_TIMING_INIT(32);

if (!oshmem_shmem_initialized) {
if (ompi_mpi_state < OMPI_MPI_STATE_INIT_COMPLETED) {
ret = ompi_mpi_init(argc, argv, requested, provided);
ret = ompi_mpi_init(argc, argv, requested, provided, true);

// It's posible that another thread is initializing MPI and
// has not completed yet. Keep checking until it is
// completed.
while (ompi_mpi_state < OMPI_MPI_STATE_INIT_COMPLETED) {
usleep(1);
}
OMPI_TIMING_NEXT("ompi_mpi_init");

Expand Down

0 comments on commit 67ba8da

Please sign in to comment.