From c7282855e77c2651d958e734e1949adf9c2fe80b Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Sat, 9 Dec 2006 14:20:08 +0000 Subject: [PATCH] Fixes trac:659 This commit fixes several aspects regarding MPI conformance of requests. * Eliminate the last argument of ompi_errhandler_request_invoke(); we ''always'' want to invoke the back-end exception handler with the real error code. * Make it clear in comments that we only invoke the ''first'' exception in a given array of requests, even if there's more than one request with a non-MPI_SUCCESS value for MPI_ERROR. * Defer the freeing of requests upon exception in the back-end functions to MPI_WAIT* and MPI_TEST* until later; the requests are kept so that we know what handler to invoke when we actually invoke the exception. After figuring that out, ''then'' we free requests with pending exceptions on them. * Clean up return codes from the back-end MPI_TEST* and MPI_WAIT* functions. * Slightly modify ompi_errcode_get_mpi_code() to return unity if it receives an MPI error code (vs. an OMPI error code). This commit was SVN r12810. The following Trac tickets were found above: Ticket 659 --> https://svn.open-mpi.org/trac/ompi/ticket/659 --- ompi/errhandler/errcode-internal.h | 8 +++ ompi/errhandler/errhandler.h | 17 +----- ompi/errhandler/errhandler_invoke.c | 38 ++++++++----- ompi/mpi/c/test.c | 2 +- ompi/mpi/c/testall.c | 2 +- ompi/mpi/c/testany.c | 2 +- ompi/mpi/c/testsome.c | 2 +- ompi/mpi/c/wait.c | 2 +- ompi/mpi/c/waitall.c | 2 +- ompi/mpi/c/waitany.c | 2 +- ompi/mpi/c/waitsome.c | 2 +- ompi/request/req_test.c | 86 +++++++++++++++++------------ ompi/request/req_wait.c | 74 ++++++++++++++----------- 13 files changed, 134 insertions(+), 105 deletions(-) diff --git a/ompi/errhandler/errcode-internal.h b/ompi/errhandler/errcode-internal.h index 7bed285aec..906708239c 100644 --- a/ompi/errhandler/errcode-internal.h +++ b/ompi/errhandler/errcode-internal.h @@ -55,6 +55,14 @@ static inline int ompi_errcode_get_mpi_code(int errcode) int __i; ompi_errcode_intern_t *__errc; + /* If the errcode is >= 0, then it's already an MPI error code, so + just return it. */ + if (errcode >= 0) { + return errcode; + } + + /* Otherwise, it's an internal OMPI code and we need to translate + it */ for ( __i=0; __icode == errcode ) { diff --git a/ompi/errhandler/errhandler.h b/ompi/errhandler/errhandler.h index a94ff423d9..faf267350e 100644 --- a/ompi/errhandler/errhandler.h +++ b/ompi/errhandler/errhandler.h @@ -268,25 +268,10 @@ struct ompi_request_t; * that has a non-MPI_SUCCESS value for MPI_ERROR in its status. It * is safe to invoke this function if none of the requests have an * outstanding error; MPI_SUCCESS will be returned. - * - * If use_actual_err_code is true and at least one of the requests - * has a non-MPI_SUCCESS error code, the MPI exception will be - * invoked with that error code (to include MPI_ERRORS_RETURN, - * meaning that the actual error code will be returned out of the - * top-level MPI API function). If use_actual_err_code is false and - * at least one of the requests has a non-MPI_SUCCESS error code, - * the MPI exception will be invoked with MPI_ERR_IN_STATUS. - * - * It is expected that per MPI-1, MPI_TEST, MPI_TESTANY, MPI_WAIT, - * and MPI_WAITANY will invoke this function with - * (use_actual_err_code = true), and MPI_TESTALL, MPI_TESTSOME, - * MPI_WAITALL, and MPI_WAITSOME will invoke this function with - * (use_actual_err_code = false). */ int ompi_errhandler_request_invoke(int count, struct ompi_request_t **requests, - const char *message, - bool use_actual_err_code); + const char *message); /** * Create a ompi_errhandler_t diff --git a/ompi/errhandler/errhandler_invoke.c b/ompi/errhandler/errhandler_invoke.c index e37ba452cc..0b91172964 100644 --- a/ompi/errhandler/errhandler_invoke.c +++ b/ompi/errhandler/errhandler_invoke.c @@ -82,16 +82,16 @@ int ompi_errhandler_invoke(ompi_errhandler_t *errhandler, void *mpi_object, int ompi_errhandler_request_invoke(int count, struct ompi_request_t **requests, - const char *message, - bool use_actual_err_code) + const char *message) { - int i, ec; + int i, ec, type; ompi_mpi_object_t mpi_object; - /* Find the first request that has an error. In an error - condition, the request will not have been reset back to - MPI_REQUEST_NULL, so there's no need to cache values from - before we call ompi_request_test(). */ + /* Find the *first* request that has an error -- that's the one + that we'll invoke the exception on. In an error condition, the + request will not have been reset back to MPI_REQUEST_NULL, so + there's no need to cache values from before we call + ompi_request_test(). */ for (i = 0; i < count; ++i) { if (MPI_REQUEST_NULL != requests[i] && MPI_SUCCESS != requests[i]->req_status.MPI_ERROR) { @@ -103,13 +103,25 @@ int ompi_errhandler_request_invoke(int count, return MPI_SUCCESS; } - if (use_actual_err_code) { - ec = ompi_errcode_get_mpi_code(requests[i]->req_status.MPI_ERROR); - } else { - ec = MPI_ERR_IN_STATUS; - } + ec = ompi_errcode_get_mpi_code(requests[i]->req_status.MPI_ERROR); mpi_object = requests[i]->req_mpi_object; - switch (requests[i]->req_type) { + type = requests[i]->req_type; + + /* Since errors on requests cause them to not be freed (until we + can examine them here), go through and free all requests with + errors. We only invoke the exception on the *first* request + that had an error. */ + for (; i < count; ++i) { + if (MPI_REQUEST_NULL != requests[i] && + MPI_SUCCESS != requests[i]->req_status.MPI_ERROR) { + /* Ignore the error -- what are we going to do? We're + already going to invoke an exception */ + ompi_request_free(&(requests[i])); + } + } + + /* Invoke the exception */ + switch (type) { case OMPI_REQUEST_PML: return ompi_errhandler_invoke(mpi_object.comm->error_handler, mpi_object.comm, diff --git a/ompi/mpi/c/test.c b/ompi/mpi/c/test.c index 1a598c6032..6e8503cfbc 100644 --- a/ompi/mpi/c/test.c +++ b/ompi/mpi/c/test.c @@ -57,5 +57,5 @@ int MPI_Test(MPI_Request *request, int *completed, MPI_Status *status) if (OMPI_SUCCESS == rc) { return MPI_SUCCESS; } - return ompi_errhandler_request_invoke(1, request, FUNC_NAME, true); + return ompi_errhandler_request_invoke(1, request, FUNC_NAME); } diff --git a/ompi/mpi/c/testall.c b/ompi/mpi/c/testall.c index b826e63b30..a64d9ae0ff 100644 --- a/ompi/mpi/c/testall.c +++ b/ompi/mpi/c/testall.c @@ -51,7 +51,7 @@ int MPI_Testall(int count, MPI_Request requests[], int *flag, return MPI_SUCCESS; } if (MPI_SUCCESS != - ompi_errhandler_request_invoke(count, requests, FUNC_NAME, false)) { + ompi_errhandler_request_invoke(count, requests, FUNC_NAME)) { return MPI_ERR_IN_STATUS; } return MPI_SUCCESS; diff --git a/ompi/mpi/c/testany.c b/ompi/mpi/c/testany.c index 8a154ae993..d833367e28 100644 --- a/ompi/mpi/c/testany.c +++ b/ompi/mpi/c/testany.c @@ -53,5 +53,5 @@ int MPI_Testany(int count, MPI_Request requests[], int *index, int *completed, M index, completed, status)) { return MPI_SUCCESS; } - return ompi_errhandler_request_invoke(count, requests, FUNC_NAME, true); + return ompi_errhandler_request_invoke(count, requests, FUNC_NAME); } diff --git a/ompi/mpi/c/testsome.c b/ompi/mpi/c/testsome.c index c7361fa3f5..333bc5466a 100644 --- a/ompi/mpi/c/testsome.c +++ b/ompi/mpi/c/testsome.c @@ -56,7 +56,7 @@ int MPI_Testsome(int incount, MPI_Request *requests, return MPI_SUCCESS; } if (MPI_SUCCESS != - ompi_errhandler_request_invoke(incount, requests, FUNC_NAME, false)) { + ompi_errhandler_request_invoke(incount, requests, FUNC_NAME)) { return MPI_ERR_IN_STATUS; } return MPI_SUCCESS; diff --git a/ompi/mpi/c/wait.c b/ompi/mpi/c/wait.c index 724268c206..bddeb1a706 100644 --- a/ompi/mpi/c/wait.c +++ b/ompi/mpi/c/wait.c @@ -59,5 +59,5 @@ int MPI_Wait(MPI_Request *request, MPI_Status *status) if (OMPI_SUCCESS == ompi_request_wait(request, status)) { return MPI_SUCCESS; } - return ompi_errhandler_request_invoke(1, request, FUNC_NAME, true); + return ompi_errhandler_request_invoke(1, request, FUNC_NAME); } diff --git a/ompi/mpi/c/waitall.c b/ompi/mpi/c/waitall.c index aa768d1cb0..9cd4b3ec18 100644 --- a/ompi/mpi/c/waitall.c +++ b/ompi/mpi/c/waitall.c @@ -54,7 +54,7 @@ int MPI_Waitall(int count, MPI_Request *requests, MPI_Status *statuses) return MPI_SUCCESS; } if (MPI_SUCCESS != - ompi_errhandler_request_invoke(count, requests, FUNC_NAME, false)) { + ompi_errhandler_request_invoke(count, requests, FUNC_NAME)) { return MPI_ERR_IN_STATUS; } return MPI_SUCCESS; diff --git a/ompi/mpi/c/waitany.c b/ompi/mpi/c/waitany.c index f93e50e336..bfd68acde0 100644 --- a/ompi/mpi/c/waitany.c +++ b/ompi/mpi/c/waitany.c @@ -54,5 +54,5 @@ int MPI_Waitany(int count, MPI_Request *requests, int *index, MPI_Status *status if (OMPI_SUCCESS == ompi_request_wait_any(count, requests, index, status)) { return MPI_SUCCESS; } - return ompi_errhandler_request_invoke(count, requests, FUNC_NAME, true); + return ompi_errhandler_request_invoke(count, requests, FUNC_NAME); } diff --git a/ompi/mpi/c/waitsome.c b/ompi/mpi/c/waitsome.c index b5fb0e9573..5a7ee4af5b 100644 --- a/ompi/mpi/c/waitsome.c +++ b/ompi/mpi/c/waitsome.c @@ -57,7 +57,7 @@ int MPI_Waitsome(int incount, MPI_Request *requests, return MPI_SUCCESS; } if (MPI_SUCCESS != - ompi_errhandler_request_invoke(incount, requests, FUNC_NAME, false)) { + ompi_errhandler_request_invoke(incount, requests, FUNC_NAME)) { return MPI_ERR_IN_STATUS; } return MPI_SUCCESS; diff --git a/ompi/request/req_test.c b/ompi/request/req_test.c index 80d58e5789..60b9adb236 100644 --- a/ompi/request/req_test.c +++ b/ompi/request/req_test.c @@ -27,7 +27,6 @@ int ompi_request_test( ompi_request_t ** rptr, int *completed, ompi_status_public_t * status ) { - int rc; ompi_request_t *request = *rptr; #if OMPI_ENABLE_PROGRESS_THREADS == 0 int do_it_once = 0; @@ -64,12 +63,15 @@ int ompi_request_test( ompi_request_t ** rptr, request->req_state = OMPI_REQUEST_INACTIVE; return request->req_status.MPI_ERROR; } - rc = request->req_status.MPI_ERROR; - if (OMPI_SUCCESS != ompi_request_free(rptr)) { - return OMPI_ERROR; - } else { - return rc; + /* If there was an error, don't free the request -- just + return the single error. */ + if (MPI_SUCCESS != request->req_status.MPI_ERROR) { + return request->req_status.MPI_ERROR; } + /* If there's an error on the request, assume that the request + is still there. Otherwise, Bad Things will happen + later! */ + return ompi_request_free(rptr); } #if OMPI_ENABLE_PROGRESS_THREADS == 0 if( 0 == do_it_once ) { @@ -97,7 +99,6 @@ int ompi_request_test_any( size_t num_requests_null_inactive = 0; ompi_request_t **rptr; ompi_request_t *request; - int rc; opal_atomic_mb(); rptr = requests; @@ -134,11 +135,14 @@ int ompi_request_test_any( request->req_state = OMPI_REQUEST_INACTIVE; return OMPI_SUCCESS; } - rc = request->req_status.MPI_ERROR; - if (OMPI_SUCCESS != ompi_request_free(rptr)) { - return OMPI_ERROR; + /* If there is an error on the request, don't free it */ + if (MPI_SUCCESS != request->req_status.MPI_ERROR) { + return request->req_status.MPI_ERROR; } - return rc; + /* If there's an error while freeing the request, assume + that the request is still there. Otherwise, Bad Things + will happen later! */ + return ompi_request_free(rptr); } } @@ -165,7 +169,7 @@ int ompi_request_test_all( int *completed, ompi_status_public_t * statuses) { - size_t i; + size_t i, rc; ompi_request_t **rptr; size_t num_completed = 0; ompi_request_t *request; @@ -190,10 +194,11 @@ int ompi_request_test_all( rptr = requests; *completed = true; + + rc = MPI_SUCCESS; if (MPI_STATUSES_IGNORE != statuses) { /* fill out completion status and free request if required */ for( i = 0; i < count; i++, rptr++ ) { - int rc; request = *rptr; if( request->req_state == OMPI_REQUEST_INACTIVE ) { statuses[i] = ompi_status_empty; @@ -207,14 +212,21 @@ int ompi_request_test_all( request->req_state = OMPI_REQUEST_INACTIVE; continue; } - rc = ompi_request_free(rptr); - if(rc != OMPI_SUCCESS) - return rc; + /* MPI-2:4.5.1 says that we can return MPI_ERR_IN_STATUS + even if MPI_STATUSES_IGNORE was used. Woot! */ + /* Only free the request if there was no error on it */ + if (MPI_SUCCESS == request->req_status.MPI_ERROR) { + int tmp = ompi_request_free(rptr); + if (tmp != OMPI_SUCCESS) { + return tmp; + } + } else { + rc = MPI_ERR_IN_STATUS; + } } } else { /* free request if required */ for( i = 0; i < count; i++, rptr++ ) { - int rc; request = *rptr; if( request->req_state == OMPI_REQUEST_INACTIVE) { continue; @@ -229,12 +241,19 @@ int ompi_request_test_all( request->req_state = OMPI_REQUEST_INACTIVE; continue; } - rc = ompi_request_free(rptr); - if(rc != OMPI_SUCCESS) - return rc; + /* Only free the request if there was no error */ + if (MPI_SUCCESS == request->req_status.MPI_ERROR) { + int tmp = ompi_request_free(rptr); + if (tmp != OMPI_SUCCESS) { + return tmp; + } + } else { + rc = MPI_ERR_IN_STATUS; + } } } - return OMPI_SUCCESS; + + return rc; } @@ -291,28 +310,23 @@ int ompi_request_test_some( statuses[i] = request->req_status; } - rc += request->req_status.MPI_ERROR; + if (MPI_SUCCESS != request->req_status.MPI_ERROR) { + rc = MPI_ERR_IN_STATUS; + } if( request->req_persistent ) { request->req_state = OMPI_REQUEST_INACTIVE; } else { - int tmp; - /* return request to pool */ - tmp = ompi_request_free(&(requests[indices[i]])); - /* - * If it fails, we are screwed. We cannot put the - * request_free return code into the status, possibly - * overwriting some other important error; therefore just quit. - */ - if (OMPI_SUCCESS != tmp) { - return tmp; + /* Only free the request if there was no error */ + if (MPI_SUCCESS == request->req_status.MPI_ERROR) { + int tmp; + tmp = ompi_request_free(&(requests[indices[i]])); + if (OMPI_SUCCESS != tmp) { + return tmp; + } } } } - if (OMPI_SUCCESS != rc) { - rc = MPI_ERR_IN_STATUS; - } - return rc; } diff --git a/ompi/request/req_wait.c b/ompi/request/req_wait.c index 179d8b72f0..a70f86d87f 100644 --- a/ompi/request/req_wait.c +++ b/ompi/request/req_wait.c @@ -27,7 +27,6 @@ int ompi_request_wait( ompi_status_public_t * status) { ompi_request_t *req = *req_ptr; - int rc; if(req->req_complete == false) { @@ -72,12 +71,15 @@ finished: return req->req_status.MPI_ERROR; } - /* return request to pool */ - rc = req->req_status.MPI_ERROR; - if (OMPI_SUCCESS != ompi_request_free(req_ptr)) { - return OMPI_ERROR; + /* If there was an error, don't free the request -- just return + the single error. */ + if (MPI_SUCCESS != req->req_status.MPI_ERROR) { + return req->req_status.MPI_ERROR; } - return rc; + /* If there's an error while freeing the request, assume that the + request is still there. Otherwise, Bad Things will happen + later! */ + return ompi_request_free(req_ptr); } @@ -179,16 +181,15 @@ finished: *status = request->req_status; status->MPI_ERROR = old_error; } + rc = request->req_status.MPI_ERROR; if( request->req_persistent ) { request->req_state = OMPI_REQUEST_INACTIVE; - rc = request->req_status.MPI_ERROR; - } else { - int tmp; - - /* return request to pool */ - rc = request->req_status.MPI_ERROR; - tmp = ompi_request_free(rptr); - if (OMPI_SUCCESS != tmp) rc = tmp; + } else if (MPI_SUCCESS == rc) { + /* Only free the request if there is no error on it */ + /* If there's an error while freeing the request, + assume that the request is still there. Otherwise, + Bad Things will happen later! */ + rc = ompi_request_free(rptr); } *index = completed; } @@ -282,7 +283,16 @@ int ompi_request_wait_all( if( request->req_persistent ) { request->req_state = OMPI_REQUEST_INACTIVE; } else { - (void)ompi_request_free(rptr); + /* Only free the request if there is no error on it */ + if (MPI_SUCCESS == request->req_status.MPI_ERROR) { + /* If there's an error while freeing the request, + assume that the request is still there. + Otherwise, Bad Things will happen later! */ + int tmp = ompi_request_free(rptr); + if (OMPI_SUCCESS != tmp) { + mpi_error = tmp; + } + } } if( statuses[i].MPI_ERROR != OMPI_SUCCESS) { mpi_error = MPI_ERR_IN_STATUS; @@ -307,8 +317,12 @@ int ompi_request_wait_all( } if( request->req_persistent ) { request->req_state = OMPI_REQUEST_INACTIVE; - } else { - (void)ompi_request_free(rptr); + } else if (MPI_SUCCESS == rc) { + /* Only free the request if there is no error on it */ + int tmp = ompi_request_free(rptr); + if (OMPI_SUCCESS != tmp) { + mpi_error = tmp; + } } if( rc != OMPI_SUCCESS) { mpi_error = rc; @@ -330,7 +344,7 @@ int ompi_request_wait_some( int c; #endif size_t i, num_requests_null_inactive=0, num_requests_done=0; - int rc = OMPI_SUCCESS; + int rc = MPI_SUCCESS; ompi_request_t **rptr=NULL; ompi_request_t *request=NULL; @@ -435,27 +449,23 @@ finished: statuses[i] = request->req_status; } - rc += request->req_status.MPI_ERROR; + if (MPI_SUCCESS != request->req_status.MPI_ERROR) { + rc = MPI_ERR_IN_STATUS; + } if( request->req_persistent ) { request->req_state = OMPI_REQUEST_INACTIVE; } else { - int tmp; - /* return request to pool */ - tmp = ompi_request_free(&(requests[indices[i]])); - /* - * If it fails, we are screwed. We cannot put the - * request_free return code into the status, possibly - * overwriting some other important error; therefore just quit. - */ - if (OMPI_SUCCESS != tmp) { - return tmp; + /* Only free the request if there was no error */ + if (MPI_SUCCESS == request->req_status.MPI_ERROR) { + int tmp; + tmp = ompi_request_free(&(requests[indices[i]])); + if (OMPI_SUCCESS != tmp) { + return tmp; + } } } } - if (OMPI_SUCCESS != rc) { - rc = MPI_ERR_IN_STATUS; - } } return rc; }