bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

abouteiller · 2025-04-25T15:05:13Z

Setting OMPI_MPI_THREAD_LEVEL to a value different than requested in MPI_Init_thread would invoke the error handler, even though it is an useful override in some threaded library use cases.

ompi/mpi/c/init_thread.c.in

ompi/mpi/c/init.c.in

ompi/mpi/c/init_thread.c.in

github-actions · 2025-05-12T17:43:09Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

1a0d2f7: review comments

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2025-06-13T03:53:22Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

b056c23: OMPI_MPI_THREAD_LEVEL can now take 'multiple' 'MPI...

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

bosilca · 2025-06-13T14:51:43Z

Hold on a little on this one, I have a fancier solution I just didn't had the time to integrate it. The code is attached below, and it allows for partial matching for as long as the match is unique. As an example "s" will not be accepted because single and serialized, but si will be.

int check_env_value(const char** valid_prepositions, const char** keywords, int nb_keywords, const char* value)
{
    char *prep = NULL, *token = (char*)value  /* full match */;
    int pidx = 0, v = strtol(value, &prep, 10), found = -1;
    if ((0 == v) && (prep != value))
        return v;
    while(NULL != (prep = (char*)valid_prepositions[pidx])) {
        if( 0 == strncasecmp(prep, value, strlen(prep)) ) {
            token = value + strlen(prep);
            break;  /* got a token let's find a match */
        }
        pidx++;
    }

    for(int i = 0; i < nb_keywords; i++) {
        if( 0 == strncasecmp(keywords[i], token, strlen(token)) ) {
            if( -1 != found ) {  /* not the first match, bail out */
                return -1;
            }
            found = i;
        }
    }
    return found;
}

static const char* keywords[] = {"single", "serialized", "funneled", "multiple"};
static const char* prepositions[] = {"mpi_thread_", "thread_", NULL};

abouteiller · 2025-06-13T18:58:05Z

I think you have perms to push in my branch if you want to do that

jsquyres

I think we're going to need to get this documented, too. It's cool new functionality, but if it doesn't appear in a man page and/or some other docs, no one will ever use it.

