bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211
Conversation
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
4 times, most recently
from
b71461f to
b481893
Compare
ompi/mpi/c/init_thread.c.in
Outdated
| */ | ||
| if (NULL != (env = getenv("OMPI_MPI_THREAD_LEVEL"))) { | ||
| int env_required = atoi(env); | ||
| err_arg_required = err_arg_required || |
There was a problem hiding this comment.
My biggest concern here is that the user is required to know the valid thread level values as defined in the MPI ABI header. We should keep doing what we do right now, aka 0-3, and use a shift operation to convert it to the ABI value env_required = (1 << (10 + atoi(env)));
There was a problem hiding this comment.
For now, the valid values are still [0-3] (because our internal ABI is not the same as the standardized ABI (yet?), I'll add this consideration to the comment requested by Jeff so we don't forget when the time comes.
There was a problem hiding this comment.
FWIW: we plan to support both the existing OMPI ABI and the official MPI Forum MPI ABI. Hence, we'll probably want code that can handle both situations.
There was a problem hiding this comment.
the important part is that we don't want the users to have to enter the ABI values for the environment variable for the thread level. We want this to remain indexed as it is today.
There was a problem hiding this comment.
Hmm. Good point.
Maybe we should also accept strings? MPI_THREAD_SINGLE, MPI_THREAD_MULTIPLE, ... etc. I.e., something that doesn't depend on a numeric value.
There was a problem hiding this comment.
can we defer this to when we implement ABI compat?
There was a problem hiding this comment.
It might behoove us to do this now, so that it can be in 6.0.0 -- i.e., at a major version change. ABI may or may not make it into 6.0.0 (e.g., it won't be ready until -- at the earliest -- the end of this summer, and we may have already released 6.0.0 by then, in which case, ABI would be in 6.1.0).
It can't be that hard to implement to also check for string values. I'd suggest the following case-insensitive values:
mpi_thread_singleandsinglempi_thread_serializedandserializedmpi_thread_funneledandfunneledmpi_thread_multipleandmultiple
There was a problem hiding this comment.
Besides, didn't you make the argument that you want to check for the individual values because of ABI? You can't argue that we should do this PR because ABI is coming, but we shouldn't bother doing all the things because ABI isn't here yet. 😇
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
from
b481893 to
1a0d2f7
Compare
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: 1a0d2f7: review comments
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
from
1a0d2f7 to
8ad7579
Compare
`requested` in `MPI_Init_thread` would invoke the error handler, even though it is an useful override in some threaded library use cases. Signed-off-by: Aurelien Bouteiller <abouteil@amd.com>
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
from
8ad7579 to
fe5d20e
Compare
Setting OMPI_MPI_THREAD_LEVEL to a value different than
requestedinMPI_Init_threadwould invoke the error handler, even though it is an useful override in some threaded library use cases.