mpi-forum / mpi-issues

Tickets for the MPI Forum
http://www.mpi-forum.org/
67 stars 8 forks source link

Users must always free (persistent request handles for operation) objects that they create #732

Open Wee-Free-Scot opened 1 year ago

Wee-Free-Scot commented 1 year ago

Problem

The existing requirement regarding (not) freeing inactive persistent requests before MPI_COMM_DISCCONNECT (and/or MPI_FINALIZE and/or MPI_SESSION_FINALIZE) was clarified in MPI-4.1 by issue https://github.com/mpi-forum/mpi-issues/issues/710 (PR https://github.com/mpi-forum/mpi-standard/pull/823).

However, there was significant discussion on that issue and the associated PR. One point that was made repeatedly claims that it would be better to require users to free all request handles before attempting to destroy the communicator used by the MPI operation they represent. There are two forks to this claim: 1) it would improve the implementation of MPI -- to release system resources held by inactive persistent operations, the MPI library must maintain a list of persistent requests, which can be used when destroying the associated communicator. The discussion also surfaced concerns about how to maintain a usable ref-count for the communicator without adding overhead to critical-path communication functions. 2) it would improve user code that uses MPI -- good coding practice requires a robust mechanism to free resources when they are no longer needed; typically requiring the user to explicitly indicate when the resource allocation life-cycle should end maps well to the user explicitly indicating when the resource allocation life-cycle should begin.

Proposal

Require that the user must free all requests (including persistent requests) for a communicator before calling MPI_COMM_DISCONNECT for that communicator. Require that the user must free all requests (including persistent requests) for all communicators derived from a session before calling MPI_SESSION_FINALIZE for that session. Require that the user must free all requests (including persistent requests) for all communicators derived from the World Model before calling MPI_FINALIZE.

This change is backwards-incompatible (because the presence/absence of this requirement was omitted before MPI-4.1 and its de facto absence was confirmed in MPI-4.1).

Changes to the Text

TBC

Impact on Implementations

Joy for some; meh for others?

Impact on Users

User codes that use persistent MPI operations but do not free all persistent requests in accordance with the new requirement become erroneous -- requiring modifications to user codes for compatibility with future versions of MPI.

References and Pull Requests

MPI-4.1 issue that confirms the absence of this proposed requirement: https://github.com/mpi-forum/mpi-issues/issues/710 MPI-4.1 pull request that implements the MPI-4.1 issue: https://github.com/mpi-forum/mpi-standard/pull/823

Fixed by: none yet.

hzhou commented 1 year ago

Pasting my comments from #710 -

I am very surprised that one of the conclusions of this issue is -- users do not need free a persistent request that they explicitly created. This is just odd with no precedence. I am a bit upset that this gets sneaked in as errata. It is one thing that the previous text neglected to mention the requirement, but it is another thing to issue an errata to clarify that the neglect was intentional.

I guess I am ahead of myself -- Is my interpretation of the errata correct? And what is the logic that allows explicit MPI object creation without corresponding explicit free/destroy?

@RolfRabenseifner

bosilca commented 1 year ago

Hardly anything was sneaked in, we wasted a lot of cycles thoroughly discussing this over (with most/all implementors against this change). Check https://github.com/mpi-forum/mpi-standard/pull/823 for the different arguments brought to the discussion.

hppritcha commented 1 year ago

I was following up on an item from the sessions WG meeting this past Monday. Namely I was going to check the behavior of Open MPI vs MPICH for the cases of not freeing a persistent request prior to calling MPI_Comm_disconnect and also MPI_Finalize. I thought it had been stated that Open MPI was observed to hang in MPI_Comm_disconnect if persistent requests associated with that communicator had not been freed. I can't seem to reproduce that behavior with Open MPI 4.1.5 using the OB1 PML. I also don't observe a hang in MPI_Finalize using this version/config of Open MPI when there are unfreed persistent requests.

hzhou commented 1 year ago

No, MPICH will hang, but Open MPI will just go ahead and leak the handle. We have a PR to fix the hang (https://github.com/pmodels/mpich/pull/6591).

But regardless of whether an MPI library can do automatic garbage collection, I don't think the MPI standard should encourage users not free persistent objects.

hppritcha commented 1 year ago

okay i got it backwards then. thanks for the clarification @hzhou

RolfRabenseifner commented 1 year ago

@hzhou wrote:

But regardless of whether an MPI library can do automatic garbage collection, I don't think the MPI standard should encourage users not free persistent objects.

I fully agree. Therefore, MPI-4.1 has in the description of MPI_START a corresponding AtoU to encourage users to free the persistent objects, see mpi-report-cdec307e_PR855_v03.pdf, page 102, lines 37-44:

Advice to users. Persistent request handles may bind internal resources such as MPI buffers in shared memory for providing efficient communication. Therefore, it is highly recommended to explicitly free inactive request handles, using MPI_REQUEST_FREE, when they are no longer in use, and in particular before freeing or disconnecting the associated communicator with MPI_COMM_FREE or MPI_COMM_DISCONNECT or finalizing the associated session with MPI_SESSIONFINALIZE. (End of advice to users.)_

hzhou commented 1 year ago

@RolfRabenseifner The AtoU made it clear that it is okay to not free inactive request handles -- "highly recommended" means it is okay not to. Since it only cites the reason being holding resources, I interpret it as encouraging users to not free the handles when they are not worrying about the resources. Most applications don't worry about resources after they are done with the resource-intensive regions such as when they are about to call MPI_Finalize.

Also, the rationale does not make sense. After MPI_Comm_free or MPI_Comm_disconnect, an inactive persistent request can no longer be used, why would it still hold internal resources? A high-quality implementation should always garbage collect the resource and the inactive request handles as well. I guess then, this AtoU is essentially saying, "be careful and watch out for low-quality implementations that may leak resources", which is a very odd AtoU.

So, IMO, the standard should either explicitly require implementation to implement garbage collection at MPI_Comm_disconnect/MPI_Session_finalize/MPI_Finalize, thus removing the above AtoU, or require users to free inactive persistent requests before MPI_Comm_disconnect/MPI_Session_finalize/MPI_Finalize, and remove above AtoU. The latter IMO is consistent in MPI. A high-quality implementation may still do garbage collection and provide graceful behaviors when user make negligence, but that is beyond the standard text.