Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

From: George Bosilca (bosilca_at_[hidden])
Date: 2007-10-18 06:47:01


On Oct 18, 2007, at 4:13 AM, Gleb Natapov wrote:

> My claim is that the logic is equivalent :) But I am asking here for
> others to comment.

I was thinking that being quiet is a kind of agreement :) Anyway, I
think this is a useful change, and there is no apparent behavior
modification between the original code and yours. Please go ahead and
commit it.

   Thanks,
     george.

>
> In most places the logic is like this:
> if (req is not completed) {
> if (more then one thread) {
> acquire lock
> wait for completion
> release lock
> } else {
> wait for completion
> }
> }
>
> inline function does:
> if (req is not completed) {
> if (more then one thread) {
> acquire lock
> }
> wait for completion
> if (more then one thread) {
> release lock
> }
> }
>
> And in non threaded build both "if (one the one thread){}"
> statements are
> removed by preprocessor.
>
>> Also, a minor nit -- it would be nice if the new inline function
>> conformed to our coding standards (constants on the left of ==, {}
>> around all blocks, etc.). :-)
> OK. The new code mimics the code it replaces :)
>
>>
>>
>> On Oct 15, 2007, at 10:27 AM, Gleb Natapov wrote:
>>
>>> Hi,
>>>
>>> Each time a someone needs to wait for request completion he
>>> implements the same piece of code. Why not put this code into
>>> inline function and use it instead. Look at the included patch, it
>>> moves the common code into ompi_request_wait_completion() function.
>>> Does somebody have any objection against committing it to the trunk?
>>>
>>> diff --git a/ompi/mca/crcp/coord/crcp_coord_pml.c b/ompi/mca/crcp/
>>> coord/crcp_coord_pml.c
>>> index b2392e4..eb9b9c1 100644
>>> --- a/ompi/mca/crcp/coord/crcp_coord_pml.c
>>> +++ b/ompi/mca/crcp/coord/crcp_coord_pml.c
>>> @@ -3857,13 +3857,7 @@ static int coord_request_wait_all( size_t
>>> count,
>>> static int coord_request_wait( ompi_request_t * req,
>>> ompi_status_public_t * status)
>>> {
>>> - OPAL_THREAD_LOCK(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (req->req_complete == false) {
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - }
>>> - ompi_request_waiting--;
>>> - OPAL_THREAD_UNLOCK(&ompi_request_lock);
>>> + ompi_request_wait_completion(req);
>>>
>>> if( MPI_STATUS_IGNORE != status ) {
>>> status->MPI_TAG = req->req_status.MPI_TAG;
>>> diff --git a/ompi/mca/pml/cm/pml_cm_recv.c b/ompi/mca/pml/cm/
>>> pml_cm_recv.c
>>> index 0e23c9a..00efffc 100644
>>> --- a/ompi/mca/pml/cm/pml_cm_recv.c
>>> +++ b/ompi/mca/pml/cm/pml_cm_recv.c
>>> @@ -112,22 +112,7 @@ mca_pml_cm_recv(void *addr,
>>> return ret;
>>> }
>>>
>>> - if (recvreq->req_base.req_ompi.req_complete == false) {
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (recvreq->req_base.req_ompi.req_complete ==
>>> false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (recvreq->req_base.req_ompi.req_complete ==
>>> false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> + ompi_request_wait_completion(&recvreq->req_base.req_ompi);
>>>
>>> if (NULL != status) { /* return status */
>>> *status = recvreq->req_base.req_ompi.req_status;
>>> diff --git a/ompi/mca/pml/cm/pml_cm_send.c b/ompi/mca/pml/cm/
>>> pml_cm_send.c
>>> index ed9b189..f7d2e8c 100644
>>> --- a/ompi/mca/pml/cm/pml_cm_send.c
>>> +++ b/ompi/mca/pml/cm/pml_cm_send.c
>>> @@ -175,23 +175,8 @@ mca_pml_cm_send(void *buf,
>>> MCA_PML_CM_THIN_SEND_REQUEST_RETURN(sendreq);
>>> return ret;
>>> }
>>> -
>>> - if (sendreq->req_send.req_base.req_ompi.req_complete
>>> == false) {
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> +
>>> + ompi_request_wait_completion(&sendreq-
>>>> req_send.req_base.req_ompi);
>>>
>>> ompi_request_free( (ompi_request_t**)&sendreq );
>>> } else {
>>> diff --git a/ompi/mca/pml/dr/pml_dr_iprobe.c b/ompi/mca/pml/dr/
>>> pml_dr_iprobe.c
>>> index 9149174..2063c54 100644
>>> --- a/ompi/mca/pml/dr/pml_dr_iprobe.c
>>> +++ b/ompi/mca/pml/dr/pml_dr_iprobe.c
>>> @@ -64,22 +64,7 @@ int mca_pml_dr_probe(int src,
>>> MCA_PML_DR_RECV_REQUEST_INIT(&recvreq, NULL, 0,
>>> &ompi_mpi_char, src, tag, comm, true);
>>> MCA_PML_DR_RECV_REQUEST_START(&recvreq);
>>>
>>> - if (recvreq.req_recv.req_base.req_ompi.req_complete == false) {
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (recvreq.req_recv.req_base.req_ompi.req_complete
>>> == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (recvreq.req_recv.req_base.req_ompi.req_complete
>>> == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> + ompi_request_wait_completion
>>> (&recvreq.req_recv.req_base.req_ompi);
>>>
>>> if (NULL != status) {
>>> *status = recvreq.req_recv.req_base.req_ompi.req_status;
>>> @@ -87,4 +72,3 @@ int mca_pml_dr_probe(int src,
>>> MCA_PML_BASE_RECV_REQUEST_FINI( &recvreq.req_recv );
>>> return OMPI_SUCCESS;
>>> }
>>> -
>>> diff --git a/ompi/mca/pml/dr/pml_dr_irecv.c b/ompi/mca/pml/dr/
>>> pml_dr_irecv.c
>>> index 4627341..8dd8c57 100644
>>> --- a/ompi/mca/pml/dr/pml_dr_irecv.c
>>> +++ b/ompi/mca/pml/dr/pml_dr_irecv.c
>>> @@ -87,33 +87,8 @@ int mca_pml_dr_recv(void *addr,
>>> count, datatype, src, tag,
>>> comm, false);
>>>
>>> MCA_PML_DR_RECV_REQUEST_START(recvreq);
>>> - if (recvreq->req_recv.req_base.req_ompi.req_complete ==
>>> false) {
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> - if(opal_progress_spin(&recvreq-
>>>> req_recv.req_base.req_ompi.req_complete)) {
>>> - goto finished;
>>> - }
>>> -#endif
>>> + ompi_request_wait_completion(&recvreq-
>>>> req_recv.req_base.req_ompi);
>>>
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (recvreq-
>>>> req_recv.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (recvreq-
>>>> req_recv.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> -
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> -finished:
>>> -#endif
>>> -
>>> if (NULL != status) { /* return status */
>>> *status = recvreq->req_recv.req_base.req_ompi.req_status;
>>> }
>>> diff --git a/ompi/mca/pml/dr/pml_dr_isend.c b/ompi/mca/pml/dr/
>>> pml_dr_isend.c
>>> index 206599a..b4d9192 100644
>>> --- a/ompi/mca/pml/dr/pml_dr_isend.c
>>> +++ b/ompi/mca/pml/dr/pml_dr_isend.c
>>> @@ -105,33 +105,10 @@ int mca_pml_dr_send(void *buf,
>>> return rc;
>>> }
>>>
>>> - if (sendreq->req_send.req_base.req_ompi.req_complete ==
>>> false) {
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> - if(opal_progress_spin(&sendreq-
>>>> req_send.req_base.req_ompi.req_complete)) {
>>> - ompi_request_free( (ompi_request_t**)&sendreq );
>>> - return OMPI_SUCCESS;
>>> - }
>>> -#endif
>>> -
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> + ompi_request_wait_completion(&sendreq-
>>>> req_send.req_base.req_ompi);
>>>
>>> /* return request to pool */
>>> rc = sendreq->req_send.req_base.req_ompi.req_status.MPI_ERROR;
>>> ompi_request_free((ompi_request_t **) & sendreq);
>>> return rc;
>>> }
>>> -
>>> diff --git a/ompi/mca/pml/ob1/pml_ob1_iprobe.c b/ompi/mca/pml/ob1/
>>> pml_ob1_iprobe.c
>>> index ac0c54d..c86f1c7 100644
>>> --- a/ompi/mca/pml/ob1/pml_ob1_iprobe.c
>>> +++ b/ompi/mca/pml/ob1/pml_ob1_iprobe.c
>>> @@ -64,22 +64,7 @@ int mca_pml_ob1_probe(int src,
>>> MCA_PML_OB1_RECV_REQUEST_INIT(&recvreq, NULL, 0,
>>> &ompi_mpi_char, src, tag, comm, true);
>>> MCA_PML_OB1_RECV_REQUEST_START(&recvreq);
>>>
>>> - if (recvreq.req_recv.req_base.req_ompi.req_complete == false) {
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (recvreq.req_recv.req_base.req_ompi.req_complete
>>> == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> - ompi_request_waiting++;
>>> - while (recvreq.req_recv.req_base.req_ompi.req_complete
>>> == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - }
>>> - }
>>> + ompi_request_wait_completion
>>> (&recvreq.req_recv.req_base.req_ompi);
>>>
>>> if (NULL != status) {
>>> *status = recvreq.req_recv.req_base.req_ompi.req_status;
>>> @@ -87,4 +72,3 @@ int mca_pml_ob1_probe(int src,
>>> MCA_PML_BASE_RECV_REQUEST_FINI( &recvreq.req_recv );
>>> return OMPI_SUCCESS;
>>> }
>>> -
>>> diff --git a/ompi/mca/pml/ob1/pml_ob1_irecv.c b/ompi/mca/pml/ob1/
>>> pml_ob1_irecv.c
>>> index d2d89ce..a8d95bd 100644
>>> --- a/ompi/mca/pml/ob1/pml_ob1_irecv.c
>>> +++ b/ompi/mca/pml/ob1/pml_ob1_irecv.c
>>> @@ -101,37 +101,7 @@ int mca_pml_ob1_recv(void *addr,
>>> PERUSE_RECV);
>>>
>>> MCA_PML_OB1_RECV_REQUEST_START(recvreq);
>>> - if (recvreq->req_recv.req_base.req_ompi.req_complete ==
>>> false) {
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> - if(opal_progress_spin(&recvreq-
>>>> req_recv.req_base.req_ompi.req_complete)) {
>>> - goto finished;
>>> - }
>>> -#endif
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (recvreq-
>>>> req_recv.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> -#if OMPI_ENABLE_DEBUG && !OMPI_HAVE_THREAD_SUPPORT
>>> - OPAL_THREAD_LOCK(&ompi_request_lock);
>>> -#endif
>>> - ompi_request_waiting++;
>>> - while (recvreq-
>>>> req_recv.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> -#if OMPI_ENABLE_DEBUG && !OMPI_HAVE_THREAD_SUPPORT
>>> - OPAL_THREAD_UNLOCK(&ompi_request_lock);
>>> -#endif
>>> - }
>>> - }
>>> -
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> -finished:
>>> -#endif
>>> + ompi_request_wait_completion(&recvreq-
>>>> req_recv.req_base.req_ompi);
>>>
>>> if (NULL != status) { /* return status */
>>> *status = recvreq->req_recv.req_base.req_ompi.req_status;
>>> diff --git a/ompi/mca/pml/ob1/pml_ob1_isend.c b/ompi/mca/pml/ob1/
>>> pml_ob1_isend.c
>>> index a647fac..0fbf7f6 100644
>>> --- a/ompi/mca/pml/ob1/pml_ob1_isend.c
>>> +++ b/ompi/mca/pml/ob1/pml_ob1_isend.c
>>> @@ -120,38 +120,9 @@ int mca_pml_ob1_send(void *buf,
>>> return rc;
>>> }
>>>
>>> - if (sendreq->req_send.req_base.req_ompi.req_complete ==
>>> false) {
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> - if(opal_progress_spin(&sendreq-
>>>> req_send.req_base.req_ompi.req_complete)) {
>>> - ompi_request_free( (ompi_request_t**)&sendreq );
>>> - return OMPI_SUCCESS;
>>> - }
>>> -#endif
>>> -
>>> - /* give up and sleep until completion */
>>> - if (opal_using_threads()) {
>>> - opal_mutex_lock(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> - opal_mutex_unlock(&ompi_request_lock);
>>> - } else {
>>> -#if OMPI_ENABLE_DEBUG && !OMPI_HAVE_THREAD_SUPPORT
>>> - OPAL_THREAD_LOCK(&ompi_request_lock);
>>> -#endif
>>> - ompi_request_waiting++;
>>> - while (sendreq-
>>>> req_send.req_base.req_ompi.req_complete == false)
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - ompi_request_waiting--;
>>> -#if OMPI_ENABLE_DEBUG && !OMPI_HAVE_THREAD_SUPPORT
>>> - OPAL_THREAD_UNLOCK(&ompi_request_lock);
>>> -#endif
>>> - }
>>> - }
>>> + ompi_request_wait_completion(&sendreq-
>>>> req_send.req_base.req_ompi);
>>>
>>> rc = sendreq->req_send.req_base.req_ompi.req_status.MPI_ERROR;
>>> ompi_request_free( (ompi_request_t**)&sendreq );
>>> return rc;
>>> }
>>> -
>>> diff --git a/ompi/request/req_wait.c b/ompi/request/req_wait.c
>>> index 0bb4fb3..4751804 100644
>>> --- a/ompi/request/req_wait.c
>>> +++ b/ompi/request/req_wait.c
>>> @@ -31,27 +31,7 @@ int ompi_request_wait(
>>> {
>>> ompi_request_t *req = *req_ptr;
>>>
>>> - if(req->req_complete == false) {
>>> -
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> - /* poll for completion */
>>> - if(opal_progress_spin(&req->req_complete))
>>> - goto finished;
>>> -#endif
>>> -
>>> - /* give up and sleep until completion */
>>> - OPAL_THREAD_LOCK(&ompi_request_lock);
>>> - ompi_request_waiting++;
>>> - while (req->req_complete == false) {
>>> - opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> - }
>>> - ompi_request_waiting--;
>>> - OPAL_THREAD_UNLOCK(&ompi_request_lock);
>>> - }
>>> -
>>> -#if OMPI_ENABLE_PROGRESS_THREADS
>>> -finished:
>>> -#endif
>>> + ompi_request_wait_completion(req);
>>>
>>> #if OPAL_ENABLE_FT == 1
>>> OMPI_CRCP_REQUEST_COMPLETE(req);
>>> diff --git a/ompi/request/request.h b/ompi/request/request.h
>>> index 70f38e1..8dc0920 100644
>>> --- a/ompi/request/request.h
>>> +++ b/ompi/request/request.h
>>> @@ -374,7 +374,22 @@ OMPI_DECLSPEC int ompi_request_wait_some(
>>> int * indices,
>>> ompi_status_public_t * statuses);
>>>
>>> +static inline void ompi_request_wait_completion(ompi_request_t
>>> *req)
>>> +{
>>> + if(req->req_complete == false) {
>>> +#if OMPI_ENABLE_PROGRESS_THREADS
>>> + if(opal_progress_spin(&req->req_complete))
>>> + return;
>>> +#endif
>>> + OPAL_THREAD_LOCK(&ompi_request_lock);
>>> + ompi_request_waiting++;
>>> + while(req->req_complete == false)
>>> + opal_condition_wait(&ompi_request_cond,
>>> &ompi_request_lock);
>>> + ompi_request_waiting--;
>>> + OPAL_THREAD_UNLOCK(&ompi_request_lock);
>>> + }
>>> +}
>>> +
>>> END_C_DECLS
>>>
>>> #endif
>>> -
>>> --
>>> Gleb.
>>> _______________________________________________
>>> devel mailing list
>>> devel_at_[hidden]
>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>>
>>
>> --
>> Jeff Squyres
>> Cisco Systems
>>
>> _______________________________________________
>> devel mailing list
>> devel_at_[hidden]
>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
> --
> Gleb.
> _______________________________________________
> devel mailing list
> devel_at_[hidden]
> http://www.open-mpi.org/mailman/listinfo.cgi/devel



  • application/pkcs7-signature attachment: smime.p7s