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.

Sign up for GitHub

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

Fix error handling for generalized requests #12392

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix error handling for generalized requests #12392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59c2af8
request: Fix multiple calls to ompi_request_complete()
dalcinl Mar 5, 2024
16c6381
request: Improve MPI_Request_free() error handling
dalcinl Mar 5, 2024
adb640d
grequest: Fix MPI_Request_get_status() to propagate errors form query_fn
dalcinl Mar 5, 2024
40a500d
grequest: Fix propagation of errors from callback functions.
dalcinl Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ompi/mpi/c/request_free.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ int MPI_Request_free(MPI_Request *request)
}

rc = ompi_request_free(request);
if (OPAL_UNLIKELY(OMPI_SUCCESS != rc && MPI_REQUEST_NULL != *request)) {
(*request)->req_status.MPI_ERROR = rc;
Copy link
Member

Choose a reason for hiding this comment

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

Did ompi_request_free(request) already set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the generic user-level MPI_Request_free() for all kinds of requests, not only generalized requests. I'm not so expert with the Open MPI implementation to 100% sure ompi_request_free() will set the status error field on errors. Without that guarantee, my use of ompi_errhandler_request_invoke() may not work as I intended. Note that ompi_errhandler_request_invoke() has the side-effect of cleaning up the request and deallocating the object. Sorry, it's a bit complicated, but I had to work within the constraints of what I was given.

For example, look at ompi_comm_request_free()... I don't see that routine setting the status error field. Therefore, I think being overzealous here is justified. IMHO, this line is a good one and should stay. Otherwise, all internal request types used in Open MPI (and future ones that may be implemented) should be audited and eventually fixed to make sure a failing free sets the error field.

return ompi_errhandler_request_invoke(1, request, FUNC_NAME);
}
OMPI_ERRHANDLER_NOHANDLE_RETURN(rc, rc, FUNC_NAME);
}

