This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "MPICH primary repository". The branch, master has been updated via 6063f89812b3b1b718abffa1b0c895cd5a19754c (commit) from 725ffc91efbfc7414be7de9b26f5f01bdd6a7cce (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- http://git.mpich.org/mpich.git/commitdiff/6063f89812b3b1b718abffa1b0c895cd5a... commit 6063f89812b3b1b718abffa1b0c895cd5a19754c Author: Michael Blocksome <[email protected]> Date: Fri May 30 15:00:15 2014 -0500 pamid: fix request completion race condition In a multi-threaded environment where one thread is waiting for a request to complete, the instant the request completion counter is set to zero by a different thread the waiting thread will begin to free the request resources. The completing thread must not touch any memory from the old request object after setting the completion count to zero. See ticket #2103 diff --git a/src/mpid/pamid/src/onesided/mpid_1s.c b/src/mpid/pamid/src/onesided/mpid_1s.c index 447530b..fa91cd9 100644 --- a/src/mpid/pamid/src/onesided/mpid_1s.c +++ b/src/mpid/pamid/src/onesided/mpid_1s.c @@ -55,8 +55,7 @@ MPIDI_Win_DoneCB(pami_context_t context, ((req->type >= MPIDI_WIN_REQUEST_COMPARE_AND_SWAP) && (req->origin.completed == req->origin.dt.num_contig))) { - if(req->req_handle) - MPID_cc_set(req->req_handle->cc_ptr, 0); + MPID_Request * req_handle = req->req_handle; if (req->buffer_free) { MPIU_Free(req->buffer); @@ -66,8 +65,22 @@ MPIDI_Win_DoneCB(pami_context_t context, MPIDI_Win_datatype_unmap(&req->target.dt); if (req->accum_headers) MPIU_Free(req->accum_headers); + if (!((req->type > MPIDI_WIN_REQUEST_GET_ACCUMULATE) && (req->type <=MPIDI_WIN_REQUEST_RGET_ACCUMULATE))) MPIU_Free(req); + + if(req_handle) { + + /* The instant this completion counter is set to zero another thread + * may notice the change and begin freeing request resources. The + * thread executing the code in this function must not touch any + * portion of the request structure after decrementing the completion + * counter. + * + * See MPID_Request_release_inline() + */ + MPID_cc_set(req_handle->cc_ptr, 0); + } } MPIDI_Progress_signal(); } diff --git a/src/mpid/pamid/src/onesided/mpid_win_get.c b/src/mpid/pamid/src/onesided/mpid_win_get.c index e3d9885..019d513 100644 --- a/src/mpid/pamid/src/onesided/mpid_win_get.c +++ b/src/mpid/pamid/src/onesided/mpid_win_get.c @@ -260,17 +260,27 @@ MPID_Get(void *origin_addr, /* If the get is a local operation, do it here */ if (target_rank == win->comm_ptr->rank) { - size_t offset = req->offset; + /* The operation is not complete until the local copy is performed */ + mpi_errno = MPIR_Localcopy(win->base + req->offset, + target_count, + target_datatype, + origin_addr, + origin_count, + origin_datatype); + + /* The instant this completion counter is set to zero another thread + * may notice the change and begin freeing request resources. The + * thread executing the code in this function must not touch any + * portion of the request structure after decrementing the completion + * counter. + * + * See MPID_Request_release_inline() + */ if(req->req_handle) MPID_cc_set(req->req_handle->cc_ptr, 0); else MPIU_Free(req); - return MPIR_Localcopy(win->base + offset, - target_count, - target_datatype, - origin_addr, - origin_count, - origin_datatype); + return mpi_errno; } req->target.rank = target_rank; diff --git a/src/mpid/pamid/src/onesided/mpid_win_get_accumulate.c b/src/mpid/pamid/src/onesided/mpid_win_get_accumulate.c index ca93a13..420c9e2 100644 --- a/src/mpid/pamid/src/onesided/mpid_win_get_accumulate.c +++ b/src/mpid/pamid/src/onesided/mpid_win_get_accumulate.c @@ -139,21 +139,34 @@ MPIDI_Win_GetAccDoneCB(pami_context_t context, if (req->origin.completed == req->target.dt.num_contig + 1) { - if(req->req_handle) - MPID_cc_set(req->req_handle->cc_ptr, 0); + MPID_Request * req_handle = req->req_handle; if (req->buffer_free) { MPIU_Free(req->buffer); req->buffer_free = 0; } if (req->accum_headers) - MPIU_Free(req->accum_headers); + MPIU_Free(req->accum_headers); MPIDI_Win_datatype_unmap(&req->target.dt); MPIDI_Win_datatype_unmap(&req->result.dt); - if( req->type != MPIDI_WIN_REQUEST_RGET_ACCUMULATE ) - MPIU_Free(req); + if( req->type != MPIDI_WIN_REQUEST_RGET_ACCUMULATE ) { + if (req_handle) { + req_handle->mpid.win_req = NULL; + } + MPIU_Free(req); + } + /* The instant this completion counter is set to zero another thread + * may notice the change and begin freeing request resources. The + * thread executing the code in this function must not touch any + * portion of the request structure after decrementing the completion + * counter. + * + * See MPID_Request_release_inline() + */ + if(req_handle) + MPID_cc_set(req_handle->cc_ptr, 0); } MPIDI_Progress_signal(); } diff --git a/src/mpid/pamid/src/onesided/mpid_win_put.c b/src/mpid/pamid/src/onesided/mpid_win_put.c index a552fa0..db5f20c 100644 --- a/src/mpid/pamid/src/onesided/mpid_win_put.c +++ b/src/mpid/pamid/src/onesided/mpid_win_put.c @@ -265,17 +265,27 @@ MPID_Put(const void *origin_addr, /* If the get is a local operation, do it here */ if (target_rank == win->comm_ptr->rank) { - size_t offset = req->offset; + /* The operation is not complete until the local copy is performed */ + mpi_errno = MPIR_Localcopy(origin_addr, + origin_count, + origin_datatype, + win->base + req->offset, + target_count, + target_datatype); + + /* The instant this completion counter is set to zero another thread + * may notice the change and begin freeing request resources. The + * thread executing the code in this function must not touch any + * portion of the request structure after decrementing the completion + * counter. + * + * See MPID_Request_release_inline() + */ if(req->req_handle) MPID_cc_set(req->req_handle->cc_ptr, 0); else MPIU_Free(req); - return MPIR_Localcopy(origin_addr, - origin_count, - origin_datatype, - win->base + offset, - target_count, - target_datatype); + return mpi_errno; } req->target.rank = target_rank; ----------------------------------------------------------------------- Summary of changes: src/mpid/pamid/src/onesided/mpid_1s.c | 17 ++++++++++++- src/mpid/pamid/src/onesided/mpid_win_get.c | 24 ++++++++++++++----- .../pamid/src/onesided/mpid_win_get_accumulate.c | 23 +++++++++++++++---- src/mpid/pamid/src/onesided/mpid_win_put.c | 24 ++++++++++++++----- 4 files changed, 67 insertions(+), 21 deletions(-) hooks/post-receive -- MPICH primary repository