Skip to content
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

Don't call the delete callback on MPI_COMM_WORLD attributes. #12068

Merged

Conversation

bosilca
Copy link
Member

@bosilca bosilca commented Nov 13, 2023

Fixes #12035.

@jsquyres
Copy link
Member

For an MPI app that caches no attributes on MPI_COMM_WORLD, deleting this code doesn't orphan or leak any memory during finalization, right?

@bosilca
Copy link
Member Author

bosilca commented Nov 13, 2023

Correct. Any attributes attached to MPI_COMM_WORLD once MPI_Finalize is called will leak.

@jsquyres
Copy link
Member

Correct. Any attributes attached to MPI_COMM_WORLD once MPI_Finalize is called will leak.

That's not quite what I'm asking.

I notice that you're deleting an OBJ_RELEASE(ompi_mpi_comm_world.comm.c_keyhash). Is that memory still freed properly during the OBJ_DESTRUCT() of COMM_WORLD? E.g., did you test running with and without this PR to verify that the amount of orphaned / leaked memory is the same? (I'm guessing that we're not overall Valgrind clean, so I'm just asking to make sure that we're not creating a new leak by deleting this code)

@bosilca bosilca force-pushed the fix/attribute_deletion_during_finalize branch from 725c657 to cb06360 Compare November 13, 2023 22:03
@bosilca
Copy link
Member Author

bosilca commented Nov 13, 2023

Thanks @jsquyres. Based on your comment I realized that the code I remove was in fact partially useful, as it had the side effect of removing the predefined attributes we were attaching to MPI_COMM_WORLD. Unfortunately, it had the side effect of also deleting user-defined attributes, leading to the issue #12035. Please recheck.

During MPI_Finalize delete all predefined attributes from
MPI_COMM_WORLD, before deleting the attributes placeholder. Note that
user-defined attributes that were still attached to MPI_COMM_WORLD will
leak.

Fixes open-mpi#12035.

Signed-off-by: George Bosilca <bosilca@icl.utk.edu>
@bosilca bosilca force-pushed the fix/attribute_deletion_during_finalize branch from cb06360 to b79004e Compare November 13, 2023 22:06
@jsquyres jsquyres merged commit 4984355 into open-mpi:main Nov 15, 2023
10 checks passed
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.

OMPI-5.0 error in MPI_Finalize() with user's comm_delete_attr_fn calling MPI
3 participants