5 changes: 3 additions & 2 deletions ompi/mpi/c/request_get_status.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,18 @@ int MPI_Request_get_status(MPI_Request request, int *flag,
return MPI_SUCCESS;
}
if( request->req_complete ) {
int rc = MPI_SUCCESS;
*flag = true;
/* If this is a generalized request, we *always* have to call
the query function to get the status (MPI-2:8.2), even if
the user passed STATUS_IGNORE. */
if (OMPI_REQUEST_GEN == request->req_type) {
ompi_grequest_invoke_query(request, &request->req_status);
rc = ompi_grequest_invoke_query(request, &request->req_status);
}
if (MPI_STATUS_IGNORE != status) {
OMPI_COPY_STATUS(status, request->req_status, false);
}
return MPI_SUCCESS;
return rc;
}
#if OPAL_ENABLE_PROGRESS_THREADS == 0
if( 0 == do_it_once ) {
Expand Down
69 changes: 46 additions & 23 deletions ompi/request/grequest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,33 +24,55 @@
#include "ompi/request/grequest.h"
#include "ompi/mpi/fortran/base/fint_2_int.h"

/**
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change (since it's not relevant)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? I'm fixing a inconsistency in the comment style respect to the other block comments in the same source file. Am I supposed to submit a separate PR for such trivial fixes? Or should these inconsistencies left alone to live forever?

* Internal function to specialize the call to the user provided free_fn
* for generalized requests.
* @return The return value of the user specified callback or MPI_SUCCESS.
*/
static inline int ompi_grequest_internal_free(ompi_grequest_t* greq)
static inline int ompi_grequest_invoke_free(ompi_grequest_t* greq)
{
int rc = MPI_SUCCESS;
int rc = OMPI_SUCCESS;
MPI_Fint ierr;

if (NULL != greq->greq_free.c_free) {
if (greq->greq_funcs_are_c) {
rc = greq->greq_free.c_free(greq->greq_state);
} else {
greq->greq_free.f_free((MPI_Aint*)greq->greq_state, &ierr);
rc = OMPI_FINT_2_INT(ierr);
}
/* We were already putting query_fn()'s return value into
* status.MPI_ERROR but for MPI_{Wait,Test}*. If there's a
* free callback to invoke, the standard says to use the
* return value from free_fn() callback, too.
*/
if (greq->greq_funcs_are_c) {
greq->greq_base.req_status.MPI_ERROR =
greq->greq_free.c_free(greq->greq_state);
} else {
MPI_Fint ierr;
greq->greq_free.f_free((MPI_Aint*)greq->greq_state, &ierr);
greq->greq_base.req_status.MPI_ERROR = OMPI_FINT_2_INT(ierr);
if (OMPI_SUCCESS != rc) {
greq->greq_base.req_status.MPI_ERROR = rc;
}
rc = greq->greq_base.req_status.MPI_ERROR;
}
return rc;
}


/*
* Internal function to dispatch the call to the user provided free_fn
* for generalized requests. The freeing code executes as soon as both
* wait/free and complete have occured.
* @return The return value of the user specified callback or MPI_SUCCESS.
*/
static inline int ompi_grequest_internal_free(ompi_grequest_t* greq)
{
int rc = OMPI_SUCCESS;

if (REQUEST_COMPLETE(&greq->greq_base) && greq->greq_user_freed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we only invoke free if the user called MPI_Request_free (i.e., from ompi_grequest_free)? Shouldn't free be invoked on completion (i.e., from ompi_grequest_complete) as well?

Copy link
Contributor Author

@dalcinl dalcinl Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@devreal Please read the large comment below this function, startin with MPI has some weird semantics with respect to generalized requests .... And no, completion should not trigger free by itself, completion is sort of flagging the thing as done, such that the progress engine running in another thread knows that the thing is completed. What makes things convoluted is that a user could Free without a more usual Wait or Test call, therefore extra care is needed.

rc = ompi_grequest_invoke_free(greq);
/* The free_fn() callback should be invoked only once. */
if (NULL != greq->greq_free.c_free)
greq->greq_free.c_free = NULL;
}
return rc;
}

/*
* See the comment in the grequest destructor for the weird semantics
* here. If the request has been marked complete via a call to
Expand All @@ -66,14 +88,10 @@ static int ompi_grequest_free(ompi_request_t** req)
ompi_grequest_t* greq = (ompi_grequest_t*)*req;
int rc = OMPI_SUCCESS;

if( greq->greq_user_freed ) {
return OMPI_ERR_OUT_OF_RESOURCE;
}
greq->greq_user_freed = true;
if( REQUEST_COMPLETE(*req) ) {
rc = ompi_grequest_internal_free(greq);
}
if (OMPI_SUCCESS == rc ) {
rc = ompi_grequest_internal_free(greq);

if (OMPI_SUCCESS == rc) {
OBJ_RELEASE(*req);
*req = MPI_REQUEST_NULL;
}
Expand Down Expand Up @@ -180,7 +198,7 @@ int ompi_grequest_start(
ompi_request_t** request)
{
ompi_grequest_t *greq = OBJ_NEW(ompi_grequest_t);
if(greq == NULL) {
if (greq == NULL) {
return OMPI_ERR_OUT_OF_RESOURCE;
}
/* We call RETAIN here specifically to increase the refcount to 2.
Expand Down Expand Up @@ -211,13 +229,14 @@ int ompi_grequest_start(
int ompi_grequest_complete(ompi_request_t *req)
{
ompi_grequest_t* greq = (ompi_grequest_t*)req;
bool greq_release = !REQUEST_COMPLETE(req);
int rc;

rc = ompi_request_complete(req, true);
if( greq->greq_user_freed ) {
if (OMPI_SUCCESS == rc && greq_release) {
rc = ompi_grequest_internal_free(greq);
OBJ_RELEASE(req);
}
OBJ_RELEASE(req);
return rc;
}

Expand All @@ -237,6 +256,11 @@ int ompi_grequest_invoke_query(ompi_request_t *request,
int rc = OMPI_SUCCESS;
ompi_grequest_t *g = (ompi_grequest_t*) request;

/* MPI mandates that query_fn must be called after the request is
* completed. Make sure the caller does not break the contract.
*/
assert( REQUEST_COMPLETE(request) );

/* MPI-3 mandates that the return value from the query function
* (i.e., the int return value from the C function or the ierr
* argument from the Fortran function) must be returned to the
Expand Down Expand Up @@ -268,9 +292,8 @@ int ompi_grequest_invoke_query(ompi_request_t *request,
rc = OMPI_FINT_2_INT(ierr);
}
}
if( MPI_SUCCESS != rc ) {
if (OMPI_SUCCESS != rc) {
status->MPI_ERROR = rc;
}
return rc;
}

2 changes: 1 addition & 1 deletion ompi/request/request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ static inline int ompi_request_complete(ompi_request_t* request, bool with_signa

ompi_wait_sync_t *tmp_sync = (ompi_wait_sync_t *) OPAL_ATOMIC_SWAP_PTR(&request->req_complete,
REQUEST_COMPLETED);
if( REQUEST_PENDING != tmp_sync ) {
if (REQUEST_PENDING != tmp_sync && REQUEST_COMPLETED != tmp_sync) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that a core premise of ompi_request_complete() is that it should only be invoked once for a given request. It wasn't intended to be safe to be called multiple times -- e.g., the first time, it actually completes the request, and subsequent times it effectively does nothing.

Copy link
Contributor Author

@dalcinl dalcinl Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ompi_request_complete() called a second time should either fail cleanly returning error, or just happily continue as a non-op because the request already completed, but it should NEVER segfault as would happen currently if the user call MPI_Request_complete() two times. The MPI standard does not say whether the routine can be called multiple times on the same request handle. With this fix, I'm just playing safe and allowing multiple calls to MPI_Request_complete(). For reference, IIRC, MPICH errors not the second but the third time, this is something I was planning to report and/or eventually fix.

IMHO, it is probably easier to just allow multiple MPI_Request_complete() calls on a request, just like I'm doing here. If you do not like such semantics and prefer to error, I believe the fix is trivial.

wait_sync_update(tmp_sync, 1, request->req_status.MPI_ERROR);
}
} else {
Expand Down
Loading