Thanks Josh. george.
On Mar 22, 2012, at 10:09 , Josh Hursey wrote: > Should be fixed in r26177. > > On Thu, Mar 22, 2012 at 7:51 AM, Josh Hursey <jjhur...@open-mpi.org> wrote: >> Fair enough. Upon further inspection of the request_invoke() handler, >> you are correct that it is not required here if we do not modify the >> default value for req_status.MPI_ERROR. >> >> I'll work on a revised patch this morning and commit. One that does >> not use this field. >> >> Per your comment from your first message: >> "The usage we're doing in the default_wait_all of MPI_ERR_PENDING is >> incorrect as well." >> I'm glad that you no longer think the patch is "incorrect", but just >> not implemented as well as it could be. >> >> Thanks, >> Josh >> >> On Thu, Mar 22, 2012 at 1:40 AM, George Bosilca <bosi...@eecs.utk.edu> wrote: >>> >>> On Mar 21, 2012, at 15:13 , Josh Hursey wrote: >>> >>>> I see your point about setting MPI_ERR_PENDING on the internal status >>>> versus the status returned by MPI_Waitall. As I mentioned, the reason >>>> I choose to do that is to support the ompi_errhandler_request_invoke() >>>> function. I could not think of an better way to fix this, so I'm open >>>> to ideas. >>> >>> If MPI_ERR_PENDING is not set on the request status then I don't see any >>> issues with the ompi_errhandler_request_invoke. Do I miss something obvious? >>> >>>> >>>> MPI_Waitall returns MPI_ERR_PENDING for 'not completed' requests (or >>>> sets the exposed status.MPI_ERROR field appropriately, depending on >>>> the completion function used). The user must still call a completion >>>> function to complete the request at which point the true error value >>>> is reported (MPI_SUCCESS, or something else). So I think we are on the >>>> same page here. >>>> >>>> Setting the request->req_status.MPI_ERROR to MPI_ERR_PENDING in >>>> MPI_Waitall does not prevent the OMPI system from overwriting that >>>> value with the correct error (e.g., MPI_ERR_TRUNCATE). When the OMPI >>>> system wants to set an error on the request it sets the value (without >>>> checking the previous value in req_status.MPI_ERROR) before calling >>>> the completion operation. We reset the value from MPI_ERR_PENDING to >>>> MPI_SUCCESS at the start of the test/wait operations so that the rest >>>> of the checks for (MPI_SUCCESS != req->req_status.MPI_ERROR) complete >>>> appropriately in those functions. >>>> >>>> So I am still failing to see why the patch is incorrect. Is it >>>> 'incorrect' or just 'not implemented the way you think it should be'? >>>> If the latter, then I'd love to see a patch. If the former, then I >>>> would really like to understand why. >>> >>> I didn't say it is incorrect, just that it is an overkill that is (1) not >>> required by the MPI standard; (2) potentially not thread safe; (3) >>> sub-optimal in terms of memory writes; (4) way to complex for what >>> MPI_ERR_PENDING is supposed to achieve. Moreover, I guess your patch is >>> indeed correct if the MPICH test you were using pass. >>> >>>> Do we both agree that before this patch Open MPI did not support >>>> MPI_ERR_PENDING? >>> >>> Who dares claim the opposite? >>> >>> george. >>> >>> >>>> >>>> -- Josh >>>> >>>> >>>> On Wed, Mar 21, 2012 at 2:45 PM, George Bosilca <bosi...@eecs.utk.edu> >>>> wrote: >>>>> My point was that MPI_ERR_PENDING should never be set on a specific >>>>> request. MPI_ERR_PENDING should only be returned in the array of statuses >>>>> attached to MPI_Waitall. Thus, there is no need to remove it from any >>>>> request. >>>>> >>>>> In addition, there is another reason why this is unnecessary (and I was >>>>> too lazy to describe it in my previous email). The MPI_Waitall is only >>>>> allowed to return MPI_ERR_PENDING for "not completed" MPI request. Such >>>>> requests will therefore be completed later by the MPI library, and at >>>>> that moment the error in the status will be set to the correct value, or >>>>> MPI_SUCCESS. >>>>> >>>>> george. >>>>> >>>>> On Mar 21, 2012, at 14:32 , Josh Hursey wrote: >>>>> >>>>>> In the patch for errhandler_invoke.c, you can see that we need to >>>>>> check for MPI_ERR_PENDING to make sure that we do not free the request >>>>>> when we are trying to decide if we should invoke the error handler. So >>>>>> setting the internal req->req_status.MPI_ERROR to MPI_ERR_PENDING made >>>>>> it possible to check for this state in this code. Maybe you have a >>>>>> better way around this, but that is why I chose to implement it this >>>>>> way. >>>>>> >>>>>> I agree that MPI_ERR_PENDING is only to be set in MPI_Waitall. And, in >>>>>> this patch, MPI_ERR_PENDING is only set in that operation. The other >>>>>> operations must look for the MPI_ERR_PENDING and reset the value - >>>>>> since we are using the internal status object to carry it around per >>>>>> above. >>>>>> >>>>>> You said that the implementation in ompi_request_default_wait_all was >>>>>> incorrect. Would you care to elaborate as to what specifically is >>>>>> incorrect? >>>>>> >>>>>> -- Josh >>>>>> >>>>>> On Wed, Mar 21, 2012 at 2:17 PM, George Bosilca <bosi...@eecs.utk.edu> >>>>>> wrote: >>>>>>> Josh, >>>>>>> >>>>>>> I don't agree that these changes are required. In the current standard >>>>>>> (2.2), MPI_ERR_PENDING is only allowed to be returned by MPI_WAITALL, >>>>>>> in some very specific conditions. Here is the snippet from the MPI >>>>>>> standard clarifying this behavior. >>>>>>> >>>>>>>> When one or more of the communications completed by a call to >>>>>>>> MPI_WAITALL fail, it is desireable to return specific information on >>>>>>>> each communication. The function MPI_WAITALL will return in such case >>>>>>>> the error code MPI_ERR_IN_STATUS and will set the error field of each >>>>>>>> status to a specific error code. This code will be MPI_SUCCESS, if the >>>>>>>> specific communication completed; it will be another specific error >>>>>>>> code, if it failed; or it can be MPI_ERR_PENDING if it has neither >>>>>>>> failed nor completed. >>>>>>> >>>>>>> As you can notice, the MPI_ERR_PENDING is only for the error in the >>>>>>> status array and not for the error field in the status attached to the >>>>>>> MPI request. Moreover, we don't use this inside Open MPI at all. >>>>>>> >>>>>>> The usage we're doing in the default_wait_all of MPI_ERR_PENDING is >>>>>>> incorrect as well. I will fix it, once we clarify the issue with your >>>>>>> patch. >>>>>>> >>>>>>> Thanks, >>>>>>> george. >>>>>>> >>>>>>> On Mar 21, 2012, at 13:46 , jjhur...@osl.iu.edu wrote: >>>>>>> >>>>>>>> Author: jjhursey >>>>>>>> Date: 2012-03-21 13:46:15 EDT (Wed, 21 Mar 2012) >>>>>>>> New Revision: 26172 >>>>>>>> URL: https://svn.open-mpi.org/trac/ompi/changeset/26172 >>>>>>>> >>>>>>>> Log: >>>>>>>> Add support for MPI_ERR_PENDING - Per MPI 2.2 p 60 >>>>>>>> >>>>>>>> Tested with: >>>>>>>> ompi-tests/mpich_tester/mpich_pt2pt/truncmult.c >>>>>>>> >>>>>>>> >>>>>>>> Text files modified: >>>>>>>> trunk/ompi/errhandler/errhandler_invoke.c | 9 ++ >>>>>>>> trunk/ompi/request/req_test.c | 32 ++++++++++ >>>>>>>> trunk/ompi/request/req_wait.c | 120 >>>>>>>> +++++++++++++++++++++++++++++++++++++-- >>>>>>>> trunk/ompi/request/request.c | 2 >>>>>>>> trunk/ompi/request/request.h | 5 + >>>>>>>> 5 files changed, 158 insertions(+), 10 deletions(-) >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> devel mailing list >>>>>>> de...@open-mpi.org >>>>>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Joshua Hursey >>>>>> Postdoctoral Research Associate >>>>>> Oak Ridge National Laboratory >>>>>> http://users.nccs.gov/~jjhursey >>>>>> >>>>>> _______________________________________________ >>>>>> devel mailing list >>>>>> de...@open-mpi.org >>>>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>>>> >>>>> >>>>> _______________________________________________ >>>>> devel mailing list >>>>> de...@open-mpi.org >>>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>>>> >>>> >>>> >>>> >>>> -- >>>> Joshua Hursey >>>> Postdoctoral Research Associate >>>> Oak Ridge National Laboratory >>>> http://users.nccs.gov/~jjhursey >>>> >>>> _______________________________________________ >>>> devel mailing list >>>> de...@open-mpi.org >>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>> >>> >>> _______________________________________________ >>> devel mailing list >>> de...@open-mpi.org >>> http://www.open-mpi.org/mailman/listinfo.cgi/devel >>> >> >> >> -- >> Joshua Hursey >> Postdoctoral Research Associate >> Oak Ridge National Laboratory >> http://users.nccs.gov/~jjhursey > > > > -- > Joshua Hursey > Postdoctoral Research Associate > Oak Ridge National Laboratory > http://users.nccs.gov/~jjhursey > > _______________________________________________ > devel mailing list > de...@open-mpi.org > http://www.open-mpi.org/mailman/listinfo.cgi/devel
