Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

From: Jeff Squyres (jsquyres_at_[hidden])
Date: 2007-10-17 17:32:47


Gleb -

I am not overly familiar with all these portions of the pml code
base, but it looks like not all of these places have exactly the same
code: the inline version is much shorter than some of the original
pml codes that it replaced. Is the logic equivalent?

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.). :-)

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