ompi_mpi_init: fix race condition #5234
Conversation
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>
ompi/runtime/ompi_mpi_init.c
Outdated
| // silently return successfully. | ||
| if (reinit_ok) { | ||
| return MPI_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Aren't the threads that failed to initialize expected to wait until the one that is currently doing the init succesfully complete the initialization ?
There was a problem hiding this comment.
I put that check in oshmem_shmem_init() (vs. here). I guess it's better to move it here.
Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
|
Thanks @jsquyres |
|
Blarg. You merged before I squashed. 😦 |
|
sorry, I thought we were done with the discussion, and the patch was looking good. |
| // 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); |
There was a problem hiding this comment.
Correct: MPI defines that you are only allowed to call MPI_INIT / MPI_FINALIZE once.
We added this ompi_mpi_init() ability to "re-init" just for OSHMEM (I put it in quotes because it's not actually re-initializing -- it's just returning safely if MPI is already initialized... but not yet finalized).
But we can't help you if MPI has already been finalized -- there's way too much infrastructure that has been shut down, and ompi_mpi_init() was not designed to actually re-initialize. MPI sessions may change this reality, but that's still a ways off. And definitely outside the scope of this PR. 😄
| // silently return successfully once the initializing | ||
| // thread has completed. | ||
| if (reinit_ok) { | ||
| while (ompi_mpi_state < OMPI_MPI_STATE_INIT_COMPLETED) { |
There was a problem hiding this comment.
Do we need a read memory barrier here?
There was a problem hiding this comment.
ompi_mpi_state is volatile and we are using a usleep(1). I don't think the rmb would be necessary.
There was a problem hiding this comment.
And there was a write memory barrier every time we wrote to ompi_mpi_state.
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