`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 <[email protected]>

(single,etc) in addition to numeric 0-3 values Signed-off-by: Aurelien Bouteiller <[email protected]>

jsquyres

I took the liberty of updating the MPI_Init* man pages. You're welcome. 😇

I made minor changes to some MPI_Session_* man pages, too, but most changes were to

jsquyres · 2025-06-22T12:48:31Z

It occurred to me overnight that the updates I made in MPI_Init*(3) and MPI_Finalize(3) were indicative of the fact that none of our man pages had been updated to reflect the fact that the world model now exists (and is distinct from the MPI session model). So I updated even more text this morning to describe and clarify the MPI world model vs. the MPI session model.

I.e., in documenting the MPI_THREAD_* constant-setting functionality via the *OMPI_MPI_THREAD_LEVEL env variable, this PR became an excuse the update the man pages about the MPI world model vs. the MPI session model.

Reviewers should read these man pages:

docs/man-openmpi/man3/MPI_Init.3.rst

docs/man-openmpi/man3/MPI_Init_thread.3.rst

docs/man-openmpi/man3/MPI_Finalize.3.rst

docs/man-openmpi/man3/MPI_Finalized.3.rst

docs/man-openmpi/man3/MPI_Init.3.rst

docs/man-openmpi/man3/MPI_Init_thread.3.rst

docs/man-openmpi/man3/MPI_Initialized.3.rst

docs/man-openmpi/man3/MPI_Finalize.3.rst

ompi/runtime/ompi_mpi_init.c

bosilca · 2025-06-25T09:41:30Z

I made all changes I wanted to the documentation. As far as I am concerned this is ready to go.

abouteiller · 2025-06-25T17:10:04Z

LGTM as well

docs/man-openmpi/man3/MPI_Init_thread.3.rst

ompi/runtime/ompi_mpi_init.c

jsquyres · 2025-06-29T16:01:00Z

ompi/runtime/ompi_mpi_init.c

+            /* did we found a valid integer value? */
+            const int nb_keywords = sizeof(ompi_thread_level_values)/sizeof(ompi_thread_level_values[0]);
+            if (found >= nb_keywords) {
+                return OMPI_ERR_BAD_PARAM;
+            }


I don't quite grok this -- this works for the integer values 0, 1, 2, 3, but won't work if -- when ABI is merged in -- the user specifies the ABI integer value for MPI_THREAD_MULTIPLE (4096), for example.

Wouldn't it be better to iterate through the values of the ompi_thread_level_values and see if the integer found value matches one of them?

The documentation states 0 to 3, the MCA help string states 0 to 3, the previous OMPI versions were also based on 0 to 3 values. If we add the fact that the internal values, ABI or not, are hard to know for users and are not necessarily monotonically increasing, I don't think that accepting the internal values makes sense or is usefull for users (except maybe nitpicking).

We've previously allowed 0-3, which is probably "well known" (or at least easy to guess).

The ABI values are published in the MPI-5 document, so those fit at least some definition of "well known", too.

Meaning: if we're going to accept integer values, we should probably accept both 0-3 and the ABI values that are published in the MPI spec.

No sensible user should have to read Appendix A of the MPI standard in order to set the threading level.

This PR offers a sensible, backward compatible, solution. enough for me.

I'm not saying that they have to read the appendix. I'm saying that if we offer the ability to use integer values, we should offer both sets of integer values because someone will try to set the ABI value and be surprised that it didn't work.

I don't have a strong opinion on the ABI values,

but I will need this in v5, where there is no ABI support planned anyway

Setting OMPI_MPI_THREAD_LEVEL to a value different than requested in MPI_Init_thread would invoke the error handler, even though it is an useful override in some threaded library use cases.

I would recommend deferring to a separate PR when we implement ABI for v6, if only to keep the cherry-picking easier.

docs/man-openmpi/man3/MPI_Init_thread.3.rst

jsquyres · 2025-07-01T13:17:07Z

ompi/runtime/ompi_mpi_init.c

+            /* did we found a valid integer value? */
+            const int nb_keywords = sizeof(ompi_thread_level_values)/sizeof(ompi_thread_level_values[0]);
+            if (found >= nb_keywords) {
+                return OMPI_ERR_BAD_PARAM;
+            }


We've previously allowed 0-3, which is probably "well known" (or at least easy to guess).

The ABI values are published in the MPI-5 document, so those fit at least some definition of "well known", too.

Meaning: if we're going to accept integer values, we should probably accept both 0-3 and the ABI values that are published in the MPI spec.

jsquyres · 2025-07-02T13:31:08Z

I would recommend deferring to a separate PR when we implement ABI for v6,

Aren't you the one who started with PR saying that we needed to do this better to handle the ABI values? 👿

if only to keep the cherry-picking easier.

That's a fair point.

We have the hood up right now -- why not spend 15 more minutes and re-do the not-very-difficult logic to handle both 0-3 and the ABI values and be done with this? Why create technical debt that no one will remember to come back and fix? Just do it -- perhaps as one more commit on this PR, because @abouteiller makes a good point that we don't really want to cherry pick that to v5.0.x -- and let's be done with it.

bosilca · 2025-07-02T20:51:37Z

In case anyone is looking for an proper example of the use of the word tetrapyloctomy, here we have it.

In the current context, aka. OMPI does not even have ABI support, why are we investing time in this as if it was the best thing since sliced bread ?

jsquyres · 2025-07-04T12:05:50Z

We're investing time because:

ABI was one of the justifications cited for this PR.
ABI support is actively being developed (e.g., Updated ABI generation code and new libraries #13280). It likely won't make 6.0, but is also likely to be a target for 6.1.
The requested issue is easy to do; it will literally only take a few minutes.
I'm against creating technical debt.

bosilca · 2025-07-04T12:08:00Z

This is the opposite of investing, aka. wasting. I will not enumerate yet again the reasons, but such insistance from some is counterproductive, and less likely to encourage meaningful contributions (at the opposite of just "innovating" unnecessary features).

We have a real issue (which is actually the reason for this PR, not ABI stuff as you claim), and you're unreasonably holding it.

edgargabriel · 2025-07-04T15:22:21Z

@jsquyres would you consider this truly a blocker? I would have said that this something that can easily be addressed by a subsequent PR, there is no real benefit in insisting on having it as part of this PR.

jsquyres · 2025-07-04T15:27:32Z

This PR is not a blocker, no.

But I am opposed to creating technical debt. All problems can be addressed by creating a subsequent PR, but why do that when a problem has been identified during the code review?

The problem was introduced when new functionality was introduced into this PR. Here's the options I see:

Just fix the issue. It's easy. Seriously; just fix it and move on.
Remove the additional prefix functionality, and let @abouteiller's original fix go in. Perhaps bring back the prefix functionality in a different PR, where the issue is addressed.

…ages Including, but not limited to: * Added much more description of and distinction between the MPI world model and the MPI session model. Updated a lot of old, pre-MPI-world-model/pre-MPI-session-model text that was now stale / outdated, especially in the following pages: * MPI_Init(3), MPI_Init_thread(3) * MPI_Initialized(3) * MPI_Finalize(3) * MPI_Finalized(3) * MPI_Session_init(3) * MPI_Session_finalize(3) * Numerous formatting updates * Slightly improve the C code examples * Describe the mathematical relationship between the various MPI_THREAD_* constants in MPI_Init_thread(3) * Note that the mathematical relationships render nicely in HTML, but don't render entirely properly in nroff. This commit author is of the opinion that the nroff rendering is currently "good enough", and some Sphinx maintainer will fix it someday. * Add descriptions about the $OMPI_MPI_THREAD_LEVEL env variable and how it is used in MPI_Init_thread(3) * Added more seealso links Signed-off-by: Jeff Squyres <[email protected]>

bosilca · 2025-07-05T08:14:13Z

I can't caution this kind of autocratic behavior. My code is gone, I hope this solve all blockers for this PR.

jsquyres

Thanks for the changes.

abouteiller added the bug label Apr 25, 2025

abouteiller self-assigned this Apr 25, 2025

abouteiller linked an issue Apr 25, 2025 that may be closed by this pull request

OMPI_MPI_THREAD_LEVEL cannot override 'required' in MPI_Init_thread #13210

Open

github-actions bot added the Target: main label Apr 25, 2025

abouteiller commented Apr 25, 2025

View reviewed changes

ompi/mpi/c/init_thread.c.in Show resolved Hide resolved

abouteiller removed the bug label Apr 25, 2025

abouteiller force-pushed the bugfix/env-thread-level-ignored branch 4 times, most recently from b71461f to b481893 Compare April 25, 2025 16:29

abouteiller requested review from jsquyres and bosilca May 5, 2025 15:33

jsquyres reviewed May 5, 2025

View reviewed changes

ompi/mpi/c/init.c.in Outdated Show resolved Hide resolved

ompi/mpi/c/init_thread.c.in Outdated Show resolved Hide resolved

ompi/mpi/c/init_thread.c.in Outdated Show resolved Hide resolved

ompi/mpi/c/init_thread.c.in Outdated Show resolved Hide resolved

abouteiller force-pushed the bugfix/env-thread-level-ignored branch from b481893 to 1a0d2f7 Compare May 12, 2025 17:42

abouteiller force-pushed the bugfix/env-thread-level-ignored branch 2 times, most recently from 8ad7579 to fe5d20e Compare May 15, 2025 15:55

abouteiller force-pushed the bugfix/env-thread-level-ignored branch from b056c23 to 5059134 Compare June 13, 2025 03:57

jsquyres requested changes Jun 14, 2025

View reviewed changes

jsquyres mentioned this pull request Jun 21, 2025

fix some memleak #13306

Merged

abouteiller added 2 commits June 21, 2025 15:38

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than

27332fc

`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 <[email protected]>

