From 9a8a87611e1d94a9aef485743e48c8f41805317a Mon Sep 17 00:00:00 2001 From: Nathan Hjelm Date: Tue, 23 Jun 2015 11:00:45 -0600 Subject: [PATCH] pml/ob1: fix bugs in static request objects This commit fixes several bugs in the static request objects used by ob1 for blocking send/receive operations. - Fix memory leak when using MPI_THREAD_MULTIPLE. Requests were allocated off the free list but were destructed and NOT returned. - Fix double-destruct of static objects. There is no reason to CONSTRUCT/DESTUCT the static object for each send/receive operation. This adds overhead and no benefit. To keep the code clean helper functions have been added to finalize ob1 send/receive requests. - Remove now unnecessary include of alloca.h. Signed-off-by: Nathan Hjelm --- ompi/mca/pml/ob1/pml_ob1_component.c | 21 ++++++++++++--------- ompi/mca/pml/ob1/pml_ob1_irecv.c | 14 +++++++------- ompi/mca/pml/ob1/pml_ob1_isend.c | 12 ++++++------ ompi/mca/pml/ob1/pml_ob1_recvreq.h | 15 ++++++++++----- ompi/mca/pml/ob1/pml_ob1_sendreq.h | 17 +++++++++++------ 5 files changed, 46 insertions(+), 33 deletions(-) diff --git a/ompi/mca/pml/ob1/pml_ob1_component.c b/ompi/mca/pml/ob1/pml_ob1_component.c index 2f37468ee2..87d765bea9 100644 --- a/ompi/mca/pml/ob1/pml_ob1_component.c +++ b/ompi/mca/pml/ob1/pml_ob1_component.c @@ -292,6 +292,18 @@ int mca_pml_ob1_component_fini(void) return OMPI_SUCCESS; /* never selected.. return success.. */ mca_pml_ob1.enabled = false; /* not anymore */ + /* return the static receive/send requests to the respective free list and + * let the free list handle destruction. */ + if( NULL != mca_pml_ob1_recvreq ) { + opal_free_list_return (&mca_pml_base_recv_requests, (opal_free_list_item_t *) mca_pml_ob1_recvreq); + mca_pml_ob1_recvreq = NULL; + } + + if( NULL != mca_pml_ob1_sendreq ) { + opal_free_list_return (&mca_pml_base_send_requests, (opal_free_list_item_t *) mca_pml_ob1_sendreq); + mca_pml_ob1_sendreq = NULL; + } + OBJ_DESTRUCT(&mca_pml_ob1.rdma_pending); OBJ_DESTRUCT(&mca_pml_ob1.pckt_pending); OBJ_DESTRUCT(&mca_pml_ob1.recv_pending); @@ -304,15 +316,6 @@ int mca_pml_ob1_component_fini(void) OBJ_DESTRUCT(&mca_pml_ob1.lock); OBJ_DESTRUCT(&mca_pml_ob1.send_ranges); - if( NULL != mca_pml_ob1_recvreq ) { - OBJ_DESTRUCT(mca_pml_ob1_recvreq); - mca_pml_ob1_recvreq = NULL; - } - if( NULL != mca_pml_ob1_sendreq ) { - OBJ_DESTRUCT(mca_pml_ob1_sendreq); - mca_pml_ob1_sendreq = NULL; - } - if( NULL != mca_pml_ob1.allocator ) { (void)mca_pml_ob1.allocator->alc_finalize(mca_pml_ob1.allocator); mca_pml_ob1.allocator = NULL; diff --git a/ompi/mca/pml/ob1/pml_ob1_irecv.c b/ompi/mca/pml/ob1/pml_ob1_irecv.c index 459af46d5e..e37bd1a854 100644 --- a/ompi/mca/pml/ob1/pml_ob1_irecv.c +++ b/ompi/mca/pml/ob1/pml_ob1_irecv.c @@ -10,7 +10,7 @@ * University of Stuttgart. All rights reserved. * Copyright (c) 2004-2005 The Regents of the University of California. * All rights reserved. - * Copyright (c) 2007-2014 Los Alamos National Security, LLC. All rights + * Copyright (c) 2007-2015 Los Alamos National Security, LLC. All rights * reserved. * Copyright (c) 2010-2012 Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2011 Sandia National Laboratories. All rights reserved. @@ -28,9 +28,6 @@ #include "pml_ob1_recvfrag.h" #include "ompi/peruse/peruse-internal.h" #include "ompi/message/message.h" -#if HAVE_ALLOCA_H -#include -#endif /* HAVE_ALLOCA_H */ mca_pml_ob1_recv_request_t *mca_pml_ob1_recvreq = NULL; @@ -109,7 +106,6 @@ int mca_pml_ob1_recv(void *addr, mca_pml_ob1_recvreq = recvreq; #endif /* !OMPI_ENABLE_THREAD_MULTIPLE */ } - OBJ_CONSTRUCT(recvreq, mca_pml_ob1_recv_request_t); MCA_PML_OB1_RECV_REQUEST_INIT(recvreq, addr, count, datatype, src, tag, comm, false); @@ -126,8 +122,12 @@ int mca_pml_ob1_recv(void *addr, } rc = recvreq->req_recv.req_base.req_ompi.req_status.MPI_ERROR; - MCA_PML_BASE_RECV_REQUEST_FINI(&recvreq->req_recv); - OBJ_DESTRUCT(recvreq); + +#if OMPI_ENABLE_THREAD_MULTIPLE + MCA_PML_OB1_RECV_REQUEST_RETURN(recvreq); +#else + mca_pml_ob1_recv_request_fini (recvreq); +#endif return rc; } diff --git a/ompi/mca/pml/ob1/pml_ob1_isend.c b/ompi/mca/pml/ob1/pml_ob1_isend.c index e85ee88847..273883ea88 100644 --- a/ompi/mca/pml/ob1/pml_ob1_isend.c +++ b/ompi/mca/pml/ob1/pml_ob1_isend.c @@ -28,9 +28,6 @@ #include "pml_ob1_sendreq.h" #include "pml_ob1_recvreq.h" #include "ompi/peruse/peruse-internal.h" -#if HAVE_ALLOCA_H -#include -#endif /* HAVE_ALLOCA_H */ mca_pml_ob1_send_request_t *mca_pml_ob1_sendreq = NULL; @@ -232,7 +229,6 @@ int mca_pml_ob1_send(void *buf, mca_pml_ob1_sendreq = sendreq; #endif /* !OMPI_ENABLE_THREAD_MULTIPLE */ } - OBJ_CONSTRUCT(sendreq, mca_pml_ob1_send_request_t); sendreq->req_send.req_base.req_proc = dst_proc; sendreq->rdma_frag = NULL; @@ -252,9 +248,13 @@ int mca_pml_ob1_send(void *buf, ompi_request_wait_completion(&sendreq->req_send.req_base.req_ompi); rc = sendreq->req_send.req_base.req_ompi.req_status.MPI_ERROR; - MCA_PML_BASE_SEND_REQUEST_FINI(&sendreq->req_send); } - OBJ_DESTRUCT(sendreq); + +#if OMPI_ENABLE_THREAD_MULTIPLE + MCA_PML_OB1_SEND_REQUEST_RETURN(sendreq); +#else + mca_pml_ob1_send_request_fini (sendreq); +#endif return rc; } diff --git a/ompi/mca/pml/ob1/pml_ob1_recvreq.h b/ompi/mca/pml/ob1/pml_ob1_recvreq.h index c9b75ecf2c..4f0338d2bb 100644 --- a/ompi/mca/pml/ob1/pml_ob1_recvreq.h +++ b/ompi/mca/pml/ob1/pml_ob1_recvreq.h @@ -128,16 +128,21 @@ do { \ ompi_request_complete( &(recvreq->req_recv.req_base.req_ompi), true ); \ } while (0) +static inline void mca_pml_ob1_recv_request_fini (mca_pml_ob1_recv_request_t *recvreq) +{ + MCA_PML_BASE_RECV_REQUEST_FINI(&recvreq->req_recv); + if ((recvreq)->local_handle) { + mca_bml_base_deregister_mem (recvreq->rdma_bml, recvreq->local_handle); + recvreq->local_handle = NULL; + } +} + /* * Free the PML receive request */ #define MCA_PML_OB1_RECV_REQUEST_RETURN(recvreq) \ { \ - MCA_PML_BASE_RECV_REQUEST_FINI(&(recvreq)->req_recv); \ - if ((recvreq)->local_handle) { \ - mca_bml_base_deregister_mem ((recvreq)->rdma_bml, (recvreq)->local_handle); \ - (recvreq)->local_handle = NULL; \ - } \ + mca_pml_ob1_recv_request_fini (recvreq); \ opal_free_list_return (&mca_pml_base_recv_requests, \ (opal_free_list_item_t*)(recvreq)); \ } diff --git a/ompi/mca/pml/ob1/pml_ob1_sendreq.h b/ompi/mca/pml/ob1/pml_ob1_sendreq.h index e606b8c99e..fc57b9b839 100644 --- a/ompi/mca/pml/ob1/pml_ob1_sendreq.h +++ b/ompi/mca/pml/ob1/pml_ob1_sendreq.h @@ -215,18 +215,23 @@ do { &(sendreq->req_send.req_base), PERUSE_SEND); \ } while(0) +static inline void mca_pml_ob1_send_request_fini (mca_pml_ob1_send_request_t *sendreq) +{ + /* Let the base handle the reference counts */ + MCA_PML_BASE_SEND_REQUEST_FINI((&(sendreq)->req_send)); + if (sendreq->rdma_frag) { + MCA_PML_OB1_RDMA_FRAG_RETURN (sendreq->rdma_frag); + sendreq->rdma_frag = NULL; + } +} + /* * Release resources associated with a request */ #define MCA_PML_OB1_SEND_REQUEST_RETURN(sendreq) \ do { \ - /* Let the base handle the reference counts */ \ - MCA_PML_BASE_SEND_REQUEST_FINI((&(sendreq)->req_send)); \ - if (sendreq->rdma_frag) { \ - MCA_PML_OB1_RDMA_FRAG_RETURN (sendreq->rdma_frag); \ - sendreq->rdma_frag = NULL; \ - } \ + mca_pml_ob1_send_request_fini (sendreq); \ opal_free_list_return ( &mca_pml_base_send_requests, \ (opal_free_list_item_t*)sendreq); \ } while(0)