Skip to content

Conversation

@bosilca
Copy link
Member

@bosilca bosilca commented Jul 16, 2022

Fixes a bunch of issues related to OMPI compilation (incorrect use of defines, missing SPC constants) and an issue related to error returning from MPI_Start. Need to be pushed on all branches.

@bosilca bosilca requested a review from devreal July 16, 2022 04:27
bosilca referenced this pull request Jul 16, 2022
bosilca added 3 commits July 16, 2022 00:31
Use OMPI_ERRHANDLER_NOHANDLE_RETURN instead of INVOKE.

Signed-off-by: George Bosilca <[email protected]>
Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Sorry about that, I copied the wrong code from startall.c. I realized that MPI_Startall does not check the return from the start call at all and always returns MPI_SUCCESS :/ See https://github.com/open-mpi/ompi/blob/main/ompi/mpi/c/startall.c#L120

@bosilca bosilca requested a review from hppritcha July 16, 2022 18:33
@bosilca
Copy link
Member Author

bosilca commented Jul 16, 2022

It is not very clear to me what MPI_Startall should return nor, assuming some error state, in what state it should leave the requests who did not generate the error. The current assumption is that as long as the requests are valid and can be started, the error code will be returned upon completion.

@devreal
Copy link
Contributor

devreal commented Jul 16, 2022

It is not very clear to me what MPI_Startall should return nor, assuming some error state, in what state it should leave the requests who did not generate the error. The current assumption is that as long as the requests are valid and can be started, the error code will be returned upon completion.

So should we instead revert my patch that introduced the error code check to always return MPI_SUCCESS from MPI_Start? In test/wait we have at least a chance to fill a status....

@awlauria
Copy link
Contributor

@devreal @bosilca do these fixes need to go to v4.0.x? It looks like this was merged: #10553

@bosilca
Copy link
Member Author

bosilca commented Jul 19, 2022

It seems you're right, this PR needs to be ported on the release branches.

@jsquyres
Copy link
Member

jsquyres commented Sep 21, 2022

Just to confirm: these changes were brought to v5.0.x in #10603.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants