On Wed, Oct 17, 2007 at 05:32:47PM -0400, Jeff Squyres wrote:
> 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?
>
My claim is that the logic is equivalent :) But I am asking here for
others to comment.
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
> > de...@open-mpi.org
> > http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
>
> --
> Jeff Squyres
> Cisco Systems
>
> _______________________________________________
> devel mailing list
> de...@open-mpi.org
> http://www.open-mpi.org/mailman/listinfo.cgi/devel
--
Gleb.
