-
Notifications
You must be signed in to change notification settings - Fork 903
v5.0.x: Add Part Comm component #9593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v5.0.x: Add Part Comm component #9593
Conversation
Signed-off-by: Matthew Dosanjh <[email protected]> Addressed ordering issues as per review by @hppritcha fixed ordering in ompi/mpi/c makefiles (cherry picked from commit d79e13d)
According to the latest MPI-4.0 draft (as of May 2021), the "buf" arg to MPI_Precv_init() is not const. Signed-off-by: Jeff Squyres <[email protected]> (cherry picked from commit 65bb9e6)
- Remove dead code - Fix shadow variable declarations - Cast %p arguments to (void*) - Make integer comparisons agree in sign - Make a local function static so that it does not require a prior declaration Signed-off-by: Jeff Squyres <[email protected]> (cherry picked from commit 91c4c29)
Signed-off-by: Emmanuel Brelle <[email protected]> (cherry picked from commit cfe0906)
Signed-off-by: Todd Kordenbrock <[email protected]> (cherry picked from commit 2834ecc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers all of the code and changes that have been made to support MPI Partitioned Communication.
exit(-1); | ||
} | ||
ompi_part_persist.part_comm_ready = 0; | ||
err = ompi_comm_idup(&ompi_mpi_comm_world.comm, &ompi_part_persist.part_comm_setup, &ompi_part_persist.part_comm_sreq); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ompi_part_persist.init_world
is initialized to -1
and changed to 0
only by ranks that want to perform partitioned communication. In the case some ranks do no partitioned communications at all, since ompi_comm_idup need all the ranks of the communicator, I guess this call will block the entire MPI application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. It shouldn't block on this call but the extra communicators will never get set up and the partitioned requests will never complete. I can move this logic to the progress function, (my initial hesitance was creating the two extra communicators if they weren't going to be used).
Why was this merged? Per the discussion above, it looks like @mdosanjh was still going to do something additional...? |
@mdosanjh can you bring any changes back to master and then cherry-pick to v5.0.x? |
Yes, I'm working on a fix for the issue brought up, I'll submit a pull request and a cherry-pick once it's done. |
v5.0.x fix: #10077 |
Closes #9554
FYI @open-mpi/ompi-gatekeeper-5-0-x