OMPI_MPI_THREAD_LEVEL can now take 'multiple' 'MPI_THREAD_MULTIPLE'

3de2489

(single,etc) in addition to numeric 0-3 values Signed-off-by: Aurelien Bouteiller <[email protected]>

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 54682e4 to 0f4673c Compare June 21, 2025 19:39

jsquyres previously approved these changes Jun 21, 2025

View reviewed changes

jsquyres dismissed their stale review via 027fda5 June 21, 2025 19:48

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 0f4673c to 027fda5 Compare June 21, 2025 19:48

jsquyres previously approved these changes Jun 21, 2025

View reviewed changes

jsquyres dismissed their stale review via 1e66d4c June 22, 2025 12:45

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 027fda5 to 1e66d4c Compare June 22, 2025 12:45

jsquyres previously approved these changes Jun 22, 2025

View reviewed changes

bosilca reviewed Jun 23, 2025

View reviewed changes

docs/man-openmpi/man3/MPI_Init.3.rst Outdated Show resolved Hide resolved

docs/man-openmpi/man3/MPI_Init_thread.3.rst Show resolved Hide resolved

bosilca reviewed Jun 23, 2025

View reviewed changes

bosilca dismissed jsquyres’s stale review via d1d16e2 June 23, 2025 15:08

abouteiller commented Jun 24, 2025

View reviewed changes

docs/man-openmpi/man3/MPI_Finalize.3.rst Outdated Show resolved Hide resolved

ompi/runtime/ompi_mpi_init.c Outdated Show resolved Hide resolved

bosilca force-pushed the bugfix/env-thread-level-ignored branch 2 times, most recently from 046983c to b3e2f68 Compare June 25, 2025 09:39

bosilca previously approved these changes Jun 27, 2025

View reviewed changes

jsquyres requested changes Jun 29, 2025

View reviewed changes

bosilca dismissed their stale review via 47c8adc July 1, 2025 10:48

bosilca force-pushed the bugfix/env-thread-level-ignored branch from b3e2f68 to 47c8adc Compare July 1, 2025 10:48

jsquyres reviewed Jul 1, 2025

View reviewed changes

bosilca force-pushed the bugfix/env-thread-level-ignored branch from 47c8adc to 91e9ec6 Compare July 1, 2025 17:52

bosilca previously approved these changes Jul 2, 2025

View reviewed changes

bosilca dismissed their stale review via aff3afd July 5, 2025 08:10

bosilca force-pushed the bugfix/env-thread-level-ignored branch from 91e9ec6 to aff3afd Compare July 5, 2025 08:10

jsquyres approved these changes Jul 5, 2025

View reviewed changes

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

Are you sure you want to change the base?

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

Conversation

abouteiller commented Apr 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 12, 2025

Uh oh!

github-actions bot commented Jun 13, 2025

Uh oh!

bosilca commented Jun 13, 2025

Uh oh!

abouteiller commented Jun 13, 2025

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bosilca commented Jun 25, 2025

Uh oh!

abouteiller commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

jsquyres Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

bosilca Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

jsquyres Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

bosilca Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

jsquyres Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abouteiller Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsquyres Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Jul 2, 2025

Uh oh!

bosilca commented Jul 2, 2025

Uh oh!

jsquyres commented Jul 4, 2025

Uh oh!

bosilca commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edgargabriel commented Jul 4, 2025

Uh oh!

jsquyres commented Jul 4, 2025

Uh oh!

jsquyres left a comment •

edited

Loading

jsquyres commented Jun 22, 2025 •

edited

Loading

jsquyres Jul 1, 2025 •

edited

Loading

bosilca commented Jul 4, 2025 •

edited

Loading