From f7b0e3c68ef66a296041690fe8526f6d980c9bd4 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Mon, 24 Aug 2020 08:08:56 +0000 Subject: [PATCH 01/33] UCP/CORE: Implement flush+destroy for UCT EPs on UCP Worker --- src/ucp/core/ucp_request.h | 9 ++ src/ucp/core/ucp_worker.c | 132 +++++++++++++++++- src/ucp/core/ucp_worker.h | 5 + test/gtest/Makefile.am | 1 + test/gtest/ucp/test_ucp_am.cc | 2 +- test/gtest/ucp/test_ucp_worker.cc | 225 ++++++++++++++++++++++++++++++ 6 files changed, 371 insertions(+), 3 deletions(-) create mode 100644 test/gtest/ucp/test_ucp_worker.cc diff --git a/src/ucp/core/ucp_request.h b/src/ucp/core/ucp_request.h index f88c28f8838..4f1312ffe72 100644 --- a/src/ucp/core/ucp_request.h +++ b/src/ucp/core/ucp_request.h @@ -208,6 +208,15 @@ struct ucp_request { uct_worker_cb_id_t prog_id;/* Slow-path callback */ } disconnect; + struct { + ucp_worker_h ucp_worker; /* UCP worker where a discard UCT EP + * operation submitted on */ + uct_ep_h uct_ep; /* UCT EP that should be flushed and + destroyed */ + unsigned ep_flush_flags; /* Flags that should be passed into + @ref uct_ep_flush */ + } discard_uct_ep; + struct { uint64_t remote_addr; /* Remote address */ ucp_rkey_h rkey; /* Remote memory key */ diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 2719573e6c6..ab0bdbbaab6 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -445,7 +445,8 @@ static unsigned ucp_worker_iface_err_handle_progress(void *arg) if (lane != failed_lane) { ucs_trace("ep %p: destroy uct_ep[%d]=%p", ucp_ep, lane, ucp_ep->uct_eps[lane]); - uct_ep_destroy(ucp_ep->uct_eps[lane]); + ucp_worker_discard_uct_ep(ucp_ep->worker, ucp_ep->uct_eps[lane], + UCT_FLUSH_FLAG_CANCEL); ucp_ep->uct_eps[lane] = NULL; } } @@ -2306,7 +2307,6 @@ void ucp_worker_release_address(ucp_worker_h worker, ucp_address_t *address) ucs_free(address); } - void ucp_worker_print_info(ucp_worker_h worker, FILE *stream) { ucp_context_h context = worker->context; @@ -2350,3 +2350,131 @@ void ucp_worker_print_info(ucp_worker_h worker, FILE *stream) UCP_WORKER_THREAD_CS_EXIT_CONDITIONAL(worker); } + +static unsigned ucp_worker_discard_uct_ep_destroy_progress(void *arg) +{ + ucp_request_t *req = (ucp_request_t*)arg; + uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; + + ucp_trace_req(req, "destroy uct_ep=%p", uct_ep); + uct_ep_destroy(uct_ep); + ucp_request_put(req); + UCS_ASYNC_BLOCK(&worker->async); + --worker->flush_ops_count; + UCS_ASYNC_UNBLOCK(&worker->async); + + return 1; +} + +static void +ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, + ucs_status_t status) +{ + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req = ucs_container_of(self, ucp_request_t, + send.state.uct_comp); + ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; + + ucp_trace_req(req, "discard_uct_ep flush completion status %s", + ucs_status_string(status)); + + /* don't destroy UCT EP from the flush completion callback, schedule + * a progress callback on the main thread to destroy UCT EP */ + uct_worker_progress_register_safe(worker->uct, + ucp_worker_discard_uct_ep_destroy_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); +} + +static unsigned ucp_worker_discard_uct_ep_progress(void *arg) +{ + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req = (ucp_request_t*)arg; + uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; + ucs_status_t status; + + status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, + &req->send.state.uct_comp); + if (status == UCS_ERR_NO_RESOURCE) { + status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); + if (status == UCS_ERR_BUSY) { + /* try again to flush UCT EP */ + uct_worker_progress_register_safe(worker->uct, + ucp_worker_discard_uct_ep_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, + &cb_id); + return 0; + } + ucs_assert(status == UCS_OK); + } else if (status != UCS_INPROGRESS) { + ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); + } + + return 1; +} + +static ucs_status_t +ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) +{ + ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); + return ucp_worker_discard_uct_ep_progress(req) ? UCS_OK : UCS_INPROGRESS; +} + +/* must be called with async lock held */ +void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, + unsigned ep_flush_flags) +{ + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_wireup_ep_t *wireup_ep; + ucp_request_t *req; + int is_owner; + + ucs_assert(uct_ep != NULL); + + if (ucp_wireup_ep_test(uct_ep)) { + wireup_ep = ucp_wireup_ep(uct_ep); + ucs_assert(wireup_ep != NULL); + + is_owner = wireup_ep->super.is_owner; + uct_ep = ucp_wireup_ep_extract_next_ep(uct_ep); + + if (wireup_ep->aux_ep != NULL) { + /* discard the WIREUP EP's auxiliary EP */ + ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, + ep_flush_flags); + ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); + } + + /* destroy WIREUP EP allocated for this UCT EP, since + * discard operation most likely won't have an access to + * UCP EP as it could be destroyed by the caller */ + uct_ep_destroy(&wireup_ep->super.super); + + if (!is_owner) { + /* do nothing, if this wireup EP is not an owner for UCT EP */ + return; + } + } + + req = ucp_request_get(worker); + if (ucs_unlikely(req == NULL)) { + ucs_error("unable to allocate request for discarding UCT EP %p " + "on UCP worker %p", uct_ep, worker); + return; + } + + ++worker->flush_ops_count; + + ucs_assert(!ucp_wireup_ep_test(uct_ep)); + req->send.uct.func = ucp_worker_discard_uct_ep_pending_cb; + req->send.state.uct_comp.func = ucp_worker_discard_uct_ep_flush_comp; + req->send.state.uct_comp.count = 1; + req->send.discard_uct_ep.ucp_worker = worker; + req->send.discard_uct_ep.uct_ep = uct_ep; + req->send.discard_uct_ep.ep_flush_flags = ep_flush_flags; + uct_worker_progress_register_safe(worker->uct, + ucp_worker_discard_uct_ep_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, + &cb_id); +} diff --git a/src/ucp/core/ucp_worker.h b/src/ucp/core/ucp_worker.h index e36a963b2ac..51dc4a97a06 100644 --- a/src/ucp/core/ucp_worker.h +++ b/src/ucp/core/ucp_worker.h @@ -304,8 +304,13 @@ void ucp_worker_iface_activate(ucp_worker_iface_t *wiface, unsigned uct_flags); int ucp_worker_err_handle_remove_filter(const ucs_callbackq_elem_t *elem, void *arg); + ucs_status_t ucp_worker_set_ep_failed(ucp_worker_h worker, ucp_ep_h ucp_ep, uct_ep_h uct_ep, ucp_lane_index_t lane, ucs_status_t status); +/* must be called with async lock held */ +void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, + unsigned ep_flush_flags); + #endif diff --git a/test/gtest/Makefile.am b/test/gtest/Makefile.am index 15de8a489ed..ed78eab162c 100644 --- a/test/gtest/Makefile.am +++ b/test/gtest/Makefile.am @@ -135,6 +135,7 @@ gtest_SOURCES = \ ucp/test_ucp_tag_mem_type.cc \ ucp/test_ucp_tag.cc \ ucp/test_ucp_context.cc \ + ucp/test_ucp_worker.cc \ ucp/test_ucp_wireup.cc \ ucp/test_ucp_wakeup.cc \ ucp/test_ucp_fence.cc \ diff --git a/test/gtest/ucp/test_ucp_am.cc b/test/gtest/ucp/test_ucp_am.cc index f58f5c9e1c5..4ebf131b21f 100644 --- a/test/gtest/ucp/test_ucp_am.cc +++ b/test/gtest/ucp/test_ucp_am.cc @@ -41,7 +41,7 @@ class test_ucp_am_base : public ucp_test { ucp_test::init(); sender().connect(&receiver(), get_ep_params()); - receiver().connect(&sender(), get_ep_params()); + receiver().connect(&sender(), get_ep_params()); } }; diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc new file mode 100644 index 00000000000..b6c769084a7 --- /dev/null +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -0,0 +1,225 @@ +/** +* Copyright (C) Mellanox Technologies Ltd. 2001-2015. ALL RIGHTS RESERVED. +* +* See file LICENSE for terms. +*/ + +#include "ucp_test.h" +#include +#include + +extern "C" { +#include +#include +#include +} + + +class test_ucp_worker_discard : public ucp_test { +public: + static ucp_params_t get_ctx_params() { + ucp_params_t params = ucp_test::get_ctx_params(); + params.features |= UCP_FEATURE_TAG; + return params; + } + +protected: + void init() { + ucp_test::init(); + m_created_ep_count = 0; + m_destroyed_ep_count = 0; + m_flush_ep_count = 0; + m_pending_add_ep_count = 0; + + m_flush_comps.clear(); + m_pending_reqs.clear(); + } + + void test_worker_discard(void *ep_flush_func, + void *ep_pending_add_func, + unsigned ep_count = 8, + unsigned wireup_ep_count = 0, + unsigned wireup_aux_ep_count = 0) { + uct_iface_ops_t ops = {0}; + unsigned created_wireup_aux_ep_count = 0; + unsigned total_ep_count = ep_count + wireup_aux_ep_count; + uct_iface_t iface; + std::vector eps(total_ep_count); + std::vector wireup_eps(wireup_ep_count); + ucp_ep_t ucp_ep; + ucs_status_t status; + + ASSERT_LE(wireup_ep_count, ep_count); + ASSERT_LE(wireup_aux_ep_count, wireup_ep_count); + + ucp_ep.worker = sender().worker(); + + ops.ep_flush = (uct_ep_flush_func_t)ep_flush_func; + ops.ep_pending_add = (uct_ep_pending_add_func_t)ep_pending_add_func; + ops.ep_destroy = ep_destroy_func; + iface.ops = ops; + + for (unsigned i = 0; i < ep_count; i++) { + uct_ep_h discard_ep; + + eps[i].iface = &iface; + m_created_ep_count++; + + if (i < wireup_ep_count) { + status = ucp_wireup_ep_create(&ucp_ep, &discard_ep); + EXPECT_UCS_OK(status); + + wireup_eps.push_back(discard_ep); + ucp_wireup_ep_set_next_ep(discard_ep, &eps[i]); + + if (i < wireup_aux_ep_count) { + eps[ep_count + created_wireup_aux_ep_count].iface = &iface; + + ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(discard_ep); + /* coverity[escape] */ + wireup_ep->aux_ep = &eps[ep_count + + created_wireup_aux_ep_count]; + + created_wireup_aux_ep_count++; + m_created_ep_count++; + } + } else { + discard_ep = &eps[i]; + } + + EXPECT_LE(m_created_ep_count, total_ep_count); + + ucp_worker_discard_uct_ep(sender().worker(), discard_ep, + UCT_FLUSH_FLAG_LOCAL); + } + + do { + progress(); + + if (!m_flush_comps.empty()) { + uct_completion_t *comp = m_flush_comps.back(); + + m_flush_comps.pop_back(); + uct_invoke_completion(comp, UCS_OK); + } + + if (!m_pending_reqs.empty()) { + uct_pending_req_t *req = m_pending_reqs.back(); + + status = req->func(req); + if (status == UCS_OK) { + m_pending_reqs.pop_back(); + } else { + EXPECT_EQ(UCS_INPROGRESS, status); + } + } + } while (sender().worker()->flush_ops_count > 0); + + EXPECT_EQ(m_created_ep_count, total_ep_count); + EXPECT_TRUE(m_flush_comps.empty()); + EXPECT_TRUE(m_pending_reqs.empty()); + + /* check that uct_ep_destroy() was called for the all EPs that + * were created in the test */ + for (unsigned i = 0; i < created_wireup_aux_ep_count; i++) { + EXPECT_EQ(NULL, eps[i].iface); + } + } + + static void ep_destroy_func(uct_ep_h ep) { + ep->iface = NULL; + m_destroyed_ep_count++; + } + + static ucs_status_t + ep_flush_func_return_3_no_resource_then_ok(uct_ep_h ep, unsigned flags, + uct_completion_t *comp) { + EXPECT_LT(m_flush_ep_count, 4 * m_created_ep_count); + return (++m_flush_ep_count < 3 * m_created_ep_count) ? + UCS_ERR_NO_RESOURCE : UCS_OK; + } + + static ucs_status_t + ep_flush_func_return_in_progress(uct_ep_h ep, unsigned flags, + uct_completion_t *comp) { + EXPECT_LT(m_flush_ep_count, m_created_ep_count); + m_flush_comps.push_back(comp); + return UCS_INPROGRESS; + } + + static ucs_status_t + ep_pending_add_func_return_ok_then_busy(uct_ep_h ep, uct_pending_req_t *req, + unsigned flags) { + EXPECT_LT(m_pending_add_ep_count, 3 * m_created_ep_count); + + if (m_pending_add_ep_count < m_created_ep_count) { + m_pending_reqs.push_back(req); + return UCS_OK; + } + + return UCS_ERR_BUSY; + } + +protected: + static unsigned m_created_ep_count; + static unsigned m_destroyed_ep_count; + static unsigned m_flush_ep_count; + static unsigned m_pending_add_ep_count; + + static std::vector m_flush_comps; + static std::vector m_pending_reqs; +}; + +unsigned test_ucp_worker_discard::m_created_ep_count = 0; +unsigned test_ucp_worker_discard::m_destroyed_ep_count = 0; +unsigned test_ucp_worker_discard::m_flush_ep_count = 0; +unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; + +std::vector test_ucp_worker_discard::m_flush_comps; +std::vector test_ucp_worker_discard::m_pending_reqs; + +UCS_TEST_P(test_ucp_worker_discard, flush_ok) { + test_worker_discard((void*)ucs_empty_function_return_success, + (void*)ucs_empty_function_do_assert); +} + +UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok) { + test_worker_discard((void*)ucs_empty_function_return_success, + (void*)ucs_empty_function_do_assert, + 8, 6, 3); +} + +UCS_TEST_P(test_ucp_worker_discard, flush_in_progress) { + test_worker_discard((void*)ep_flush_func_return_in_progress, + (void*)ucs_empty_function_do_assert); +} + +UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_in_progress) { + test_worker_discard((void*)ep_flush_func_return_in_progress, + (void*)ucs_empty_function_do_assert, + 8, 6, 3); +} + +UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_busy) { + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, + (void*)ucs_empty_function_return_busy); +} + +UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_busy) { + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, + (void*)ucs_empty_function_return_busy, + 8, 6, 3); +} + +UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_ok_then_busy) { + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, + (void*)ep_pending_add_func_return_ok_then_busy); +} + +UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_ok_then_busy) { + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, + (void*)ep_pending_add_func_return_ok_then_busy, + 8, 6, 3); +} + +UCP_INSTANTIATE_TEST_CASE_TLS(test_ucp_worker_discard, all, "all") From 627a8a5f7d84229cbf32765fffd1807c64d003bb Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 26 Aug 2020 08:57:01 +0000 Subject: [PATCH 02/33] UCP/GTEST: Complete worker_flus operation if there is no flush ops and all EPs flushed --- src/ucp/rma/flush.c | 11 ++++++----- test/gtest/ucp/test_ucp_worker.cc | 21 +++++++++++++++------ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index d2da3d5400b..9d5a73ee586 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -383,7 +383,7 @@ static ucs_status_t ucp_worker_flush_check(ucp_worker_h worker) ucp_worker_iface_t *wiface; ucs_status_t status; - if (worker->flush_ops_count) { + if (worker->flush_ops_count != 0) { return UCS_INPROGRESS; } @@ -444,16 +444,17 @@ static unsigned ucp_worker_flush_progress(void *arg) ucp_ep_h ep; status = ucp_worker_flush_check(worker); - if ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps)) { + if ((status == UCS_OK) && (&next_ep->ep_list == &worker->all_eps)) { /* If all ifaces are flushed, or we finished going over all endpoints, - * no need to progress this request actively any more. Just wait until - * all associated endpoint flush requests are completed. + * and all scheduled operations on worker were completed or iface flush + * failed with error, no need to progress this request actively anymore. */ ucp_worker_flush_complete_one(req, UCS_OK, 1); } else if (status != UCS_INPROGRESS) { /* Error returned from uct iface flush */ ucp_worker_flush_complete_one(req, status, 1); - } else if (worker->context->config.ext.flush_worker_eps) { + } else if ((worker->context->config.ext.flush_worker_eps) && + (&next_ep->ep_list != &worker->all_eps)) { /* Some endpoints are not flushed yet. Take next endpoint from the list * and start flush operation on it. */ diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index b6c769084a7..5524000584a 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -1,8 +1,8 @@ /** -* Copyright (C) Mellanox Technologies Ltd. 2001-2015. ALL RIGHTS RESERVED. -* -* See file LICENSE for terms. -*/ + * Copyright (C) Mellanox Technologies Ltd. 2020. ALL RIGHTS RESERVED. + * + * See file LICENSE for terms. + */ #include "ucp_test.h" #include @@ -67,7 +67,7 @@ class test_ucp_worker_discard : public ucp_test { if (i < wireup_ep_count) { status = ucp_wireup_ep_create(&ucp_ep, &discard_ep); - EXPECT_UCS_OK(status); + ASSERT_UCS_OK(status); wireup_eps.push_back(discard_ep); ucp_wireup_ep_set_next_ep(discard_ep, &eps[i]); @@ -93,6 +93,11 @@ class test_ucp_worker_discard : public ucp_test { UCT_FLUSH_FLAG_LOCAL); } + void *flush_req = sender().flush_worker_nb(0); + + ASSERT_FALSE(flush_req == NULL); + ASSERT_TRUE(UCS_PTR_IS_PTR(flush_req)); + do { progress(); @@ -113,12 +118,16 @@ class test_ucp_worker_discard : public ucp_test { EXPECT_EQ(UCS_INPROGRESS, status); } } - } while (sender().worker()->flush_ops_count > 0); + } while (ucp_request_check_status(flush_req) == UCS_INPROGRESS); + EXPECT_UCS_OK(ucp_request_check_status(flush_req)); + EXPECT_EQ(m_created_ep_count, m_destroyed_ep_count); EXPECT_EQ(m_created_ep_count, total_ep_count); EXPECT_TRUE(m_flush_comps.empty()); EXPECT_TRUE(m_pending_reqs.empty()); + ucp_request_release(flush_req); + /* check that uct_ep_destroy() was called for the all EPs that * were created in the test */ for (unsigned i = 0; i < created_wireup_aux_ep_count; i++) { From e9d3b1db4391cd51f5ed77ec7eac5533b55510ab Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 26 Aug 2020 13:16:12 +0000 Subject: [PATCH 03/33] UCT/IB: Fix uct_iface_flush()/uct_ep_flush(LOCAL) after uct_ep_flush(CANCEL) --- src/uct/ib/ud/base/ud_ep.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/uct/ib/ud/base/ud_ep.c b/src/uct/ib/ud/base/ud_ep.c index c34ad4070a7..f53cf304b1b 100644 --- a/src/uct/ib/ud/base/ud_ep.c +++ b/src/uct/ib/ud/base/ud_ep.c @@ -933,6 +933,19 @@ ucs_status_t uct_ud_ep_flush_nolock(uct_ud_iface_t *iface, uct_ud_ep_t *ep, return UCS_OK; /* Nothing was ever sent */ } + /* The check for emptiness of TX window must go before checking EP window, + * since in case of error flow, when EP is flushed with the `CANCEL` flag, + * `uct_ud_ep_no_window()` returns true, but TX window is empty. So, it + * leads that `uct_iface_flush()`/`uct_ep_flush(LOCAL)` aren't completed + * after `uct_ep_flush(CANCEL)`. + */ + if (ucs_queue_is_empty(&ep->tx.window) && + ucs_queue_is_empty(&iface->tx.async_comp_q)) { + /* No outstanding operations */ + ucs_assert(ep->tx.resend_count == 0); + return UCS_OK; + } + if (!uct_ud_iface_can_tx(iface) || !uct_ud_iface_has_skbs(iface) || uct_ud_ep_no_window(ep)) { @@ -942,13 +955,6 @@ ucs_status_t uct_ud_ep_flush_nolock(uct_ud_iface_t *iface, uct_ud_ep_t *ep, return UCS_ERR_NO_RESOURCE; } - if (ucs_queue_is_empty(&ep->tx.window) && - ucs_queue_is_empty(&iface->tx.async_comp_q)) { - /* No outstanding operations */ - ucs_assert(ep->tx.resend_count == 0); - return UCS_OK; - } - /* Expedite acknowledgment on the last skb in the window */ if (uct_ud_ep_is_last_ack_received(ep)) { uct_ud_ep_ctl_op_del(ep, UCT_UD_EP_OP_ACK_REQ); From ecd86639ca55d2c285327b9eb321f77e0ea42e3e Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 26 Aug 2020 17:12:07 +0000 Subject: [PATCH 04/33] UCT/IB/UD: Reset max_psn instead of stopping TX --- src/ucm/ptmalloc286/malloc.c | 2 +- src/uct/ib/ud/base/ud_ep.c | 28 +++++++++++++--------------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/ucm/ptmalloc286/malloc.c b/src/ucm/ptmalloc286/malloc.c index e1779967885..40336b2fb33 100644 --- a/src/ucm/ptmalloc286/malloc.c +++ b/src/ucm/ptmalloc286/malloc.c @@ -1810,7 +1810,7 @@ static FORCEINLINE int win32munmap(void* ptr, size_t size) { #if !USE_LOCKS #define USE_LOCK_BIT (0U) -#define INITIAL_LOCK(l) (0) +()#define INITIAL_LOCK(l) (0) #define DESTROY_LOCK(l) (0) #define ACQUIRE_MALLOC_GLOBAL_LOCK() #define RELEASE_MALLOC_GLOBAL_LOCK() diff --git a/src/uct/ib/ud/base/ud_ep.c b/src/uct/ib/ud/base/ud_ep.c index f53cf304b1b..9d105799a77 100644 --- a/src/uct/ib/ud/base/ud_ep.c +++ b/src/uct/ib/ud/base/ud_ep.c @@ -109,6 +109,10 @@ static UCS_F_ALWAYS_INLINE void uct_ud_ep_ca_ack(uct_ud_ep_t *ep) ep->tx.max_psn = ep->tx.acked_psn + ep->ca.cwnd; } +static void uct_ud_ep_reset_max_psn(uct_ud_ep_t *ep) +{ + ep->tx.max_psn = ep->tx.psn + ep->ca.cwnd; +} static void uct_ud_ep_reset(uct_ud_ep_t *ep) { @@ -116,9 +120,9 @@ static void uct_ud_ep_reset(uct_ud_ep_t *ep) ep->ca.cwnd = UCT_UD_CA_MIN_WINDOW; ep->ca.wmax = ucs_derived_of(ep->super.super.iface, uct_ud_iface_t)->config.max_window; - ep->tx.max_psn = ep->tx.psn + ep->ca.cwnd; ep->tx.acked_psn = UCT_UD_INITIAL_PSN - 1; ep->tx.pending.ops = UCT_UD_EP_OP_NONE; + uct_ud_ep_reset_max_psn(ep); ucs_queue_head_init(&ep->tx.window); ep->resend.pos = ucs_queue_iter_begin(&ep->tx.window); @@ -221,7 +225,7 @@ static void uct_ud_ep_purge_outstanding(uct_ud_ep_t *ep) static void uct_ud_ep_purge(uct_ud_ep_t *ep, ucs_status_t status) { - uct_ud_ep_tx_stop(ep); + uct_ud_ep_reset_max_psn(ep); uct_ud_ep_purge_outstanding(ep); ep->tx.acked_psn = (uct_ud_psn_t)(ep->tx.psn - 1); uct_ud_ep_window_release(ep, status, 0); @@ -933,19 +937,6 @@ ucs_status_t uct_ud_ep_flush_nolock(uct_ud_iface_t *iface, uct_ud_ep_t *ep, return UCS_OK; /* Nothing was ever sent */ } - /* The check for emptiness of TX window must go before checking EP window, - * since in case of error flow, when EP is flushed with the `CANCEL` flag, - * `uct_ud_ep_no_window()` returns true, but TX window is empty. So, it - * leads that `uct_iface_flush()`/`uct_ep_flush(LOCAL)` aren't completed - * after `uct_ep_flush(CANCEL)`. - */ - if (ucs_queue_is_empty(&ep->tx.window) && - ucs_queue_is_empty(&iface->tx.async_comp_q)) { - /* No outstanding operations */ - ucs_assert(ep->tx.resend_count == 0); - return UCS_OK; - } - if (!uct_ud_iface_can_tx(iface) || !uct_ud_iface_has_skbs(iface) || uct_ud_ep_no_window(ep)) { @@ -955,6 +946,13 @@ ucs_status_t uct_ud_ep_flush_nolock(uct_ud_iface_t *iface, uct_ud_ep_t *ep, return UCS_ERR_NO_RESOURCE; } + if (ucs_queue_is_empty(&ep->tx.window) && + ucs_queue_is_empty(&iface->tx.async_comp_q)) { + /* No outstanding operations */ + ucs_assert(ep->tx.resend_count == 0); + return UCS_OK; + } + /* Expedite acknowledgment on the last skb in the window */ if (uct_ud_ep_is_last_ack_received(ep)) { uct_ud_ep_ctl_op_del(ep, UCT_UD_EP_OP_ACK_REQ); From 8d4a9f9c627e8525b0e0893015564eac4184d584 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 27 Aug 2020 04:38:11 +0000 Subject: [PATCH 05/33] UCM/GTEST: Fix leftovers --- src/ucm/ptmalloc286/malloc.c | 2 +- test/gtest/ucp/test_ucp_am.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ucm/ptmalloc286/malloc.c b/src/ucm/ptmalloc286/malloc.c index 40336b2fb33..e1779967885 100644 --- a/src/ucm/ptmalloc286/malloc.c +++ b/src/ucm/ptmalloc286/malloc.c @@ -1810,7 +1810,7 @@ static FORCEINLINE int win32munmap(void* ptr, size_t size) { #if !USE_LOCKS #define USE_LOCK_BIT (0U) -()#define INITIAL_LOCK(l) (0) +#define INITIAL_LOCK(l) (0) #define DESTROY_LOCK(l) (0) #define ACQUIRE_MALLOC_GLOBAL_LOCK() #define RELEASE_MALLOC_GLOBAL_LOCK() diff --git a/test/gtest/ucp/test_ucp_am.cc b/test/gtest/ucp/test_ucp_am.cc index 4ebf131b21f..f58f5c9e1c5 100644 --- a/test/gtest/ucp/test_ucp_am.cc +++ b/test/gtest/ucp/test_ucp_am.cc @@ -41,7 +41,7 @@ class test_ucp_am_base : public ucp_test { ucp_test::init(); sender().connect(&receiver(), get_ep_params()); - receiver().connect(&sender(), get_ep_params()); + receiver().connect(&sender(), get_ep_params()); } }; From d229eae06215b35d67d7db3437e5a570ddefac8e Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 27 Aug 2020 19:24:22 +0000 Subject: [PATCH 06/33] UCP/WORKER: Introduce khash to find whether UCT EP there or not --- src/ucp/core/ucp_worker.c | 56 ++++++++++++++++++--- src/ucp/core/ucp_worker.h | 103 ++++++++++++++++++++------------------ 2 files changed, 105 insertions(+), 54 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index ab0bdbbaab6..bfae8f478ea 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -102,6 +102,14 @@ ucs_mpool_ops_t ucp_frag_mpool_ops = { }; +#define ucp_worker_discard_uct_ep_hash_key(_uct_ep) \ + kh_int64_hash_func((uintptr_t)(_uct_ep)) + + +KHASH_IMPL(ucp_worker_discard_uct_ep_hash, uct_ep_h, char, 0, + ucp_worker_discard_uct_ep_hash_key, kh_int64_hash_equal); + + static ucs_status_t ucp_worker_wakeup_ctl_fd(ucp_worker_h worker, ucp_worker_event_fd_op_t op, int event_fd) @@ -617,15 +625,16 @@ ucs_status_t ucp_worker_set_ep_failed(ucp_worker_h worker, ucp_ep_h ucp_ep, static ucs_status_t ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) { - ucp_worker_h worker = (ucp_worker_h)arg; + ucp_worker_h worker = (ucp_worker_h)arg; ucp_lane_index_t lane; ucs_status_t ret_status; ucp_ep_ext_gen_t *ep_ext; ucp_ep_h ucp_ep; + khiter_t iter; UCS_ASYNC_BLOCK(&worker->async); - ucs_debug("worker %p: error handler called for uct_ep %p: %s", + ucs_debug("worker %p: error handler called for UCT EP %p: %s", worker, uct_ep, ucs_status_string(status)); /* TODO: need to optimize uct_ep -> ucp_ep lookup */ @@ -642,11 +651,21 @@ ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) } } - ucs_error("no uct_ep_h %p associated with ucp_ep_h on ucp_worker_h %p", - uct_ep, worker); + iter = kh_get(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash, uct_ep); + if (iter != kh_end(&worker->discard_uct_ep_hash)) { + ucs_debug("UCT EP %p is being discarded on UCP Worker %p", + uct_ep, worker); + ret_status = UCS_OK; + } else { + ucs_error("no UCT EP %p associated with UCP EP on UCP Worker %p", + uct_ep, worker); + ret_status = UCS_ERR_NO_ELEM; + } + UCS_ASYNC_UNBLOCK(&worker->async); - return UCS_ERR_NO_ELEM; + return ret_status; } void ucp_worker_iface_activate(ucp_worker_iface_t *wiface, unsigned uct_flags) @@ -1825,6 +1844,7 @@ ucs_status_t ucp_worker_create(ucp_context_h context, ucs_conn_match_init(&worker->conn_match_ctx, sizeof(uint64_t), &ucp_ep_match_ops); kh_init_inplace(ucp_worker_rkey_config, &worker->rkey_config_hash); + kh_init_inplace(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash); UCS_STATIC_ASSERT(sizeof(ucp_ep_ext_gen_t) <= sizeof(ucp_ep_t)); if (context->config.features & (UCP_FEATURE_STREAM | UCP_FEATURE_AM)) { @@ -2028,6 +2048,8 @@ void ucp_worker_destroy(ucp_worker_h worker) uct_worker_destroy(worker->uct); ucs_async_context_cleanup(&worker->async); ucs_conn_match_cleanup(&worker->conn_match_ctx); + kh_destroy_inplace(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash); kh_destroy_inplace(ucp_worker_rkey_config, &worker->rkey_config_hash); ucs_ptr_map_destroy(&worker->ptr_map); ucs_strided_alloc_cleanup(&worker->ep_alloc); @@ -2356,12 +2378,22 @@ static unsigned ucp_worker_discard_uct_ep_destroy_progress(void *arg) ucp_request_t *req = (ucp_request_t*)arg; uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; + khiter_t iter; ucp_trace_req(req, "destroy uct_ep=%p", uct_ep); - uct_ep_destroy(uct_ep); ucp_request_put(req); + UCS_ASYNC_BLOCK(&worker->async); + uct_ep_destroy(uct_ep); --worker->flush_ops_count; + iter = kh_get(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash, uct_ep); + if (iter == kh_end(&worker->discard_uct_ep_hash)) { + ucs_fatal("no %p UCT EP in the %p worker hash of discarded UCT EPs", + uct_ep, worker); + } + kh_del(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash, iter); UCS_ASYNC_UNBLOCK(&worker->async); return 1; @@ -2428,7 +2460,9 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; ucp_wireup_ep_t *wireup_ep; ucp_request_t *req; + khiter_t UCS_V_UNUSED iter; int is_owner; + int ret; ucs_assert(uct_ep != NULL); @@ -2465,6 +2499,16 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, } ++worker->flush_ops_count; + iter = kh_put(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash, + uct_ep, &ret); + if (ret == UCS_KH_PUT_FAILED) { + ucs_fatal("failed to put %p UCT EP into the %p worker hash", + uct_ep, worker); + } else if (ret == UCS_KH_PUT_KEY_PRESENT) { + ucs_fatal("%p UCT EP is already present in the %p worker hash", + uct_ep, worker); + } ucs_assert(!ucp_wireup_ep_test(uct_ep)); req->send.uct.func = ucp_worker_discard_uct_ep_pending_cb; diff --git a/src/ucp/core/ucp_worker.h b/src/ucp/core/ucp_worker.h index 51dc4a97a06..3ddec736451 100644 --- a/src/ucp/core/ucp_worker.h +++ b/src/ucp/core/ucp_worker.h @@ -172,6 +172,11 @@ KHASH_TYPE(ucp_worker_rkey_config, ucp_rkey_config_key_t, ucp_worker_cfg_index_t typedef khash_t(ucp_worker_rkey_config) ucp_worker_rkey_config_hash_t; +/* Hash set to UCT EPs that are being discarded on UCP Worker */ +KHASH_TYPE(ucp_worker_discard_uct_ep_hash, uct_ep_h, char); +typedef khash_t(ucp_worker_discard_uct_ep_hash) ucp_worker_discard_uct_ep_hash_t; + + /** * UCP worker iface, which encapsulates UCT iface, its attributes and * some auxiliary info needed for tag matching offloads. @@ -207,61 +212,63 @@ struct ucp_worker_cm { * UCP worker (thread context). */ typedef struct ucp_worker { - unsigned flags; /* Worker flags */ - ucs_async_context_t async; /* Async context for this worker */ - ucp_context_h context; /* Back-reference to UCP context */ - uint64_t uuid; /* Unique ID for wireup */ - uct_worker_h uct; /* UCT worker handle */ - ucs_mpool_t req_mp; /* Memory pool for requests */ - ucs_mpool_t rkey_mp; /* Pool for small memory keys */ - uint64_t atomic_tls; /* Which resources can be used for atomics */ - - int inprogress; - char name[UCP_WORKER_NAME_MAX]; /* Worker name */ - - unsigned flush_ops_count;/* Number of pending operations */ - - int event_fd; /* Allocated (on-demand) event fd for wakeup */ - ucs_sys_event_set_t *event_set; /* Allocated UCS event set for wakeup */ - int eventfd; /* Event fd to support signal() calls */ - unsigned uct_events; /* UCT arm events */ - ucs_list_link_t arm_ifaces; /* List of interfaces to arm */ - - void *user_data; /* User-defined data */ - ucs_strided_alloc_t ep_alloc; /* Endpoint allocator */ - ucs_list_link_t stream_ready_eps; /* List of EPs with received stream data */ - ucs_list_link_t all_eps; /* List of all endpoints */ - ucs_conn_match_ctx_t conn_match_ctx; /* Endpoint-to-endpoint matching context */ - ucp_worker_iface_t **ifaces; /* Array of pointers to interfaces, - one for each resource */ - unsigned num_ifaces; /* Number of elements in ifaces array */ - unsigned num_active_ifaces; /* Number of activated ifaces */ - uint64_t scalable_tl_bitmap; /* Map of scalable tl resources */ - ucp_worker_cm_t *cms; /* Array of CMs, one for each component */ - ucs_mpool_t am_mp; /* Memory pool for AM receives */ - ucs_mpool_t reg_mp; /* Registered memory pool */ - ucs_mpool_t rndv_frag_mp; /* Memory pool for RNDV fragments */ - ucs_queue_head_t rkey_ptr_reqs; /* Queue of submitted RKEY PTR requests that - * are in-progress */ - uct_worker_cb_id_t rkey_ptr_cb_id;/* RKEY PTR worker callback queue ID */ - ucp_tag_match_t tm; /* Tag-matching queues and offload info */ - ucs_array_t(ucp_am_cbs) am; /* Array of AM callbacks and their data */ - uint64_t am_message_id; /* For matching long AMs */ - ucp_ep_h mem_type_ep[UCS_MEMORY_TYPE_LAST];/* memory type eps */ + unsigned flags; /* Worker flags */ + ucs_async_context_t async; /* Async context for this worker */ + ucp_context_h context; /* Back-reference to UCP context */ + uint64_t uuid; /* Unique ID for wireup */ + uct_worker_h uct; /* UCT worker handle */ + ucs_mpool_t req_mp; /* Memory pool for requests */ + ucs_mpool_t rkey_mp; /* Pool for small memory keys */ + uint64_t atomic_tls; /* Which resources can be used for atomics */ + + int inprogress; + char name[UCP_WORKER_NAME_MAX]; /* Worker name */ + + unsigned flush_ops_count; /* Number of pending operations */ + + int event_fd; /* Allocated (on-demand) event fd for wakeup */ + ucs_sys_event_set_t *event_set; /* Allocated UCS event set for wakeup */ + int eventfd; /* Event fd to support signal() calls */ + unsigned uct_events; /* UCT arm events */ + ucs_list_link_t arm_ifaces; /* List of interfaces to arm */ + + void *user_data; /* User-defined data */ + ucs_strided_alloc_t ep_alloc; /* Endpoint allocator */ + ucs_list_link_t stream_ready_eps; /* List of EPs with received stream data */ + ucs_list_link_t all_eps; /* List of all endpoints */ + ucs_conn_match_ctx_t conn_match_ctx; /* Endpoint-to-endpoint matching context */ + ucp_worker_iface_t **ifaces; /* Array of pointers to interfaces, + one for each resource */ + unsigned num_ifaces; /* Number of elements in ifaces array */ + unsigned num_active_ifaces; /* Number of activated ifaces */ + uint64_t scalable_tl_bitmap; /* Map of scalable tl resources */ + ucp_worker_cm_t *cms; /* Array of CMs, one for each component */ + ucs_mpool_t am_mp; /* Memory pool for AM receives */ + ucs_mpool_t reg_mp; /* Registered memory pool */ + ucs_mpool_t rndv_frag_mp; /* Memory pool for RNDV fragments */ + ucs_queue_head_t rkey_ptr_reqs; /* Queue of submitted RKEY PTR requests that + * are in-progress */ + uct_worker_cb_id_t rkey_ptr_cb_id; /* RKEY PTR worker callback queue ID */ + ucp_tag_match_t tm; /* Tag-matching queues and offload info */ + ucs_array_t(ucp_am_cbs) am; /* Array of AM callbacks and their data */ + uint64_t am_message_id; /* For matching long AMs */ + ucp_ep_h mem_type_ep[UCS_MEMORY_TYPE_LAST]; /* Memory type EPs */ UCS_STATS_NODE_DECLARE(stats) UCS_STATS_NODE_DECLARE(tm_offload_stats) - ucs_cpu_set_t cpu_mask; /* Save CPU mask for subsequent calls to ucp_worker_listen */ + ucs_cpu_set_t cpu_mask; /* Save CPU mask for subsequent calls to + ucp_worker_listen */ - ucp_worker_rkey_config_hash_t rkey_config_hash; /* rkey config key -> index */ - ucs_ptr_map_t ptr_map; /* UCP objects key to ptr mapping */ + ucp_worker_rkey_config_hash_t rkey_config_hash; /* RKEY config key -> index */ + ucp_worker_discard_uct_ep_hash_t discard_uct_ep_hash; /* Hash of discarded UCT EPs */ + ucs_ptr_map_t ptr_map; /* UCP objects key to ptr mapping */ - unsigned ep_config_count; /* Current number of ep configurations */ - ucp_ep_config_t ep_config[UCP_WORKER_MAX_EP_CONFIG]; + unsigned ep_config_count; /* Current number of ep configurations */ + ucp_ep_config_t ep_config[UCP_WORKER_MAX_EP_CONFIG]; - unsigned rkey_config_count; /* Current number of rkey configurations */ - ucp_rkey_config_t rkey_config[UCP_WORKER_MAX_RKEY_CONFIG]; + unsigned rkey_config_count; /* Current number of rkey configurations */ + ucp_rkey_config_t rkey_config[UCP_WORKER_MAX_RKEY_CONFIG]; } ucp_worker_t; From 8558d4caba18df70b71afc21ab4d8e255f859fd5 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Fri, 28 Aug 2020 05:11:52 +0000 Subject: [PATCH 07/33] UCP/CORE: Fix Coverity issue --- src/ucp/core/ucp_worker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index bfae8f478ea..9b97c4caa38 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2384,7 +2384,6 @@ static unsigned ucp_worker_discard_uct_ep_destroy_progress(void *arg) ucp_request_put(req); UCS_ASYNC_BLOCK(&worker->async); - uct_ep_destroy(uct_ep); --worker->flush_ops_count; iter = kh_get(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, uct_ep); @@ -2394,6 +2393,7 @@ static unsigned ucp_worker_discard_uct_ep_destroy_progress(void *arg) } kh_del(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, iter); + uct_ep_destroy(uct_ep); UCS_ASYNC_UNBLOCK(&worker->async); return 1; From cdfe88d269860d0689074ea764d06a01a29ff9e6 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Fri, 28 Aug 2020 17:07:56 +0000 Subject: [PATCH 08/33] UCP/UCT: Fix review comments --- src/ucp/core/ucp_worker.c | 3 ++- src/uct/ib/ud/base/ud_ep.c | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 9b97c4caa38..a08dd23a192 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -658,7 +658,8 @@ ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) uct_ep, worker); ret_status = UCS_OK; } else { - ucs_error("no UCT EP %p associated with UCP EP on UCP Worker %p", + ucs_error("UCT EP %p isn't associated with UCP EP and was not scheduled " + "to be discarded on UCP Worker %p", uct_ep, worker); ret_status = UCS_ERR_NO_ELEM; } diff --git a/src/uct/ib/ud/base/ud_ep.c b/src/uct/ib/ud/base/ud_ep.c index 9d105799a77..828e91b31c8 100644 --- a/src/uct/ib/ud/base/ud_ep.c +++ b/src/uct/ib/ud/base/ud_ep.c @@ -225,6 +225,9 @@ static void uct_ud_ep_purge_outstanding(uct_ud_ep_t *ep) static void uct_ud_ep_purge(uct_ud_ep_t *ep, ucs_status_t status) { + /* reset the maximal TX psn value to the default, since we should be able + * to do TX operation after purging of the EP and uct_ep_flush(LOCAL) + * operation has to return UCS_OK */ uct_ud_ep_reset_max_psn(ep); uct_ud_ep_purge_outstanding(ep); ep->tx.acked_psn = (uct_ud_psn_t)(ep->tx.psn - 1); From c1a8feec14df4e8ca705eb9219b3a22d75140cfc Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Mon, 31 Aug 2020 21:26:51 +0300 Subject: [PATCH 09/33] UCP/CORE: Fix review comments --- src/ucp/core/ucp_worker.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index a08dd23a192..a344982ac19 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2394,9 +2394,10 @@ static unsigned ucp_worker_discard_uct_ep_destroy_progress(void *arg) } kh_del(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, iter); - uct_ep_destroy(uct_ep); UCS_ASYNC_UNBLOCK(&worker->async); + uct_ep_destroy(uct_ep); + return 1; } @@ -2419,10 +2420,14 @@ ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); } -static unsigned ucp_worker_discard_uct_ep_progress(void *arg) +/* Forward declaration to use in the pending callback */ +static unsigned ucp_worker_discard_uct_ep_progress(void *arg); + +static ucs_status_t +ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { + ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; - ucp_request_t *req = (ucp_request_t*)arg; uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; ucs_status_t status; @@ -2437,21 +2442,25 @@ static unsigned ucp_worker_discard_uct_ep_progress(void *arg) ucp_worker_discard_uct_ep_progress, req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); - return 0; + } else { + ucs_assert(status == UCS_OK); } - ucs_assert(status == UCS_OK); + + return UCS_ERR_NO_RESOURCE; } else if (status != UCS_INPROGRESS) { ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); + return status; } - return 1; + return UCS_OK; } -static ucs_status_t -ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) +static unsigned ucp_worker_discard_uct_ep_progress(void *arg) { - ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); - return ucp_worker_discard_uct_ep_progress(req) ? UCS_OK : UCS_INPROGRESS; + ucp_request_t *req = (ucp_request_t*)arg; + ucs_status_t status = ucp_worker_discard_uct_ep_pending_cb(&req->send.uct); + + return !UCS_STATUS_IS_ERR(status); } /* must be called with async lock held */ From 28edf6759f65e080ab1689a64be77d40e4df082d Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Mon, 31 Aug 2020 21:44:10 +0300 Subject: [PATCH 10/33] UCP/CORE: Fix EP-by-EP flush when iface_flush returns NO_RESOURCE --- src/ucp/rma/flush.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index 9d5a73ee586..bec2a2a6d0c 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -377,16 +377,18 @@ UCS_PROFILE_FUNC(ucs_status_ptr_t, ucp_ep_flush_nbx, (ep, param), return request; } +static UCS_F_ALWAYS_INLINE int +ucp_worker_flush_ops_count_check(ucp_worker_h worker) +{ + return !worker->flush_ops_count; +} + static ucs_status_t ucp_worker_flush_check(ucp_worker_h worker) { ucp_rsc_index_t iface_id; ucp_worker_iface_t *wiface; ucs_status_t status; - if (worker->flush_ops_count != 0) { - return UCS_INPROGRESS; - } - for (iface_id = 0; iface_id < worker->num_ifaces; ++iface_id) { wiface = worker->ifaces[iface_id]; if (wiface->iface == NULL) { @@ -444,7 +446,8 @@ static unsigned ucp_worker_flush_progress(void *arg) ucp_ep_h ep; status = ucp_worker_flush_check(worker); - if ((status == UCS_OK) && (&next_ep->ep_list == &worker->all_eps)) { + if (ucp_worker_flush_ops_count_check(worker) && + ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps))) { /* If all ifaces are flushed, or we finished going over all endpoints, * and all scheduled operations on worker were completed or iface flush * failed with error, no need to progress this request actively anymore. @@ -487,9 +490,12 @@ ucp_worker_flush_nbx_internal(ucp_worker_h worker, ucs_status_t status; ucp_request_t *req; - status = ucp_worker_flush_check(worker); - if ((status != UCS_INPROGRESS) && (status != UCS_ERR_NO_RESOURCE)) { - return UCS_STATUS_PTR(status); + if (ucp_worker_flush_ops_count_check(worker)) { + status = ucp_worker_flush_check(worker); + if ((status != UCS_INPROGRESS) && (status != UCS_ERR_NO_RESOURCE)) { + /* UCS_OK could be returned here */ + return UCS_STATUS_PTR(status); + } } req = ucp_request_get_param(worker, param, From 022a6f246a3071dc9f96716ff9375512b3d27b31 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 1 Sep 2020 05:47:48 +0000 Subject: [PATCH 11/33] UCP/RMA/FLUSH: Fix flush ops count check usage --- src/ucp/rma/flush.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index bec2a2a6d0c..116d0e88ebc 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -445,17 +445,19 @@ static unsigned ucp_worker_flush_progress(void *arg) ucs_status_t status; ucp_ep_h ep; - status = ucp_worker_flush_check(worker); - if (ucp_worker_flush_ops_count_check(worker) && - ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps))) { - /* If all ifaces are flushed, or we finished going over all endpoints, - * and all scheduled operations on worker were completed or iface flush - * failed with error, no need to progress this request actively anymore. - */ - ucp_worker_flush_complete_one(req, UCS_OK, 1); - } else if (status != UCS_INPROGRESS) { - /* Error returned from uct iface flush */ - ucp_worker_flush_complete_one(req, status, 1); + if (ucp_worker_flush_ops_count_check(worker)) { + status = ucp_worker_flush_check(worker); + if ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps)) { + /* If all ifaces are flushed, or we finished going over all + * endpoints, and all scheduled operations on worker were + * completed or iface flush failed with error, no need to + * progress this request actively anymore. + */ + ucp_worker_flush_complete_one(req, UCS_OK, 1); + } else if (status != UCS_INPROGRESS) { + /* Error returned from uct iface flush */ + ucp_worker_flush_complete_one(req, status, 1); + } } else if ((worker->context->config.ext.flush_worker_eps) && (&next_ep->ep_list != &worker->all_eps)) { /* Some endpoints are not flushed yet. Take next endpoint from the list From 46b12725fd25b751a58a6679cdab95ffb35977e7 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 2 Sep 2020 12:48:48 +0000 Subject: [PATCH 12/33] UCP/CORE: Fix tests --- src/ucp/core/ucp_worker.c | 29 +++++++++++++++-------------- src/ucp/rma/flush.c | 9 +++++++-- test/gtest/ucp/test_ucp_worker.cc | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index a344982ac19..251870b7afc 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2426,33 +2426,34 @@ static unsigned ucp_worker_discard_uct_ep_progress(void *arg); static ucs_status_t ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { - ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; - ucs_status_t status; + ucs_status_t status, status_add; status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, &req->send.state.uct_comp); if (status == UCS_ERR_NO_RESOURCE) { - status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); - if (status == UCS_ERR_BUSY) { - /* try again to flush UCT EP */ + status_add = uct_ep_pending_add(uct_ep, &req->send.uct, 0); + ucs_assert((status_add == UCS_ERR_BUSY) || (status_add == UCS_OK)); + /* if added to the pending queue or to worker progress, need to remove + * the callback to not invoke it several times */ + if (status_add == UCS_ERR_BUSY) { uct_worker_progress_register_safe(worker->uct, - ucp_worker_discard_uct_ep_progress, - req, UCS_CALLBACKQ_FLAG_ONESHOT, - &cb_id); - } else { - ucs_assert(status == UCS_OK); + ucp_worker_discard_uct_ep_destroy_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); } - return UCS_ERR_NO_RESOURCE; - } else if (status != UCS_INPROGRESS) { + return UCS_OK; + } if (status == UCS_INPROGRESS) { + /* need to remove from the pending queue */ + status = UCS_OK; + } else { ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); - return status; } - return UCS_OK; + return status; } static unsigned ucp_worker_discard_uct_ep_progress(void *arg) diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index 116d0e88ebc..fe830742aba 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -454,12 +454,16 @@ static unsigned ucp_worker_flush_progress(void *arg) * progress this request actively anymore. */ ucp_worker_flush_complete_one(req, UCS_OK, 1); + goto out; } else if (status != UCS_INPROGRESS) { /* Error returned from uct iface flush */ ucp_worker_flush_complete_one(req, status, 1); + goto out; } - } else if ((worker->context->config.ext.flush_worker_eps) && - (&next_ep->ep_list != &worker->all_eps)) { + } + + if ((worker->context->config.ext.flush_worker_eps) && + (&next_ep->ep_list != &worker->all_eps)) { /* Some endpoints are not flushed yet. Take next endpoint from the list * and start flush operation on it. */ @@ -482,6 +486,7 @@ static unsigned ucp_worker_flush_progress(void *arg) } } +out: return 0; } diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 5524000584a..3eef49e6471 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -115,7 +115,7 @@ class test_ucp_worker_discard : public ucp_test { if (status == UCS_OK) { m_pending_reqs.pop_back(); } else { - EXPECT_EQ(UCS_INPROGRESS, status); + EXPECT_EQ(UCS_ERR_NO_RESOURCE, status); } } } while (ucp_request_check_status(flush_req) == UCS_INPROGRESS); From cb197a413f99b7b649075e176d9668da71830e35 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 2 Sep 2020 19:04:31 +0000 Subject: [PATCH 13/33] UCP/CORE: Implemented purge --- src/ucp/core/ucp_worker.c | 26 +++++-- src/ucp/core/ucp_worker.h | 4 +- test/gtest/ucp/test_ucp_worker.cc | 114 ++++++++++++++++++++++++++---- 3 files changed, 123 insertions(+), 21 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 251870b7afc..a9be2e3ff24 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -447,15 +447,18 @@ static unsigned ucp_worker_iface_err_handle_progress(void *arg) /* Purge pending queue */ ucs_trace("ep %p: purge pending on uct_ep[%d]=%p", ucp_ep, lane, ucp_ep->uct_eps[lane]); - uct_ep_pending_purge(ucp_ep->uct_eps[lane], ucp_ep_err_pending_purge, - UCS_STATUS_PTR(status)); if (lane != failed_lane) { ucs_trace("ep %p: destroy uct_ep[%d]=%p", ucp_ep, lane, ucp_ep->uct_eps[lane]); ucp_worker_discard_uct_ep(ucp_ep->worker, ucp_ep->uct_eps[lane], - UCT_FLUSH_FLAG_CANCEL); + UCT_FLUSH_FLAG_CANCEL, + ucp_ep_err_pending_purge, + UCS_STATUS_PTR(status)); ucp_ep->uct_eps[lane] = NULL; + } else { + uct_ep_pending_purge(ucp_ep->uct_eps[lane], ucp_ep_err_pending_purge, + UCS_STATUS_PTR(status)); } } @@ -2466,7 +2469,9 @@ static unsigned ucp_worker_discard_uct_ep_progress(void *arg) /* must be called with async lock held */ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, - unsigned ep_flush_flags) + unsigned ep_flush_flags, + uct_pending_purge_callback_t purge_cb, + void *purge_arg) { uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; ucp_wireup_ep_t *wireup_ep; @@ -2487,10 +2492,19 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, if (wireup_ep->aux_ep != NULL) { /* discard the WIREUP EP's auxiliary EP */ ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, - ep_flush_flags); + ep_flush_flags, + purge_cb, purge_arg); ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); } + if (purge_cb != NULL) { + /* purge the next UCT EP first to pass WIREUP MSGs to the user + * purge callback, otherwise, they will be release when purging + * the WIREUP EP */ + uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); + uct_ep_pending_purge(&wireup_ep->super.super, purge_cb, purge_arg); + } + /* destroy WIREUP EP allocated for this UCT EP, since * discard operation most likely won't have an access to * UCP EP as it could be destroyed by the caller */ @@ -2500,6 +2514,8 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, /* do nothing, if this wireup EP is not an owner for UCT EP */ return; } + } else if (purge_cb != NULL) { + uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); } req = ucp_request_get(worker); diff --git a/src/ucp/core/ucp_worker.h b/src/ucp/core/ucp_worker.h index 3ddec736451..1d9022c7e7b 100644 --- a/src/ucp/core/ucp_worker.h +++ b/src/ucp/core/ucp_worker.h @@ -318,6 +318,8 @@ ucs_status_t ucp_worker_set_ep_failed(ucp_worker_h worker, ucp_ep_h ucp_ep, /* must be called with async lock held */ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, - unsigned ep_flush_flags); + unsigned ep_flush_flags, + uct_pending_purge_callback_t purge_cb, + void *purge_arg); #endif diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 3eef49e6471..2e4b0a8ebf1 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -35,8 +35,20 @@ class test_ucp_worker_discard : public ucp_test { m_pending_reqs.clear(); } + void add_pending_req(uct_ep_h uct_ep, + uct_pending_callback_t func, + std::vector &pending_reqs) { + uct_pending_req_t pending_req; + + pending_req.func = func; + pending_reqs.push_back(pending_req); + + uct_ep_pending_add(uct_ep, &pending_reqs.back(), 0); + } + void test_worker_discard(void *ep_flush_func, void *ep_pending_add_func, + void *ep_pending_purge_func, unsigned ep_count = 8, unsigned wireup_ep_count = 0, unsigned wireup_aux_ep_count = 0) { @@ -46,6 +58,7 @@ class test_ucp_worker_discard : public ucp_test { uct_iface_t iface; std::vector eps(total_ep_count); std::vector wireup_eps(wireup_ep_count); + std::vector pending_reqs; ucp_ep_t ucp_ep; ucs_status_t status; @@ -54,10 +67,11 @@ class test_ucp_worker_discard : public ucp_test { ucp_ep.worker = sender().worker(); - ops.ep_flush = (uct_ep_flush_func_t)ep_flush_func; - ops.ep_pending_add = (uct_ep_pending_add_func_t)ep_pending_add_func; - ops.ep_destroy = ep_destroy_func; - iface.ops = ops; + ops.ep_flush = (uct_ep_flush_func_t)ep_flush_func; + ops.ep_pending_add = (uct_ep_pending_add_func_t)ep_pending_add_func; + ops.ep_pending_purge = (uct_ep_pending_purge_func_t)ep_pending_purge_func; + ops.ep_destroy = ep_destroy_func; + iface.ops = ops; for (unsigned i = 0; i < ep_count; i++) { uct_ep_h discard_ep; @@ -72,10 +86,12 @@ class test_ucp_worker_discard : public ucp_test { wireup_eps.push_back(discard_ep); ucp_wireup_ep_set_next_ep(discard_ep, &eps[i]); + ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(discard_ep); + wireup_ep->flags |= UCP_WIREUP_EP_FLAG_READY; + if (i < wireup_aux_ep_count) { eps[ep_count + created_wireup_aux_ep_count].iface = &iface; - ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(discard_ep); /* coverity[escape] */ wireup_ep->aux_ep = &eps[ep_count + created_wireup_aux_ep_count]; @@ -83,14 +99,41 @@ class test_ucp_worker_discard : public ucp_test { created_wireup_aux_ep_count++; m_created_ep_count++; } + + if (ep_pending_purge_func == ep_pending_purge_func_iter) { + /* add WIREUP MSGs */ + for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + add_pending_req(discard_ep, ucp_wireup_msg_progress, + pending_reqs); + } + + /* add user's pending requests */ + for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + add_pending_req(discard_ep, + (uct_pending_callback_t) + ucs_empty_function, + pending_reqs); + } + } } else { discard_ep = &eps[i]; } EXPECT_LE(m_created_ep_count, total_ep_count); + unsigned purged_reqs_count = 0; ucp_worker_discard_uct_ep(sender().worker(), discard_ep, - UCT_FLUSH_FLAG_LOCAL); + UCT_FLUSH_FLAG_LOCAL, + ep_pending_purge_count_reqs_cb, + &purged_reqs_count); + if (ep_pending_purge_func == ep_pending_purge_func_iter) { + unsigned expected_pending_purge_reqs_count = + m_pending_purge_reqs_count * + (1 + (i < wireup_ep_count) + (i < wireup_aux_ep_count)); + + + EXPECT_EQ(expected_pending_purge_reqs_count, purged_reqs_count); + } } void *flush_req = sender().flush_worker_nb(0); @@ -161,7 +204,7 @@ class test_ucp_worker_discard : public ucp_test { unsigned flags) { EXPECT_LT(m_pending_add_ep_count, 3 * m_created_ep_count); - if (m_pending_add_ep_count < m_created_ep_count) { + if (++m_pending_add_ep_count < m_created_ep_count) { m_pending_reqs.push_back(req); return UCS_OK; } @@ -169,65 +212,106 @@ class test_ucp_worker_discard : public ucp_test { return UCS_ERR_BUSY; } + static void + ep_pending_purge_count_reqs_cb(uct_pending_req_t *self, + void *arg) { + unsigned *count = (unsigned*)arg; + (*count)++; + } + + static void + ep_pending_purge_func_iter(uct_ep_h ep, + uct_pending_purge_callback_t cb, + void *arg) { + uct_pending_req_t req = {}; + for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + cb(&req, arg); + } + } + protected: static unsigned m_created_ep_count; static unsigned m_destroyed_ep_count; static unsigned m_flush_ep_count; static unsigned m_pending_add_ep_count; + static const unsigned m_pending_purge_reqs_count; static std::vector m_flush_comps; static std::vector m_pending_reqs; }; -unsigned test_ucp_worker_discard::m_created_ep_count = 0; -unsigned test_ucp_worker_discard::m_destroyed_ep_count = 0; -unsigned test_ucp_worker_discard::m_flush_ep_count = 0; -unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; +unsigned test_ucp_worker_discard::m_created_ep_count = 0; +unsigned test_ucp_worker_discard::m_destroyed_ep_count = 0; +unsigned test_ucp_worker_discard::m_flush_ep_count = 0; +unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; +const unsigned test_ucp_worker_discard::m_pending_purge_reqs_count = 10; std::vector test_ucp_worker_discard::m_flush_comps; std::vector test_ucp_worker_discard::m_pending_reqs; + UCS_TEST_P(test_ucp_worker_discard, flush_ok) { test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ucs_empty_function_do_assert); + (void*)ucs_empty_function_do_assert, + (void*)ucs_empty_function); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok) { test_worker_discard((void*)ucs_empty_function_return_success, (void*)ucs_empty_function_do_assert, + (void*)ucs_empty_function, + 8, 6, 3); +} + +UCS_TEST_P(test_ucp_worker_discard, flush_ok_pending_purge) { + test_worker_discard((void*)ucs_empty_function_return_success, + (void*)ucs_empty_function, + (void*)ep_pending_purge_func_iter); +} + +UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok_pending_purge) { + test_worker_discard((void*)ucs_empty_function_return_success, + (void*)ucs_empty_function, + (void*)ep_pending_purge_func_iter, 8, 6, 3); } UCS_TEST_P(test_ucp_worker_discard, flush_in_progress) { test_worker_discard((void*)ep_flush_func_return_in_progress, - (void*)ucs_empty_function_do_assert); + (void*)ucs_empty_function_do_assert, + (void*)ucs_empty_function); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_in_progress) { test_worker_discard((void*)ep_flush_func_return_in_progress, (void*)ucs_empty_function_do_assert, + (void*)ucs_empty_function, 8, 6, 3); } UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_busy) { test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ucs_empty_function_return_busy); + (void*)ucs_empty_function_return_busy, + (void*)ucs_empty_function); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_busy) { test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, (void*)ucs_empty_function_return_busy, + (void*)ucs_empty_function, 8, 6, 3); } UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_ok_then_busy) { test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ep_pending_add_func_return_ok_then_busy); + (void*)ep_pending_add_func_return_ok_then_busy, + (void*)ucs_empty_function); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_ok_then_busy) { test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, (void*)ep_pending_add_func_return_ok_then_busy, + (void*)ucs_empty_function, 8, 6, 3); } From 5d0b3fa8009353d6f5b40dcf79bb0b0b06855b30 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 05:37:39 +0000 Subject: [PATCH 14/33] UCP/CORE: Fix bug in purging --- src/ucp/core/ucp_worker.c | 17 ++++---- test/gtest/ucp/test_ucp_worker.cc | 66 +++++++++++++++---------------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index a9be2e3ff24..70d18de9949 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2486,9 +2486,6 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, wireup_ep = ucp_wireup_ep(uct_ep); ucs_assert(wireup_ep != NULL); - is_owner = wireup_ep->super.is_owner; - uct_ep = ucp_wireup_ep_extract_next_ep(uct_ep); - if (wireup_ep->aux_ep != NULL) { /* discard the WIREUP EP's auxiliary EP */ ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, @@ -2497,11 +2494,17 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); } + is_owner = wireup_ep->super.is_owner; + uct_ep = ucp_wireup_ep_extract_next_ep(uct_ep); + if (purge_cb != NULL) { - /* purge the next UCT EP first to pass WIREUP MSGs to the user - * purge callback, otherwise, they will be release when purging - * the WIREUP EP */ - uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); + if (is_owner /* purge from the next UCT EP, if the WIREUP EP is + * an owner for it */) { + /* purge the next UCT EP first to pass WIREUP MSGs to the user + * purge callback, otherwise, they will be release when purging + * the WIREUP EP */ + uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); + } uct_ep_pending_purge(&wireup_ep->super.super, purge_cb, purge_arg); } diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 2e4b0a8ebf1..316a9f24db9 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -35,15 +35,14 @@ class test_ucp_worker_discard : public ucp_test { m_pending_reqs.clear(); } - void add_pending_req(uct_ep_h uct_ep, - uct_pending_callback_t func, - std::vector &pending_reqs) { - uct_pending_req_t pending_req; - - pending_req.func = func; - pending_reqs.push_back(pending_req); - - uct_ep_pending_add(uct_ep, &pending_reqs.back(), 0); + void add_pending_reqs(uct_ep_h uct_ep, + uct_pending_callback_t func, + std::vector &pending_reqs, + unsigned base = 0) { + for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + pending_reqs[i].func = func; + uct_ep_pending_add(uct_ep, &pending_reqs[i], 0); + } } void test_worker_discard(void *ep_flush_func, @@ -58,7 +57,6 @@ class test_ucp_worker_discard : public ucp_test { uct_iface_t iface; std::vector eps(total_ep_count); std::vector wireup_eps(wireup_ep_count); - std::vector pending_reqs; ucp_ep_t ucp_ep; ucs_status_t status; @@ -79,6 +77,15 @@ class test_ucp_worker_discard : public ucp_test { eps[i].iface = &iface; m_created_ep_count++; + unsigned expected_pending_purge_reqs_count = + (ep_pending_purge_func == ep_pending_purge_func_iter) ? + (m_pending_purge_reqs_count * + (1 /* UCT EP */ + + (i < wireup_ep_count) /* WIREUP EP */ + + (i < wireup_aux_ep_count) /* AUX EP */)) : 0; + std::vector + pending_reqs(expected_pending_purge_reqs_count); + if (i < wireup_ep_count) { status = ucp_wireup_ep_create(&ucp_ep, &discard_ep); ASSERT_UCS_OK(status); @@ -93,27 +100,25 @@ class test_ucp_worker_discard : public ucp_test { eps[ep_count + created_wireup_aux_ep_count].iface = &iface; /* coverity[escape] */ - wireup_ep->aux_ep = &eps[ep_count + - created_wireup_aux_ep_count]; + wireup_ep->aux_ep = &eps[ep_count + + created_wireup_aux_ep_count]; created_wireup_aux_ep_count++; m_created_ep_count++; } - if (ep_pending_purge_func == ep_pending_purge_func_iter) { - /* add WIREUP MSGs */ - for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - add_pending_req(discard_ep, ucp_wireup_msg_progress, - pending_reqs); - } - - /* add user's pending requests */ - for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - add_pending_req(discard_ep, - (uct_pending_callback_t) - ucs_empty_function, - pending_reqs); - } + if (expected_pending_purge_reqs_count > 0) { + /* add WIREUP MSGs to the WIREUP EP */ + add_pending_reqs(discard_ep, + (uct_pending_callback_t) + ucp_wireup_msg_progress, + pending_reqs); + + /* add user's pending requests to the WIREUP EP */ + add_pending_reqs(discard_ep, + (uct_pending_callback_t) + ucs_empty_function, + pending_reqs, m_pending_purge_reqs_count); } } else { discard_ep = &eps[i]; @@ -126,14 +131,7 @@ class test_ucp_worker_discard : public ucp_test { UCT_FLUSH_FLAG_LOCAL, ep_pending_purge_count_reqs_cb, &purged_reqs_count); - if (ep_pending_purge_func == ep_pending_purge_func_iter) { - unsigned expected_pending_purge_reqs_count = - m_pending_purge_reqs_count * - (1 + (i < wireup_ep_count) + (i < wireup_aux_ep_count)); - - - EXPECT_EQ(expected_pending_purge_reqs_count, purged_reqs_count); - } + EXPECT_EQ(expected_pending_purge_reqs_count, purged_reqs_count); } void *flush_req = sender().flush_worker_nb(0); From 12d1c3452cc3f2af65155f5305303a804316c73c Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 07:12:35 +0000 Subject: [PATCH 15/33] UCP/CORE/GTEST: Fix review comments --- src/ucp/core/ucp_worker.c | 2 +- test/gtest/ucp/test_ucp_worker.cc | 25 +++++++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 70d18de9949..9e15e3e2434 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2501,7 +2501,7 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, if (is_owner /* purge from the next UCT EP, if the WIREUP EP is * an owner for it */) { /* purge the next UCT EP first to pass WIREUP MSGs to the user - * purge callback, otherwise, they will be release when purging + * purge callback, otherwise, they will be released when purging * the WIREUP EP */ uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); } diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 316a9f24db9..d38999fd869 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -77,12 +77,25 @@ class test_ucp_worker_discard : public ucp_test { eps[i].iface = &iface; m_created_ep_count++; - unsigned expected_pending_purge_reqs_count = - (ep_pending_purge_func == ep_pending_purge_func_iter) ? - (m_pending_purge_reqs_count * - (1 /* UCT EP */ + - (i < wireup_ep_count) /* WIREUP EP */ + - (i < wireup_aux_ep_count) /* AUX EP */)) : 0; + unsigned expected_pending_purge_reqs_count = 0; + + if (ep_pending_purge_func == ep_pending_purge_func_iter) { + /* expected purging count is the number of pending + * reqyests in the UCT EP, in the WIREUP EP (if used) + * and in the AUX EP (if used) */ + expected_pending_purge_reqs_count += + m_pending_purge_reqs_count; + + if (i < wireup_ep_count) { + expected_pending_purge_reqs_count += + m_pending_purge_reqs_count; + } + + if (i < wireup_aux_ep_count) { + expected_pending_purge_reqs_count += + m_pending_purge_reqs_count; + } + } std::vector pending_reqs(expected_pending_purge_reqs_count); From d1ab7d4c19423b3d5608f624be204c180529faee Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 08:16:20 +0000 Subject: [PATCH 16/33] GTEST/UCP: Fix review comments --- test/gtest/ucp/test_ucp_worker.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index d38999fd869..9dffceb65b6 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -81,8 +81,8 @@ class test_ucp_worker_discard : public ucp_test { if (ep_pending_purge_func == ep_pending_purge_func_iter) { /* expected purging count is the number of pending - * reqyests in the UCT EP, in the WIREUP EP (if used) - * and in the AUX EP (if used) */ + * requests in the UCT EP, in the WIREUP EP (if used) + * and in the AUX EP (if used) */ expected_pending_purge_reqs_count += m_pending_purge_reqs_count; From d809dc155d8d7294b7bef4a1479cabbfff340e5c Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 16:50:18 +0300 Subject: [PATCH 17/33] UCP/CORE/GTEST: Fix bug in purging --- src/ucp/core/ucp_worker.c | 23 +++++----- src/ucp/wireup/wireup_ep.c | 49 ++++++++++++++------ src/ucp/wireup/wireup_ep.h | 7 +++ test/gtest/ucp/test_ucp_worker.cc | 75 ++++++++++++++++++++++--------- 4 files changed, 110 insertions(+), 44 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 9e15e3e2434..520dc2c7e71 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2486,7 +2486,19 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, wireup_ep = ucp_wireup_ep(uct_ep); ucs_assert(wireup_ep != NULL); + if (purge_cb != NULL) { + ucp_wireup_ep_pending_purge_common(wireup_ep, + purge_cb, purge_arg, + purge_cb, purge_arg); + } + if (wireup_ep->aux_ep != NULL) { + /* make sure that there is no WIREUP MSGs anymore */ + uct_ep_pending_purge(wireup_ep->aux_ep, + (uct_pending_purge_callback_t) + ucs_empty_function_do_assert, + NULL); + /* discard the WIREUP EP's auxiliary EP */ ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, ep_flush_flags, @@ -2497,17 +2509,6 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, is_owner = wireup_ep->super.is_owner; uct_ep = ucp_wireup_ep_extract_next_ep(uct_ep); - if (purge_cb != NULL) { - if (is_owner /* purge from the next UCT EP, if the WIREUP EP is - * an owner for it */) { - /* purge the next UCT EP first to pass WIREUP MSGs to the user - * purge callback, otherwise, they will be released when purging - * the WIREUP EP */ - uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); - } - uct_ep_pending_purge(&wireup_ep->super.super, purge_cb, purge_arg); - } - /* destroy WIREUP EP allocated for this UCT EP, since * discard operation most likely won't have an access to * UCP EP as it could be destroyed by the caller */ diff --git a/src/ucp/wireup/wireup_ep.c b/src/ucp/wireup/wireup_ep.c index 53fdc20cd98..8a0fcea5a58 100644 --- a/src/ucp/wireup/wireup_ep.c +++ b/src/ucp/wireup/wireup_ep.c @@ -208,31 +208,54 @@ static ucs_status_t ucp_wireup_ep_pending_add(uct_ep_h uct_ep, return status; } -static void -ucp_wireup_ep_pending_purge(uct_ep_h uct_ep, uct_pending_purge_callback_t cb, - void *arg) +void +ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, + uct_pending_purge_callback_t wireup_msg_cb, + void *wireup_msg_arg, + uct_pending_purge_callback_t user_msg_cb, + void *user_msg_arg) { - ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep); - ucp_worker_h worker; + ucp_worker_h worker; uct_pending_req_t *req; - ucp_request_t *ucp_req; + ucp_request_t *ucp_req; + uct_ep_h uct_ep; worker = wireup_ep->super.ucp_ep->worker; + if (wireup_ep->pending_count > 0) { + uct_ep = ucp_wireup_ep_get_msg_ep(wireup_ep); + /* do purging on AUX EP or on UCT EP is WIREUP is an owner of it */ + if ((uct_ep == wireup_ep->aux_ep) || + wireup_ep->super.is_owner) { + uct_ep_pending_purge(uct_ep, + wireup_msg_cb, wireup_msg_arg); + } + if (wireup_msg_cb != ucp_wireup_ep_pending_req_release) { + /* reset the pending count, if it is not a request release + * callback */ + wireup_ep->pending_count = 0; + } + } + + ucs_assert(wireup_ep->pending_count == 0); + ucs_queue_for_each_extract(req, &wireup_ep->pending_q, priv, 1) { ucp_req = ucs_container_of(req, ucp_request_t, send.uct); UCS_ASYNC_BLOCK(&worker->async); --worker->flush_ops_count; UCS_ASYNC_UNBLOCK(&worker->async); - cb(&ucp_req->send.uct, arg); - } - - if (wireup_ep->pending_count > 0) { - uct_ep_pending_purge(ucp_wireup_ep_get_msg_ep(wireup_ep), - ucp_wireup_ep_pending_req_release, arg); + user_msg_cb(&ucp_req->send.uct, user_msg_arg); } +} - ucs_assert(wireup_ep->pending_count == 0); +static void +ucp_wireup_ep_pending_purge(uct_ep_h uct_ep, uct_pending_purge_callback_t cb, + void *arg) +{ + ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep); + ucp_wireup_ep_pending_purge_common(wireup_ep, + ucp_wireup_ep_pending_req_release, NULL, + cb, arg); } static ssize_t ucp_wireup_ep_am_bcopy(uct_ep_h uct_ep, uint8_t id, diff --git a/src/ucp/wireup/wireup_ep.h b/src/ucp/wireup/wireup_ep.h index efa3eb60f17..d315f459ad8 100644 --- a/src/ucp/wireup/wireup_ep.h +++ b/src/ucp/wireup/wireup_ep.h @@ -96,6 +96,13 @@ void ucp_wireup_ep_disown(uct_ep_h uct_ep, uct_ep_h owned_ep); ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self); +void +ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, + uct_pending_purge_callback_t wireup_msg_cb, + void *wireup_msg_arg, + uct_pending_purge_callback_t user_msg_cb, + void *user_msg_arg); + ucp_wireup_ep_t *ucp_wireup_ep(uct_ep_h uct_ep); #endif diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 9dffceb65b6..60a1ca5e6fb 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -40,8 +40,8 @@ class test_ucp_worker_discard : public ucp_test { std::vector &pending_reqs, unsigned base = 0) { for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - pending_reqs[i].func = func; - uct_ep_pending_add(uct_ep, &pending_reqs[i], 0); + pending_reqs[base + i].func = func; + uct_ep_pending_add(uct_ep, &pending_reqs[base + i], 0); } } @@ -78,11 +78,13 @@ class test_ucp_worker_discard : public ucp_test { m_created_ep_count++; unsigned expected_pending_purge_reqs_count = 0; + unsigned added_pending_purge_reqs_count = 0; if (ep_pending_purge_func == ep_pending_purge_func_iter) { /* expected purging count is the number of pending - * requests in the UCT EP, in the WIREUP EP (if used) - * and in the AUX EP (if used) */ + * requests in the WIREUP EP (if used) and in the + * WIREUP AUX EP (if used) or UCT EP (if no WIREUP EP + * or no WIREUP AUX EP) */ expected_pending_purge_reqs_count += m_pending_purge_reqs_count; @@ -90,12 +92,8 @@ class test_ucp_worker_discard : public ucp_test { expected_pending_purge_reqs_count += m_pending_purge_reqs_count; } - - if (i < wireup_aux_ep_count) { - expected_pending_purge_reqs_count += - m_pending_purge_reqs_count; - } } + std::vector pending_reqs(expected_pending_purge_reqs_count); @@ -106,8 +104,7 @@ class test_ucp_worker_discard : public ucp_test { wireup_eps.push_back(discard_ep); ucp_wireup_ep_set_next_ep(discard_ep, &eps[i]); - ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(discard_ep); - wireup_ep->flags |= UCP_WIREUP_EP_FLAG_READY; + ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(discard_ep); if (i < wireup_aux_ep_count) { eps[ep_count + created_wireup_aux_ep_count].iface = &iface; @@ -121,17 +118,14 @@ class test_ucp_worker_discard : public ucp_test { } if (expected_pending_purge_reqs_count > 0) { - /* add WIREUP MSGs to the WIREUP EP */ + /* add WIREUP MSGs to the WIREUP EP (it will be added to + * UCT EP or WIREUP AUX EP) */ add_pending_reqs(discard_ep, (uct_pending_callback_t) ucp_wireup_msg_progress, pending_reqs); - - /* add user's pending requests to the WIREUP EP */ - add_pending_reqs(discard_ep, - (uct_pending_callback_t) - ucs_empty_function, - pending_reqs, m_pending_purge_reqs_count); + added_pending_purge_reqs_count += + m_pending_purge_reqs_count; } } else { discard_ep = &eps[i]; @@ -139,6 +133,21 @@ class test_ucp_worker_discard : public ucp_test { EXPECT_LE(m_created_ep_count, total_ep_count); + + if (expected_pending_purge_reqs_count > 0) { + /* add user's pending requests */ + add_pending_reqs(discard_ep, + (uct_pending_callback_t) + ucs_empty_function, + pending_reqs, + added_pending_purge_reqs_count); + added_pending_purge_reqs_count += + m_pending_purge_reqs_count; + } + + EXPECT_EQ(expected_pending_purge_reqs_count, + added_pending_purge_reqs_count); + unsigned purged_reqs_count = 0; ucp_worker_discard_uct_ep(sender().worker(), discard_ep, UCT_FLUSH_FLAG_LOCAL, @@ -230,12 +239,36 @@ class test_ucp_worker_discard : public ucp_test { (*count)++; } + static ucs_status_t + ep_pending_add_and_inc_count(uct_ep_h ep, uct_pending_req_t *req, + unsigned flags) { + std::map::iterator it = + m_pending_reqs_count.find(ep); + if (it == m_pending_reqs_count.end()) { + m_pending_reqs_count.insert(std::make_pair(ep, 1)); + } else { + unsigned *count = &it->second; + (*count)++; + } + return UCS_OK; + } + static void ep_pending_purge_func_iter(uct_ep_h ep, uct_pending_purge_callback_t cb, void *arg) { uct_pending_req_t req = {}; for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + std::map::iterator it = + m_pending_reqs_count.find(ep); + ASSERT_NE(it, m_pending_reqs_count.end()); + + unsigned *count = &it->second; + if (*count == 0) { + break; + } + + (*count)--; cb(&req, arg); } } @@ -249,6 +282,7 @@ class test_ucp_worker_discard : public ucp_test { static std::vector m_flush_comps; static std::vector m_pending_reqs; + static std::map m_pending_reqs_count; }; unsigned test_ucp_worker_discard::m_created_ep_count = 0; @@ -259,6 +293,7 @@ const unsigned test_ucp_worker_discard::m_pending_purge_reqs_count = 10; std::vector test_ucp_worker_discard::m_flush_comps; std::vector test_ucp_worker_discard::m_pending_reqs; +std::map test_ucp_worker_discard::m_pending_reqs_count; UCS_TEST_P(test_ucp_worker_discard, flush_ok) { @@ -276,13 +311,13 @@ UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok) { UCS_TEST_P(test_ucp_worker_discard, flush_ok_pending_purge) { test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ucs_empty_function, + (void*)ep_pending_add_and_inc_count, (void*)ep_pending_purge_func_iter); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok_pending_purge) { test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ucs_empty_function, + (void*)ep_pending_add_and_inc_count, (void*)ep_pending_purge_func_iter, 8, 6, 3); } From a0f759b71cbe2a62cfd0b8510595b814cac6e04c Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 16:01:52 +0000 Subject: [PATCH 18/33] UCP/CORE: Fix review comments --- src/ucp/core/ucp_worker.c | 122 +++++++++++++++++------------- test/gtest/ucp/test_ucp_worker.cc | 80 +++++++++++--------- 2 files changed, 113 insertions(+), 89 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 79ff1b8bcaa..e5844517803 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -110,6 +110,10 @@ KHASH_IMPL(ucp_worker_discard_uct_ep_hash, uct_ep_h, char, 0, ucp_worker_discard_uct_ep_hash_key, kh_int64_hash_equal); +/* Forward declaration to use in the dicarding of UCT EP pending callback */ +static unsigned ucp_worker_discard_uct_ep_progress(void *arg); + + static ucs_status_t ucp_worker_wakeup_ctl_fd(ucp_worker_h worker, ucp_worker_event_fd_op_t op, int event_fd) @@ -640,6 +644,15 @@ ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) ucs_debug("worker %p: error handler called for UCT EP %p: %s", worker, uct_ep, ucs_status_string(status)); + iter = kh_get(ucp_worker_discard_uct_ep_hash, + &worker->discard_uct_ep_hash, uct_ep); + if (iter != kh_end(&worker->discard_uct_ep_hash)) { + ucs_debug("UCT EP %p is being discarded on UCP Worker %p", + uct_ep, worker); + ret_status = UCS_OK; + goto out; + } + /* TODO: need to optimize uct_ep -> ucp_ep lookup */ ucs_list_for_each(ep_ext, &worker->all_eps, ep_list) { ucp_ep = ucp_ep_from_ext_gen(ep_ext); @@ -648,27 +661,18 @@ ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) ucp_wireup_ep_is_owner(ucp_ep->uct_eps[lane], uct_ep)) { ret_status = ucp_worker_set_ep_failed(worker, ucp_ep, uct_ep, lane, status); - UCS_ASYNC_UNBLOCK(&worker->async); - return ret_status; + goto out; } } } - iter = kh_get(ucp_worker_discard_uct_ep_hash, - &worker->discard_uct_ep_hash, uct_ep); - if (iter != kh_end(&worker->discard_uct_ep_hash)) { - ucs_debug("UCT EP %p is being discarded on UCP Worker %p", - uct_ep, worker); - ret_status = UCS_OK; - } else { - ucs_error("UCT EP %p isn't associated with UCP EP and was not scheduled " - "to be discarded on UCP Worker %p", - uct_ep, worker); - ret_status = UCS_ERR_NO_ELEM; - } + ucs_error("UCT EP %p isn't associated with UCP EP and was not scheduled " + "to be discarded on UCP Worker %p", + uct_ep, worker); + ret_status = UCS_ERR_NO_ELEM; +out: UCS_ASYNC_UNBLOCK(&worker->async); - return ret_status; } @@ -2433,9 +2437,6 @@ ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); } -/* Forward declaration to use in the pending callback */ -static unsigned ucp_worker_discard_uct_ep_progress(void *arg); - static ucs_status_t ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { @@ -2459,7 +2460,7 @@ ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) } return UCS_OK; - } if (status == UCS_INPROGRESS) { + } else if (status == UCS_INPROGRESS) { /* need to remove from the pending queue */ status = UCS_OK; } else { @@ -2477,6 +2478,50 @@ static unsigned ucp_worker_discard_uct_ep_progress(void *arg) return !UCS_STATUS_IS_ERR(status); } +static uct_ep_h +ucp_worker_discard_wireup_ep(ucp_worker_h worker, + ucp_wireup_ep_t *wireup_ep, + unsigned ep_flush_flags, + uct_pending_purge_callback_t purge_cb, + void *purge_arg) +{ + uct_ep_h uct_ep; + int is_owner; + + ucs_assert(wireup_ep != NULL); + + if (purge_cb != NULL) { + ucp_wireup_ep_pending_purge_common(wireup_ep, + purge_cb, purge_arg, + purge_cb, purge_arg); + } + + if (wireup_ep->aux_ep != NULL) { + /* make sure that there is no WIREUP MSGs anymore */ + uct_ep_pending_purge(wireup_ep->aux_ep, + (uct_pending_purge_callback_t) + ucs_empty_function_do_assert, + NULL); + + /* discard the WIREUP EP's auxiliary EP */ + ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, + ep_flush_flags, + purge_cb, purge_arg); + ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); + } + + is_owner = wireup_ep->super.is_owner; + uct_ep = ucp_wireup_ep_extract_next_ep(&wireup_ep->super.super); + + /* destroy WIREUP EP allocated for this UCT EP, since + * discard operation most likely won't have an access to + * UCP EP as it could be destroyed by the caller */ + uct_ep_destroy(&wireup_ep->super.super); + + /* do nothing, if this wireup EP is not an owner for UCT EP */ + return is_owner ? uct_ep : NULL; +} + /* must be called with async lock held */ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, unsigned ep_flush_flags, @@ -2484,48 +2529,17 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, void *purge_arg) { uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; - ucp_wireup_ep_t *wireup_ep; ucp_request_t *req; khiter_t UCS_V_UNUSED iter; - int is_owner; int ret; ucs_assert(uct_ep != NULL); if (ucp_wireup_ep_test(uct_ep)) { - wireup_ep = ucp_wireup_ep(uct_ep); - ucs_assert(wireup_ep != NULL); - - if (purge_cb != NULL) { - ucp_wireup_ep_pending_purge_common(wireup_ep, - purge_cb, purge_arg, - purge_cb, purge_arg); - } - - if (wireup_ep->aux_ep != NULL) { - /* make sure that there is no WIREUP MSGs anymore */ - uct_ep_pending_purge(wireup_ep->aux_ep, - (uct_pending_purge_callback_t) - ucs_empty_function_do_assert, - NULL); - - /* discard the WIREUP EP's auxiliary EP */ - ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, - ep_flush_flags, - purge_cb, purge_arg); - ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); - } - - is_owner = wireup_ep->super.is_owner; - uct_ep = ucp_wireup_ep_extract_next_ep(uct_ep); - - /* destroy WIREUP EP allocated for this UCT EP, since - * discard operation most likely won't have an access to - * UCP EP as it could be destroyed by the caller */ - uct_ep_destroy(&wireup_ep->super.super); - - if (!is_owner) { - /* do nothing, if this wireup EP is not an owner for UCT EP */ + uct_ep = ucp_worker_discard_wireup_ep(worker, ucp_wireup_ep(uct_ep), + ep_flush_flags, + purge_cb, purge_arg); + if (uct_ep == NULL) { return; } } else if (purge_cb != NULL) { diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 60a1ca5e6fb..5cb63a76819 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -297,68 +297,78 @@ std::map test_ucp_worker_discard::m_pending_reqs_count; UCS_TEST_P(test_ucp_worker_discard, flush_ok) { - test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ucs_empty_function_do_assert, - (void*)ucs_empty_function); + test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, + (void*)ucs_empty_function_do_assert /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok) { - test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ucs_empty_function_do_assert, - (void*)ucs_empty_function, - 8, 6, 3); + test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, + (void*)ucs_empty_function_do_assert /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */, + 8 /* UCT EP count */, + 6 /* WIREUP EP count */, + 3 /* WIREUP AUX EP count */); } UCS_TEST_P(test_ucp_worker_discard, flush_ok_pending_purge) { - test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ep_pending_add_and_inc_count, - (void*)ep_pending_purge_func_iter); + test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, + (void*)ep_pending_add_and_inc_count /* ep_pending_add */, + (void*)ep_pending_purge_func_iter /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok_pending_purge) { - test_worker_discard((void*)ucs_empty_function_return_success, - (void*)ep_pending_add_and_inc_count, - (void*)ep_pending_purge_func_iter, - 8, 6, 3); + test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, + (void*)ep_pending_add_and_inc_count /* ep_pending_add */, + (void*)ep_pending_purge_func_iter /* ep_pending_purge */, + 8 /* UCT EP count */, + 6 /* WIREUP EP count */, + 3 /* WIREUP AUX EP count */); } UCS_TEST_P(test_ucp_worker_discard, flush_in_progress) { - test_worker_discard((void*)ep_flush_func_return_in_progress, - (void*)ucs_empty_function_do_assert, - (void*)ucs_empty_function); + test_worker_discard((void*)ep_flush_func_return_in_progress /* ep_flush */, + (void*)ucs_empty_function_do_assert /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_in_progress) { - test_worker_discard((void*)ep_flush_func_return_in_progress, - (void*)ucs_empty_function_do_assert, - (void*)ucs_empty_function, - 8, 6, 3); + test_worker_discard((void*)ep_flush_func_return_in_progress /* ep_flush */, + (void*)ucs_empty_function_do_assert /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */, + 8 /* UCT EP count */, + 6 /* WIREUP EP count */, + 3 /* WIREUP AUX EP count */); } UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_busy) { - test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ucs_empty_function_return_busy, - (void*)ucs_empty_function); + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok /* ep_flush */, + (void*)ucs_empty_function_return_busy /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_busy) { - test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ucs_empty_function_return_busy, - (void*)ucs_empty_function, - 8, 6, 3); + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok /* ep_flush */, + (void*)ucs_empty_function_return_busy /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */, + 8 /* UCT EP count */, + 6 /* WIREUP EP count */, + 3 /* WIREUP AUX EP count */); } UCS_TEST_P(test_ucp_worker_discard, flush_no_resource_pending_add_ok_then_busy) { - test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ep_pending_add_func_return_ok_then_busy, - (void*)ucs_empty_function); + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok /* ep_flush */, + (void*)ep_pending_add_func_return_ok_then_busy /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_no_resource_pending_add_ok_then_busy) { - test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok, - (void*)ep_pending_add_func_return_ok_then_busy, - (void*)ucs_empty_function, - 8, 6, 3); + test_worker_discard((void*)ep_flush_func_return_3_no_resource_then_ok /* ep_flush */, + (void*)ep_pending_add_func_return_ok_then_busy /* ep_pending_add */, + (void*)ucs_empty_function /* ep_pending_purge */, + 8 /* UCT EP count */, + 6 /* WIREUP EP count */, + 3 /* WIREUP AUX EP count */); } UCP_INSTANTIATE_TEST_CASE_TLS(test_ucp_worker_discard, all, "all") From 46afaaf1ab5f2709bf716b6cb0f4b5ffabf53649 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Thu, 3 Sep 2020 20:51:04 +0000 Subject: [PATCH 19/33] UCP/CORE/WIREUP/GTEST: Fix leak of WIREUP MSG proxy req --- src/ucp/wireup/wireup.c | 1 - src/ucp/wireup/wireup_ep.c | 71 +++++++++++++++++----- test/gtest/ucp/test_ucp_worker.cc | 97 +++++++++++++++++++++---------- 3 files changed, 124 insertions(+), 45 deletions(-) diff --git a/src/ucp/wireup/wireup.c b/src/ucp/wireup/wireup.c index db27bc74ffa..71e52843478 100644 --- a/src/ucp/wireup/wireup.c +++ b/src/ucp/wireup/wireup.c @@ -133,7 +133,6 @@ ucs_status_t ucp_wireup_msg_progress(uct_pending_req_t *self) out: ucs_free((void*)req->send.buffer); - ucs_free(req); return UCS_OK; } diff --git a/src/ucp/wireup/wireup_ep.c b/src/ucp/wireup/wireup_ep.c index 3ba8b37974f..cd03999f3bf 100644 --- a/src/ucp/wireup/wireup_ep.c +++ b/src/ucp/wireup/wireup_ep.c @@ -22,6 +22,18 @@ #include +typedef struct ucp_wireup_ep_pending_purge_arg { + ucp_wireup_ep_t *wireup_ep; /* WIREUP EP that owns the UCT + * pending request for the WIREUP + * MSG */ + uct_pending_purge_callback_t wireup_msg_cb; /* UCT pending purge cakkback that + * will handle WIREUP MSG pending + * request */ + void *wireup_msg_arg; /* UCT pending purge callback + * argument */ +} ucp_wireup_ep_pending_purge_arg_t; + + UCS_CLASS_DECLARE(ucp_wireup_ep_t, ucp_ep_h); @@ -141,14 +153,19 @@ static uct_ep_h ucp_wireup_ep_get_msg_ep(ucp_wireup_ep_t *wireup_ep) ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self) { - ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct); - uct_pending_req_t *req = proxy_req->send.proxy.req; + ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct); + uct_pending_req_t *req = proxy_req->send.proxy.req; ucp_wireup_ep_t *wireup_ep = proxy_req->send.proxy.wireup_ep; ucs_status_t status; status = req->func(req); if (status == UCS_OK) { - ucs_atomic_sub32(&wireup_ep->pending_count, 1); + /* WIREUP EP pointer could be NULL in case of it was discarded, but this + * pending request was completed on the UCT EP or the WIREUP AUX EP */ + if (wireup_ep != NULL) { + ucs_atomic_sub32(&wireup_ep->pending_count, 1); + ucs_free(req); + } ucs_free(proxy_req); } return status; @@ -218,6 +235,34 @@ static ucs_status_t ucp_wireup_ep_pending_add(uct_ep_h uct_ep, return status; } +static void +ucp_wireup_ep_pending_purge_cb(uct_pending_req_t *self, void *arg) +{ + ucp_wireup_ep_pending_purge_arg_t *purge_arg = + (ucp_wireup_ep_pending_purge_arg_t*)arg; + ucp_wireup_ep_t *wireup_ep = purge_arg->wireup_ep; + uct_ep_h uct_ep = + ucp_wireup_ep_get_msg_ep(wireup_ep); + ucp_request_t *wireup_proxy_req = + ucs_container_of(self, ucp_request_t, send.uct); + + /* do purging on AUX EP or on UCT EP if WIREUP is an owner of it */ + if ((uct_ep == wireup_ep->aux_ep) || wireup_ep->super.is_owner) { + /* need to NULL the WIREUP EP in the WIREUP MSG proxy pending request + * to avoid dereferencing it when progressing prnding requests, since + * it will be destroyed */ + wireup_proxy_req->send.proxy.wireup_ep = NULL; + + purge_arg->wireup_msg_cb(self, purge_arg->wireup_msg_arg); + if (purge_arg->wireup_msg_cb != ucp_wireup_ep_pending_req_release) { + /* decrement the pending count, if it is not a request release + * callback */ + ucs_atomic_sub32(&wireup_ep->pending_count, 1); + + } + } +} + void ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, uct_pending_purge_callback_t wireup_msg_cb, @@ -225,6 +270,7 @@ ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, uct_pending_purge_callback_t user_msg_cb, void *user_msg_arg) { + ucp_wireup_ep_pending_purge_arg_t purge_arg; ucp_worker_h worker; uct_pending_req_t *req; ucp_request_t *ucp_req; @@ -234,17 +280,14 @@ ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, if (wireup_ep->pending_count > 0) { uct_ep = ucp_wireup_ep_get_msg_ep(wireup_ep); - /* do purging on AUX EP or on UCT EP is WIREUP is an owner of it */ - if ((uct_ep == wireup_ep->aux_ep) || - wireup_ep->super.is_owner) { - uct_ep_pending_purge(uct_ep, - wireup_msg_cb, wireup_msg_arg); - } - if (wireup_msg_cb != ucp_wireup_ep_pending_req_release) { - /* reset the pending count, if it is not a request release - * callback */ - wireup_ep->pending_count = 0; - } + + purge_arg.wireup_ep = wireup_ep; + purge_arg.wireup_msg_cb = wireup_msg_cb; + purge_arg.wireup_msg_arg = wireup_msg_arg; + + uct_ep_pending_purge(uct_ep, + ucp_wireup_ep_pending_purge_cb, + &purge_arg); } ucs_assert(wireup_ep->pending_count == 0); diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 5cb63a76819..2e93befbdd3 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -10,6 +10,7 @@ extern "C" { #include +#include #include #include } @@ -30,6 +31,7 @@ class test_ucp_worker_discard : public ucp_test { m_destroyed_ep_count = 0; m_flush_ep_count = 0; m_pending_add_ep_count = 0; + m_fake_ep.flags = UCP_EP_FLAG_REMOTE_CONNECTED; m_flush_comps.clear(); m_pending_reqs.clear(); @@ -37,11 +39,22 @@ class test_ucp_worker_discard : public ucp_test { void add_pending_reqs(uct_ep_h uct_ep, uct_pending_callback_t func, - std::vector &pending_reqs, + std::vector &pending_reqs, unsigned base = 0) { for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - pending_reqs[base + i].func = func; - uct_ep_pending_add(uct_ep, &pending_reqs[base + i], 0); + ucp_request_t *req = &pending_reqs[base + i]; + + if (func == ucp_wireup_msg_progress) { + req->send.ep = &m_fake_ep; + + /* for fast completing the WIREUP MSG, it frees the send + * buffer and returns UCS_OK */ + req->send.wireup.type = UCP_WIREUP_MSG_REQUEST; + ASSERT_TRUE(m_fake_ep.flags & UCP_EP_FLAG_REMOTE_CONNECTED); + } + + req->send.uct.func = func; + uct_ep_pending_add(uct_ep, &req->send.uct, 0); } } @@ -80,7 +93,7 @@ class test_ucp_worker_discard : public ucp_test { unsigned expected_pending_purge_reqs_count = 0; unsigned added_pending_purge_reqs_count = 0; - if (ep_pending_purge_func == ep_pending_purge_func_iter) { + if (ep_pending_purge_func == ep_pending_purge_func_iter_reqs) { /* expected purging count is the number of pending * requests in the WIREUP EP (if used) and in the * WIREUP AUX EP (if used) or UCT EP (if no WIREUP EP @@ -94,7 +107,7 @@ class test_ucp_worker_discard : public ucp_test { } } - std::vector + std::vector pending_reqs(expected_pending_purge_reqs_count); if (i < wireup_ep_count) { @@ -237,39 +250,57 @@ class test_ucp_worker_discard : public ucp_test { void *arg) { unsigned *count = (unsigned*)arg; (*count)++; + + if (self->func == ucp_wireup_ep_progress_pending) { + /* need to complete WIREUP MSG to release allocated + * proxy request */ + ucp_request_t *req = ucs_container_of(self, + ucp_request_t, + send.uct); + /* TODO: replace by `ucp_request_send()` when + * `ucp/core/ucp_request.inl` file could be compiled + * by C++ compiler */ + ucs_status_t status = req->send.uct.func(&req->send.uct); + EXPECT_EQ(UCS_OK, status); + } } static ucs_status_t - ep_pending_add_and_inc_count(uct_ep_h ep, uct_pending_req_t *req, - unsigned flags) { - std::map::iterator it = - m_pending_reqs_count.find(ep); - if (it == m_pending_reqs_count.end()) { - m_pending_reqs_count.insert(std::make_pair(ep, 1)); + ep_pending_add_save_req(uct_ep_h ep, uct_pending_req_t *req, + unsigned flags) { + std::map >::iterator it = + m_pending_reqs_map.find(ep); + if (it == m_pending_reqs_map.end()) { + std::vector vec; + vec.push_back(req); + m_pending_reqs_map.insert(std::make_pair(ep, vec)); } else { - unsigned *count = &it->second; - (*count)++; + std::vector *req_vec = &it->second; + req_vec->push_back(req); } return UCS_OK; } static void - ep_pending_purge_func_iter(uct_ep_h ep, + ep_pending_purge_func_iter_reqs(uct_ep_h ep, uct_pending_purge_callback_t cb, void *arg) { - uct_pending_req_t req = {}; + uct_pending_req_t *req; for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - std::map::iterator it = - m_pending_reqs_count.find(ep); - ASSERT_NE(it, m_pending_reqs_count.end()); + std::map >::iterator it = + m_pending_reqs_map.find(ep); + ASSERT_NE(it, m_pending_reqs_map.end()); - unsigned *count = &it->second; - if (*count == 0) { + std::vector *req_vec = &it->second; + if (req_vec->size() == 0) { break; } - (*count)--; - cb(&req, arg); + req = req_vec->back(); + req_vec->pop_back(); + cb(req, arg); } } @@ -278,22 +309,28 @@ class test_ucp_worker_discard : public ucp_test { static unsigned m_destroyed_ep_count; static unsigned m_flush_ep_count; static unsigned m_pending_add_ep_count; + static ucp_ep_t m_fake_ep; static const unsigned m_pending_purge_reqs_count; - static std::vector m_flush_comps; + static std::vector m_flush_comps; static std::vector m_pending_reqs; - static std::map m_pending_reqs_count; + static std::map > + m_pending_reqs_map; }; unsigned test_ucp_worker_discard::m_created_ep_count = 0; unsigned test_ucp_worker_discard::m_destroyed_ep_count = 0; unsigned test_ucp_worker_discard::m_flush_ep_count = 0; unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; +ucp_ep_t test_ucp_worker_discard::m_fake_ep = {}; const unsigned test_ucp_worker_discard::m_pending_purge_reqs_count = 10; -std::vector test_ucp_worker_discard::m_flush_comps; +std::vector test_ucp_worker_discard::m_flush_comps; std::vector test_ucp_worker_discard::m_pending_reqs; -std::map test_ucp_worker_discard::m_pending_reqs_count; +std::map > + test_ucp_worker_discard::m_pending_reqs_map; UCS_TEST_P(test_ucp_worker_discard, flush_ok) { @@ -313,14 +350,14 @@ UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok) { UCS_TEST_P(test_ucp_worker_discard, flush_ok_pending_purge) { test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, - (void*)ep_pending_add_and_inc_count /* ep_pending_add */, - (void*)ep_pending_purge_func_iter /* ep_pending_purge */); + (void*)ep_pending_add_save_req /* ep_pending_add */, + (void*)ep_pending_purge_func_iter_reqs /* ep_pending_purge */); } UCS_TEST_P(test_ucp_worker_discard, wireup_ep_flush_ok_pending_purge) { test_worker_discard((void*)ucs_empty_function_return_success /* ep_flush */, - (void*)ep_pending_add_and_inc_count /* ep_pending_add */, - (void*)ep_pending_purge_func_iter /* ep_pending_purge */, + (void*)ep_pending_add_save_req /* ep_pending_add */, + (void*)ep_pending_purge_func_iter_reqs /* ep_pending_purge */, 8 /* UCT EP count */, 6 /* WIREUP EP count */, 3 /* WIREUP AUX EP count */); From e2982d52bf3c06ba400b4fc6e4dd411626a22700 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Fri, 4 Sep 2020 04:06:40 +0000 Subject: [PATCH 20/33] UCP/WIREUP/GTEST: Use pointer to the UCP request to be able free it in WIREUP case --- src/ucp/wireup/wireup.c | 1 + src/ucp/wireup/wireup_ep.c | 2 -- test/gtest/ucp/test_ucp_worker.cc | 27 +++++++++++++++++++++------ 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/ucp/wireup/wireup.c b/src/ucp/wireup/wireup.c index 71e52843478..db27bc74ffa 100644 --- a/src/ucp/wireup/wireup.c +++ b/src/ucp/wireup/wireup.c @@ -133,6 +133,7 @@ ucs_status_t ucp_wireup_msg_progress(uct_pending_req_t *self) out: ucs_free((void*)req->send.buffer); + ucs_free(req); return UCS_OK; } diff --git a/src/ucp/wireup/wireup_ep.c b/src/ucp/wireup/wireup_ep.c index cd03999f3bf..e5588ad36e6 100644 --- a/src/ucp/wireup/wireup_ep.c +++ b/src/ucp/wireup/wireup_ep.c @@ -164,7 +164,6 @@ ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self) * pending request was completed on the UCT EP or the WIREUP AUX EP */ if (wireup_ep != NULL) { ucs_atomic_sub32(&wireup_ep->pending_count, 1); - ucs_free(req); } ucs_free(proxy_req); } @@ -258,7 +257,6 @@ ucp_wireup_ep_pending_purge_cb(uct_pending_req_t *self, void *arg) /* decrement the pending count, if it is not a request release * callback */ ucs_atomic_sub32(&wireup_ep->pending_count, 1); - } } } diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 2e93befbdd3..b39610077f1 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -39,12 +39,20 @@ class test_ucp_worker_discard : public ucp_test { void add_pending_reqs(uct_ep_h uct_ep, uct_pending_callback_t func, - std::vector &pending_reqs, + std::vector &pending_reqs, unsigned base = 0) { for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - ucp_request_t *req = &pending_reqs[base + i]; + /* use `ucs_calloc()` here, since the memory could be released + * in the `ucp_wireup_msg_progress()` function by `ucs_free()` */ + ucp_request_t *req = static_cast( + ucs_calloc(1, sizeof(*req), + "ucp_request")); + ASSERT_TRUE(req != NULL); + + pending_reqs[base + i] = req; if (func == ucp_wireup_msg_progress) { + req->send.ep = &m_fake_ep; /* for fast completing the WIREUP MSG, it frees the send @@ -107,7 +115,7 @@ class test_ucp_worker_discard : public ucp_test { } } - std::vector + std::vector pending_reqs(expected_pending_purge_reqs_count); if (i < wireup_ep_count) { @@ -251,17 +259,24 @@ class test_ucp_worker_discard : public ucp_test { unsigned *count = (unsigned*)arg; (*count)++; + ucp_request_t *req = ucs_container_of(self, + ucp_request_t, + send.uct); + if (self->func == ucp_wireup_ep_progress_pending) { /* need to complete WIREUP MSG to release allocated * proxy request */ - ucp_request_t *req = ucs_container_of(self, - ucp_request_t, - send.uct); /* TODO: replace by `ucp_request_send()` when * `ucp/core/ucp_request.inl` file could be compiled * by C++ compiler */ ucs_status_t status = req->send.uct.func(&req->send.uct); EXPECT_EQ(UCS_OK, status); + /* no need to release the memory allocated for the request. + * it will be freed in the `ucp_wireup_msg_send()` function, + * since it expects that the request was allocated in the + * `ucp_wireup_msg_send()` function */ + } else { + ucs_free(req); } } From 7ecd09d17852a0281cb15ad8639151264549efac Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Fri, 4 Sep 2020 12:10:40 +0000 Subject: [PATCH 21/33] GTEST/UCP: Fix review comments --- test/gtest/ucp/test_ucp_worker.cc | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index b39610077f1..dbbc6be3a9d 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -25,6 +25,9 @@ class test_ucp_worker_discard : public ucp_test { } protected: + typedef std::map > ep_pending_reqs_map; + void init() { ucp_test::init(); m_created_ep_count = 0; @@ -283,9 +286,7 @@ class test_ucp_worker_discard : public ucp_test { static ucs_status_t ep_pending_add_save_req(uct_ep_h ep, uct_pending_req_t *req, unsigned flags) { - std::map >::iterator it = - m_pending_reqs_map.find(ep); + ep_pending_reqs_map::iterator it = m_pending_reqs_map.find(ep); if (it == m_pending_reqs_map.end()) { std::vector vec; vec.push_back(req); @@ -303,9 +304,7 @@ class test_ucp_worker_discard : public ucp_test { void *arg) { uct_pending_req_t *req; for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - std::map >::iterator it = - m_pending_reqs_map.find(ep); + ep_pending_reqs_map::iterator it = m_pending_reqs_map.find(ep); ASSERT_NE(it, m_pending_reqs_map.end()); std::vector *req_vec = &it->second; @@ -329,9 +328,7 @@ class test_ucp_worker_discard : public ucp_test { static std::vector m_flush_comps; static std::vector m_pending_reqs; - static std::map > - m_pending_reqs_map; + static ep_pending_reqs_map m_pending_reqs_map; }; unsigned test_ucp_worker_discard::m_created_ep_count = 0; @@ -341,11 +338,9 @@ unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; ucp_ep_t test_ucp_worker_discard::m_fake_ep = {}; const unsigned test_ucp_worker_discard::m_pending_purge_reqs_count = 10; -std::vector test_ucp_worker_discard::m_flush_comps; -std::vector test_ucp_worker_discard::m_pending_reqs; -std::map > - test_ucp_worker_discard::m_pending_reqs_map; +std::vector test_ucp_worker_discard::m_flush_comps; +std::vector test_ucp_worker_discard::m_pending_reqs; +test_ucp_worker_discard::ep_pending_reqs_map test_ucp_worker_discard::m_pending_reqs_map; UCS_TEST_P(test_ucp_worker_discard, flush_ok) { From 9ef1f3439e3ec92359cd871180d8ebb0c10029d7 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Fri, 4 Sep 2020 15:52:57 +0000 Subject: [PATCH 22/33] UCP/CORE: Fix review comments --- src/ucp/core/ucp_worker.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index e5844517803..c1afba1a00e 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2455,8 +2455,9 @@ ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) * the callback to not invoke it several times */ if (status_add == UCS_ERR_BUSY) { uct_worker_progress_register_safe(worker->uct, - ucp_worker_discard_uct_ep_destroy_progress, - req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); + ucp_worker_discard_uct_ep_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, + &cb_id); } return UCS_OK; @@ -2489,12 +2490,11 @@ ucp_worker_discard_wireup_ep(ucp_worker_h worker, int is_owner; ucs_assert(wireup_ep != NULL); + ucs_assert(purge_cb != NULL); - if (purge_cb != NULL) { - ucp_wireup_ep_pending_purge_common(wireup_ep, - purge_cb, purge_arg, - purge_cb, purge_arg); - } + ucp_wireup_ep_pending_purge_common(wireup_ep, + purge_cb, purge_arg, + purge_cb, purge_arg); if (wireup_ep->aux_ep != NULL) { /* make sure that there is no WIREUP MSGs anymore */ From efdb4d30a68dcdf43cb384c6d6a3e0c675b7bfd4 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Sun, 6 Sep 2020 10:23:56 +0000 Subject: [PATCH 23/33] GTEST/UCP: Ensure that fluah+pending_add is registered on Worker progress when pending_add completed with UCS_ERR_BUSY --- test/gtest/ucp/test_ucp_worker.cc | 111 ++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 35 deletions(-) diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index dbbc6be3a9d..3deffd1a654 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -25,19 +25,22 @@ class test_ucp_worker_discard : public ucp_test { } protected: - typedef std::map > ep_pending_reqs_map; + struct ep_test_info_t { + std::vector pending_reqs; + unsigned flush_count; + unsigned pending_add_count; + }; + typedef std::map ep_test_info_map_t; void init() { ucp_test::init(); - m_created_ep_count = 0; - m_destroyed_ep_count = 0; - m_flush_ep_count = 0; - m_pending_add_ep_count = 0; - m_fake_ep.flags = UCP_EP_FLAG_REMOTE_CONNECTED; + m_created_ep_count = 0; + m_destroyed_ep_count = 0; + m_fake_ep.flags = UCP_EP_FLAG_REMOTE_CONNECTED; m_flush_comps.clear(); m_pending_reqs.clear(); + m_ep_test_info_map.clear(); } void add_pending_reqs(uct_ep_h uct_ep, @@ -210,6 +213,23 @@ class test_ucp_worker_discard : public ucp_test { EXPECT_UCS_OK(ucp_request_check_status(flush_req)); EXPECT_EQ(m_created_ep_count, m_destroyed_ep_count); EXPECT_EQ(m_created_ep_count, total_ep_count); + + for (unsigned i = 0; i < m_created_ep_count; i++) { + ep_test_info_t *test_info = ep_test_info_get(&eps[i]); + + /* check EP flush counters */ + if (ep_flush_func == ep_flush_func_return_3_no_resource_then_ok) { + EXPECT_EQ(4, test_info->flush_count); + } else if (ep_flush_func == ep_flush_func_return_in_progress) { + EXPECT_EQ(1, test_info->flush_count); + } + + /* check EP pending add counters */ + if (ep_pending_add_func == ep_pending_add_func_return_ok_then_busy) { + EXPECT_EQ(3, test_info->pending_add_count); + } + } + EXPECT_TRUE(m_flush_comps.empty()); EXPECT_TRUE(m_pending_reqs.empty()); @@ -227,18 +247,50 @@ class test_ucp_worker_discard : public ucp_test { m_destroyed_ep_count++; } + static ep_test_info_t* ep_test_info_get(uct_ep_h ep) { + ep_test_info_t *test_info_p; + ep_test_info_map_t::iterator it = m_ep_test_info_map.find(ep); + + if (it == m_ep_test_info_map.end()) { + ep_test_info_t test_info = {}; + + m_ep_test_info_map.insert(std::make_pair(ep, test_info)); + test_info_p = &m_ep_test_info_map.find(ep)->second; + } else { + test_info_p = &it->second; + } + + return test_info_p; + } + + static unsigned + ep_test_info_flush_inc(uct_ep_h ep) { + ep_test_info_t *test_info = ep_test_info_get(ep); + test_info->flush_count++; + return test_info->flush_count; + } + + static unsigned + ep_test_info_pending_add_inc(uct_ep_h ep) { + ep_test_info_t *test_info = ep_test_info_get(ep); + test_info->pending_add_count++; + return test_info->pending_add_count; + } + static ucs_status_t ep_flush_func_return_3_no_resource_then_ok(uct_ep_h ep, unsigned flags, uct_completion_t *comp) { - EXPECT_LT(m_flush_ep_count, 4 * m_created_ep_count); - return (++m_flush_ep_count < 3 * m_created_ep_count) ? + unsigned flush_ep_count = ep_test_info_flush_inc(ep); + EXPECT_LE(flush_ep_count, 4); + return (flush_ep_count < 4) ? UCS_ERR_NO_RESOURCE : UCS_OK; } static ucs_status_t ep_flush_func_return_in_progress(uct_ep_h ep, unsigned flags, uct_completion_t *comp) { - EXPECT_LT(m_flush_ep_count, m_created_ep_count); + unsigned flush_ep_count = ep_test_info_flush_inc(ep); + EXPECT_LE(flush_ep_count, m_created_ep_count); m_flush_comps.push_back(comp); return UCS_INPROGRESS; } @@ -246,9 +298,10 @@ class test_ucp_worker_discard : public ucp_test { static ucs_status_t ep_pending_add_func_return_ok_then_busy(uct_ep_h ep, uct_pending_req_t *req, unsigned flags) { - EXPECT_LT(m_pending_add_ep_count, 3 * m_created_ep_count); + unsigned pending_add_ep_count = ep_test_info_pending_add_inc(ep); + EXPECT_LE(pending_add_ep_count, m_created_ep_count); - if (++m_pending_add_ep_count < m_created_ep_count) { + if (pending_add_ep_count < m_created_ep_count) { m_pending_reqs.push_back(req); return UCS_OK; } @@ -286,28 +339,20 @@ class test_ucp_worker_discard : public ucp_test { static ucs_status_t ep_pending_add_save_req(uct_ep_h ep, uct_pending_req_t *req, unsigned flags) { - ep_pending_reqs_map::iterator it = m_pending_reqs_map.find(ep); - if (it == m_pending_reqs_map.end()) { - std::vector vec; - vec.push_back(req); - m_pending_reqs_map.insert(std::make_pair(ep, vec)); - } else { - std::vector *req_vec = &it->second; - req_vec->push_back(req); - } + ep_test_info_t *test_info = ep_test_info_get(ep); + test_info->pending_reqs.push_back(req); return UCS_OK; } static void ep_pending_purge_func_iter_reqs(uct_ep_h ep, - uct_pending_purge_callback_t cb, - void *arg) { + uct_pending_purge_callback_t cb, + void *arg) { + ep_test_info_t *test_info = ep_test_info_get(ep); uct_pending_req_t *req; - for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - ep_pending_reqs_map::iterator it = m_pending_reqs_map.find(ep); - ASSERT_NE(it, m_pending_reqs_map.end()); - std::vector *req_vec = &it->second; + for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { + std::vector *req_vec = &test_info->pending_reqs; if (req_vec->size() == 0) { break; } @@ -321,26 +366,22 @@ class test_ucp_worker_discard : public ucp_test { protected: static unsigned m_created_ep_count; static unsigned m_destroyed_ep_count; - static unsigned m_flush_ep_count; - static unsigned m_pending_add_ep_count; static ucp_ep_t m_fake_ep; static const unsigned m_pending_purge_reqs_count; static std::vector m_flush_comps; static std::vector m_pending_reqs; - static ep_pending_reqs_map m_pending_reqs_map; + static ep_test_info_map_t m_ep_test_info_map; }; unsigned test_ucp_worker_discard::m_created_ep_count = 0; unsigned test_ucp_worker_discard::m_destroyed_ep_count = 0; -unsigned test_ucp_worker_discard::m_flush_ep_count = 0; -unsigned test_ucp_worker_discard::m_pending_add_ep_count = 0; ucp_ep_t test_ucp_worker_discard::m_fake_ep = {}; const unsigned test_ucp_worker_discard::m_pending_purge_reqs_count = 10; -std::vector test_ucp_worker_discard::m_flush_comps; -std::vector test_ucp_worker_discard::m_pending_reqs; -test_ucp_worker_discard::ep_pending_reqs_map test_ucp_worker_discard::m_pending_reqs_map; +std::vector test_ucp_worker_discard::m_flush_comps; +std::vector test_ucp_worker_discard::m_pending_reqs; +test_ucp_worker_discard::ep_test_info_map_t test_ucp_worker_discard::m_ep_test_info_map; UCS_TEST_P(test_ucp_worker_discard, flush_ok) { From 509dd6c951b60787f0e3e9c7ffd902c26431cdb9 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Mon, 7 Sep 2020 10:39:10 +0000 Subject: [PATCH 24/33] UCP/GTEST: Fix review comments --- src/ucp/core/ucp_worker.c | 54 ++++++++----------- src/ucp/wireup/wireup_ep.c | 90 +++++-------------------------- src/ucp/wireup/wireup_ep.h | 7 --- test/gtest/ucp/test_ucp_worker.cc | 70 ++++++------------------ 4 files changed, 50 insertions(+), 171 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index c1afba1a00e..1352e29cf39 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2440,33 +2440,25 @@ ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, static ucs_status_t ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { - uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; - ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); - uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; - ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; - ucs_status_t status, status_add; - - status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, - &req->send.state.uct_comp); - if (status == UCS_ERR_NO_RESOURCE) { - status_add = uct_ep_pending_add(uct_ep, &req->send.uct, 0); - ucs_assert((status_add == UCS_ERR_BUSY) || (status_add == UCS_OK)); - /* if added to the pending queue or to worker progress, need to remove - * the callback to not invoke it several times */ - if (status_add == UCS_ERR_BUSY) { - uct_worker_progress_register_safe(worker->uct, - ucp_worker_discard_uct_ep_progress, - req, UCS_CALLBACKQ_FLAG_ONESHOT, - &cb_id); - } + ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); + uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + ucs_status_t status; - return UCS_OK; - } else if (status == UCS_INPROGRESS) { - /* need to remove from the pending queue */ - status = UCS_OK; - } else { - ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); - } + do { + status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, + &req->send.state.uct_comp); + if (status == UCS_ERR_NO_RESOURCE) { + status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); + ucs_assert((status == UCS_OK) || (status == UCS_ERR_BUSY)); + } else if (status == UCS_INPROGRESS) { + /* need to remove from the pending queue */ + status = UCS_OK; + } else { + ucs_assert(status != UCS_ERR_BUSY); + ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, + status); + } + } while (status == UCS_ERR_BUSY); return status; } @@ -2492,9 +2484,7 @@ ucp_worker_discard_wireup_ep(ucp_worker_h worker, ucs_assert(wireup_ep != NULL); ucs_assert(purge_cb != NULL); - ucp_wireup_ep_pending_purge_common(wireup_ep, - purge_cb, purge_arg, - purge_cb, purge_arg); + uct_ep_pending_purge(&wireup_ep->super.super, purge_cb, purge_arg); if (wireup_ep->aux_ep != NULL) { /* make sure that there is no WIREUP MSGs anymore */ @@ -2513,9 +2503,9 @@ ucp_worker_discard_wireup_ep(ucp_worker_h worker, is_owner = wireup_ep->super.is_owner; uct_ep = ucp_wireup_ep_extract_next_ep(&wireup_ep->super.super); - /* destroy WIREUP EP allocated for this UCT EP, since - * discard operation most likely won't have an access to - * UCP EP as it could be destroyed by the caller */ + /* destroy WIREUP EP allocated for this UCT EP, since discard operation + * most likely won't have an access to UCP EP as it could be destroyed + * by the caller */ uct_ep_destroy(&wireup_ep->super.super); /* do nothing, if this wireup EP is not an owner for UCT EP */ diff --git a/src/ucp/wireup/wireup_ep.c b/src/ucp/wireup/wireup_ep.c index e5588ad36e6..82a0323957c 100644 --- a/src/ucp/wireup/wireup_ep.c +++ b/src/ucp/wireup/wireup_ep.c @@ -22,18 +22,6 @@ #include -typedef struct ucp_wireup_ep_pending_purge_arg { - ucp_wireup_ep_t *wireup_ep; /* WIREUP EP that owns the UCT - * pending request for the WIREUP - * MSG */ - uct_pending_purge_callback_t wireup_msg_cb; /* UCT pending purge cakkback that - * will handle WIREUP MSG pending - * request */ - void *wireup_msg_arg; /* UCT pending purge callback - * argument */ -} ucp_wireup_ep_pending_purge_arg_t; - - UCS_CLASS_DECLARE(ucp_wireup_ep_t, ucp_ep_h); @@ -160,11 +148,7 @@ ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self) status = req->func(req); if (status == UCS_OK) { - /* WIREUP EP pointer could be NULL in case of it was discarded, but this - * pending request was completed on the UCT EP or the WIREUP AUX EP */ - if (wireup_ep != NULL) { - ucs_atomic_sub32(&wireup_ep->pending_count, 1); - } + ucs_atomic_sub32(&wireup_ep->pending_count, 1); ucs_free(proxy_req); } return status; @@ -235,78 +219,30 @@ static ucs_status_t ucp_wireup_ep_pending_add(uct_ep_h uct_ep, } static void -ucp_wireup_ep_pending_purge_cb(uct_pending_req_t *self, void *arg) -{ - ucp_wireup_ep_pending_purge_arg_t *purge_arg = - (ucp_wireup_ep_pending_purge_arg_t*)arg; - ucp_wireup_ep_t *wireup_ep = purge_arg->wireup_ep; - uct_ep_h uct_ep = - ucp_wireup_ep_get_msg_ep(wireup_ep); - ucp_request_t *wireup_proxy_req = - ucs_container_of(self, ucp_request_t, send.uct); - - /* do purging on AUX EP or on UCT EP if WIREUP is an owner of it */ - if ((uct_ep == wireup_ep->aux_ep) || wireup_ep->super.is_owner) { - /* need to NULL the WIREUP EP in the WIREUP MSG proxy pending request - * to avoid dereferencing it when progressing prnding requests, since - * it will be destroyed */ - wireup_proxy_req->send.proxy.wireup_ep = NULL; - - purge_arg->wireup_msg_cb(self, purge_arg->wireup_msg_arg); - if (purge_arg->wireup_msg_cb != ucp_wireup_ep_pending_req_release) { - /* decrement the pending count, if it is not a request release - * callback */ - ucs_atomic_sub32(&wireup_ep->pending_count, 1); - } - } -} - -void -ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, - uct_pending_purge_callback_t wireup_msg_cb, - void *wireup_msg_arg, - uct_pending_purge_callback_t user_msg_cb, - void *user_msg_arg) +ucp_wireup_ep_pending_purge(uct_ep_h uct_ep, uct_pending_purge_callback_t cb, + void *arg) { - ucp_wireup_ep_pending_purge_arg_t purge_arg; - ucp_worker_h worker; + ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep); + ucp_worker_h worker; uct_pending_req_t *req; - ucp_request_t *ucp_req; - uct_ep_h uct_ep; + ucp_request_t *ucp_req; worker = wireup_ep->super.ucp_ep->worker; - if (wireup_ep->pending_count > 0) { - uct_ep = ucp_wireup_ep_get_msg_ep(wireup_ep); - - purge_arg.wireup_ep = wireup_ep; - purge_arg.wireup_msg_cb = wireup_msg_cb; - purge_arg.wireup_msg_arg = wireup_msg_arg; - - uct_ep_pending_purge(uct_ep, - ucp_wireup_ep_pending_purge_cb, - &purge_arg); - } - - ucs_assert(wireup_ep->pending_count == 0); - ucs_queue_for_each_extract(req, &wireup_ep->pending_q, priv, 1) { ucp_req = ucs_container_of(req, ucp_request_t, send.uct); UCS_ASYNC_BLOCK(&worker->async); --worker->flush_ops_count; UCS_ASYNC_UNBLOCK(&worker->async); - user_msg_cb(&ucp_req->send.uct, user_msg_arg); + cb(&ucp_req->send.uct, arg); } -} -static void -ucp_wireup_ep_pending_purge(uct_ep_h uct_ep, uct_pending_purge_callback_t cb, - void *arg) -{ - ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep); - ucp_wireup_ep_pending_purge_common(wireup_ep, - ucp_wireup_ep_pending_req_release, NULL, - cb, arg); + if (wireup_ep->pending_count > 0) { + uct_ep_pending_purge(ucp_wireup_ep_get_msg_ep(wireup_ep), + ucp_wireup_ep_pending_req_release, arg); + } + + ucs_assert(wireup_ep->pending_count == 0); } static ssize_t ucp_wireup_ep_am_bcopy(uct_ep_h uct_ep, uint8_t id, diff --git a/src/ucp/wireup/wireup_ep.h b/src/ucp/wireup/wireup_ep.h index 7786710fb92..b2b309b4c19 100644 --- a/src/ucp/wireup/wireup_ep.h +++ b/src/ucp/wireup/wireup_ep.h @@ -104,13 +104,6 @@ void ucp_wireup_ep_disown(uct_ep_h uct_ep, uct_ep_h owned_ep); ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self); -void -ucp_wireup_ep_pending_purge_common(ucp_wireup_ep_t *wireup_ep, - uct_pending_purge_callback_t wireup_msg_cb, - void *wireup_msg_arg, - uct_pending_purge_callback_t user_msg_cb, - void *user_msg_arg); - void ucp_wireup_ep_replay_pending_requests(ucp_ep_h ucp_ep, ucs_queue_head_t *tmp_pending_queue); diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 3deffd1a654..e668e20bcd7 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -55,16 +55,10 @@ class test_ucp_worker_discard : public ucp_test { "ucp_request")); ASSERT_TRUE(req != NULL); - pending_reqs[base + i] = req; + pending_reqs.push_back(req); - if (func == ucp_wireup_msg_progress) { - - req->send.ep = &m_fake_ep; - - /* for fast completing the WIREUP MSG, it frees the send - * buffer and returns UCS_OK */ - req->send.wireup.type = UCP_WIREUP_MSG_REQUEST; - ASSERT_TRUE(m_fake_ep.flags & UCP_EP_FLAG_REMOTE_CONNECTED); + if (func == ucp_wireup_msg_progress) { + req->send.ep = &m_fake_ep; } req->send.uct.func = func; @@ -104,25 +98,7 @@ class test_ucp_worker_discard : public ucp_test { eps[i].iface = &iface; m_created_ep_count++; - unsigned expected_pending_purge_reqs_count = 0; - unsigned added_pending_purge_reqs_count = 0; - - if (ep_pending_purge_func == ep_pending_purge_func_iter_reqs) { - /* expected purging count is the number of pending - * requests in the WIREUP EP (if used) and in the - * WIREUP AUX EP (if used) or UCT EP (if no WIREUP EP - * or no WIREUP AUX EP) */ - expected_pending_purge_reqs_count += - m_pending_purge_reqs_count; - - if (i < wireup_ep_count) { - expected_pending_purge_reqs_count += - m_pending_purge_reqs_count; - } - } - - std::vector - pending_reqs(expected_pending_purge_reqs_count); + std::vector pending_reqs; if (i < wireup_ep_count) { status = ucp_wireup_ep_create(&ucp_ep, &discard_ep); @@ -144,15 +120,13 @@ class test_ucp_worker_discard : public ucp_test { m_created_ep_count++; } - if (expected_pending_purge_reqs_count > 0) { + if (ep_pending_purge_func == ep_pending_purge_func_iter_reqs) { /* add WIREUP MSGs to the WIREUP EP (it will be added to * UCT EP or WIREUP AUX EP) */ add_pending_reqs(discard_ep, (uct_pending_callback_t) ucp_wireup_msg_progress, pending_reqs); - added_pending_purge_reqs_count += - m_pending_purge_reqs_count; } } else { discard_ep = &eps[i]; @@ -161,26 +135,25 @@ class test_ucp_worker_discard : public ucp_test { EXPECT_LE(m_created_ep_count, total_ep_count); - if (expected_pending_purge_reqs_count > 0) { + if (ep_pending_purge_func == ep_pending_purge_func_iter_reqs) { /* add user's pending requests */ add_pending_reqs(discard_ep, (uct_pending_callback_t) ucs_empty_function, - pending_reqs, - added_pending_purge_reqs_count); - added_pending_purge_reqs_count += - m_pending_purge_reqs_count; + pending_reqs); } - EXPECT_EQ(expected_pending_purge_reqs_count, - added_pending_purge_reqs_count); - unsigned purged_reqs_count = 0; ucp_worker_discard_uct_ep(sender().worker(), discard_ep, UCT_FLUSH_FLAG_LOCAL, ep_pending_purge_count_reqs_cb, &purged_reqs_count); - EXPECT_EQ(expected_pending_purge_reqs_count, purged_reqs_count); + + if (ep_pending_purge_func == ep_pending_purge_func_iter_reqs) { + EXPECT_EQ(m_pending_purge_reqs_count, purged_reqs_count); + } else { + EXPECT_EQ(0u, purged_reqs_count); + } } void *flush_req = sender().flush_worker_nb(0); @@ -319,21 +292,8 @@ class test_ucp_worker_discard : public ucp_test { ucp_request_t, send.uct); - if (self->func == ucp_wireup_ep_progress_pending) { - /* need to complete WIREUP MSG to release allocated - * proxy request */ - /* TODO: replace by `ucp_request_send()` when - * `ucp/core/ucp_request.inl` file could be compiled - * by C++ compiler */ - ucs_status_t status = req->send.uct.func(&req->send.uct); - EXPECT_EQ(UCS_OK, status); - /* no need to release the memory allocated for the request. - * it will be freed in the `ucp_wireup_msg_send()` function, - * since it expects that the request was allocated in the - * `ucp_wireup_msg_send()` function */ - } else { - ucs_free(req); - } + ASSERT_TRUE(self->func != ucp_wireup_ep_progress_pending); + ucs_free(req); } static ucs_status_t From 07b8bb318d3b06c1bf74b5cadfa723be7a9692c6 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Mon, 7 Sep 2020 10:52:43 +0000 Subject: [PATCH 25/33] UCP/RMA: Remove flush_ops_count_check --- src/ucp/rma/flush.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index 7030de06571..da9a3216925 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -378,12 +378,6 @@ UCS_PROFILE_FUNC(ucs_status_ptr_t, ucp_ep_flush_nbx, (ep, param), return request; } -static UCS_F_ALWAYS_INLINE int -ucp_worker_flush_ops_count_check(ucp_worker_h worker) -{ - return !worker->flush_ops_count; -} - static ucs_status_t ucp_worker_flush_check(ucp_worker_h worker) { ucp_rsc_index_t iface_id; @@ -446,7 +440,7 @@ static unsigned ucp_worker_flush_progress(void *arg) ucs_status_t status; ucp_ep_h ep; - if (ucp_worker_flush_ops_count_check(worker)) { + if (!worker->flush_ops_count) { status = ucp_worker_flush_check(worker); if ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps)) { /* If all ifaces are flushed, or we finished going over all @@ -498,10 +492,10 @@ ucp_worker_flush_nbx_internal(ucp_worker_h worker, ucs_status_t status; ucp_request_t *req; - if (ucp_worker_flush_ops_count_check(worker)) { + if (!worker->flush_ops_count) { status = ucp_worker_flush_check(worker); if ((status != UCS_INPROGRESS) && (status != UCS_ERR_NO_RESOURCE)) { - /* UCS_OK could be returned here */ + /* UCS_OK is returned here as well */ return UCS_STATUS_PTR(status); } } From 43c3e921c02501eee52373224cc43eac3fcabfff Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 8 Sep 2020 04:18:38 +0000 Subject: [PATCH 26/33] UCP/GTEST: Fix review comments --- src/ucp/core/ucp_worker.c | 6 ++-- src/ucp/wireup/wireup_ep.c | 4 +-- test/gtest/ucp/test_ucp_worker.cc | 51 ++++++++++++++++--------------- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 1352e29cf39..c14264e4a6d 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2520,7 +2520,6 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, { uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; ucp_request_t *req; - khiter_t UCS_V_UNUSED iter; int ret; ucs_assert(uct_ep != NULL); @@ -2544,9 +2543,8 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, } ++worker->flush_ops_count; - iter = kh_put(ucp_worker_discard_uct_ep_hash, - &worker->discard_uct_ep_hash, - uct_ep, &ret); + kh_put(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, + uct_ep, &ret); if (ret == UCS_KH_PUT_FAILED) { ucs_fatal("failed to put %p UCT EP into the %p worker hash", uct_ep, worker); diff --git a/src/ucp/wireup/wireup_ep.c b/src/ucp/wireup/wireup_ep.c index 82a0323957c..53c91469e72 100644 --- a/src/ucp/wireup/wireup_ep.c +++ b/src/ucp/wireup/wireup_ep.c @@ -141,8 +141,8 @@ static uct_ep_h ucp_wireup_ep_get_msg_ep(ucp_wireup_ep_t *wireup_ep) ucs_status_t ucp_wireup_ep_progress_pending(uct_pending_req_t *self) { - ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct); - uct_pending_req_t *req = proxy_req->send.proxy.req; + ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct); + uct_pending_req_t *req = proxy_req->send.proxy.req; ucp_wireup_ep_t *wireup_ep = proxy_req->send.proxy.wireup_ep; ucs_status_t status; diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index e668e20bcd7..5faa28a2512 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -29,6 +29,12 @@ class test_ucp_worker_discard : public ucp_test { std::vector pending_reqs; unsigned flush_count; unsigned pending_add_count; + + ep_test_info_t() { + pending_reqs.clear(); + flush_count = 0; + pending_add_count = 0; + } }; typedef std::map ep_test_info_map_t; @@ -188,18 +194,18 @@ class test_ucp_worker_discard : public ucp_test { EXPECT_EQ(m_created_ep_count, total_ep_count); for (unsigned i = 0; i < m_created_ep_count; i++) { - ep_test_info_t *test_info = ep_test_info_get(&eps[i]); + ep_test_info_t &test_info = ep_test_info_get(&eps[i]); /* check EP flush counters */ if (ep_flush_func == ep_flush_func_return_3_no_resource_then_ok) { - EXPECT_EQ(4, test_info->flush_count); + EXPECT_EQ(4, test_info.flush_count); } else if (ep_flush_func == ep_flush_func_return_in_progress) { - EXPECT_EQ(1, test_info->flush_count); + EXPECT_EQ(1, test_info.flush_count); } /* check EP pending add counters */ if (ep_pending_add_func == ep_pending_add_func_return_ok_then_busy) { - EXPECT_EQ(3, test_info->pending_add_count); + EXPECT_EQ(3, test_info.pending_add_count); } } @@ -220,34 +226,31 @@ class test_ucp_worker_discard : public ucp_test { m_destroyed_ep_count++; } - static ep_test_info_t* ep_test_info_get(uct_ep_h ep) { - ep_test_info_t *test_info_p; + static ep_test_info_t& ep_test_info_get(uct_ep_h ep) { ep_test_info_map_t::iterator it = m_ep_test_info_map.find(ep); if (it == m_ep_test_info_map.end()) { - ep_test_info_t test_info = {}; + ep_test_info_t test_info; m_ep_test_info_map.insert(std::make_pair(ep, test_info)); - test_info_p = &m_ep_test_info_map.find(ep)->second; - } else { - test_info_p = &it->second; + it = m_ep_test_info_map.find(ep); } - return test_info_p; + return it->second; } static unsigned ep_test_info_flush_inc(uct_ep_h ep) { - ep_test_info_t *test_info = ep_test_info_get(ep); - test_info->flush_count++; - return test_info->flush_count; + ep_test_info_t &test_info = ep_test_info_get(ep); + test_info.flush_count++; + return test_info.flush_count; } static unsigned ep_test_info_pending_add_inc(uct_ep_h ep) { - ep_test_info_t *test_info = ep_test_info_get(ep); - test_info->pending_add_count++; - return test_info->pending_add_count; + ep_test_info_t &test_info = ep_test_info_get(ep); + test_info.pending_add_count++; + return test_info.pending_add_count; } static ucs_status_t @@ -299,8 +302,8 @@ class test_ucp_worker_discard : public ucp_test { static ucs_status_t ep_pending_add_save_req(uct_ep_h ep, uct_pending_req_t *req, unsigned flags) { - ep_test_info_t *test_info = ep_test_info_get(ep); - test_info->pending_reqs.push_back(req); + ep_test_info_t &test_info = ep_test_info_get(ep); + test_info.pending_reqs.push_back(req); return UCS_OK; } @@ -308,17 +311,17 @@ class test_ucp_worker_discard : public ucp_test { ep_pending_purge_func_iter_reqs(uct_ep_h ep, uct_pending_purge_callback_t cb, void *arg) { - ep_test_info_t *test_info = ep_test_info_get(ep); + ep_test_info_t &test_info = ep_test_info_get(ep); uct_pending_req_t *req; for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) { - std::vector *req_vec = &test_info->pending_reqs; - if (req_vec->size() == 0) { + std::vector &req_vec = test_info.pending_reqs; + if (req_vec.size() == 0) { break; } - req = req_vec->back(); - req_vec->pop_back(); + req = req_vec.back(); + req_vec.pop_back(); cb(req, arg); } } From 9153e506ed4d187aa2c5d027040a0159894e4d65 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 8 Sep 2020 04:22:55 +0000 Subject: [PATCH 27/33] UCP/CORE: Added useful comments --- src/ucp/core/ucp_worker.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index c14264e4a6d..57789a8ad27 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2454,7 +2454,10 @@ ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) /* need to remove from the pending queue */ status = UCS_OK; } else { + /* make sure that uct_ep_flush() doenst return UCS_ERR_BUSY to not + * prevent the endless loop in this case */ ucs_assert(status != UCS_ERR_BUSY); + /* UCS_OK is handled here as well */ ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); } From 9c346d6bbac8f90a5e175dae86bf46bdc2c43e8d Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 8 Sep 2020 04:54:43 +0000 Subject: [PATCH 28/33] UCP/RMA: Add more comments for flush to make it clear + fix typo --- src/ucp/core/ucp_worker.c | 2 +- src/ucp/rma/flush.c | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 57789a8ad27..39a615d7b1c 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2454,7 +2454,7 @@ ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) /* need to remove from the pending queue */ status = UCS_OK; } else { - /* make sure that uct_ep_flush() doenst return UCS_ERR_BUSY to not + /* make sure that uct_ep_flush() does not return UCS_ERR_BUSY to * prevent the endless loop in this case */ ucs_assert(status != UCS_ERR_BUSY); /* UCS_OK is handled here as well */ diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index da9a3216925..8d86e2e4eaf 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -440,18 +440,19 @@ static unsigned ucp_worker_flush_progress(void *arg) ucs_status_t status; ucp_ep_h ep; - if (!worker->flush_ops_count) { + if (!worker->flush_ops_count) /* all scheduled operations on worker + * were completed */ { status = ucp_worker_flush_check(worker); if ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps)) { /* If all ifaces are flushed, or we finished going over all - * endpoints, and all scheduled operations on worker were - * completed or iface flush failed with error, no need to - * progress this request actively anymore. - */ + * endpoints, no need to progress this request actively anymore + * and we complete the flush operation with UCS_OK status. */ ucp_worker_flush_complete_one(req, UCS_OK, 1); goto out; } else if (status != UCS_INPROGRESS) { - /* Error returned from uct iface flush */ + /* Error returned from uct iface flush, no need to progress + * this request actively anymore and we complete the flush + * operation with an error status. */ ucp_worker_flush_complete_one(req, status, 1); goto out; } From 465be856738c5f9f681e7e37222592f4a1cee029 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 8 Sep 2020 09:50:21 +0000 Subject: [PATCH 29/33] GTEST/UCP: Fix review comments --- test/gtest/ucp/test_ucp_worker.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 5faa28a2512..2f19157f03c 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -30,10 +30,7 @@ class test_ucp_worker_discard : public ucp_test { unsigned flush_count; unsigned pending_add_count; - ep_test_info_t() { - pending_reqs.clear(); - flush_count = 0; - pending_add_count = 0; + ep_test_info_t() : flush_count(0), pending_add_count(0) { } }; typedef std::map ep_test_info_map_t; @@ -242,15 +239,13 @@ class test_ucp_worker_discard : public ucp_test { static unsigned ep_test_info_flush_inc(uct_ep_h ep) { ep_test_info_t &test_info = ep_test_info_get(ep); - test_info.flush_count++; - return test_info.flush_count; + return ++test_info.flush_count; } static unsigned ep_test_info_pending_add_inc(uct_ep_h ep) { ep_test_info_t &test_info = ep_test_info_get(ep); - test_info.pending_add_count++; - return test_info.pending_add_count; + return ++test_info.pending_add_count; } static ucs_status_t From cdb88759b704a78b9600f33e1d962f055df7c5df Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Tue, 8 Sep 2020 12:38:42 +0000 Subject: [PATCH 30/33] UCP/CORE: Use progress instead of loop over ERR_BUSY --- src/ucp/core/ucp_worker.c | 44 +++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index 39a615d7b1c..a62dc9c2acd 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2440,30 +2440,34 @@ ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, static ucs_status_t ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { - ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); - uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); + uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; ucs_status_t status; - do { - status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, - &req->send.state.uct_comp); - if (status == UCS_ERR_NO_RESOURCE) { - status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); - ucs_assert((status == UCS_OK) || (status == UCS_ERR_BUSY)); - } else if (status == UCS_INPROGRESS) { - /* need to remove from the pending queue */ - status = UCS_OK; - } else { - /* make sure that uct_ep_flush() does not return UCS_ERR_BUSY to - * prevent the endless loop in this case */ - ucs_assert(status != UCS_ERR_BUSY); - /* UCS_OK is handled here as well */ - ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, - status); + status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, + &req->send.state.uct_comp); + if (status == UCS_ERR_NO_RESOURCE) { + status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); + ucs_assert((status == UCS_ERR_BUSY) || (status == UCS_OK)); + if (status == UCS_ERR_BUSY) { + uct_worker_progress_register_safe(worker->uct, + ucp_worker_discard_uct_ep_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, + &cb_id); } - } while (status == UCS_ERR_BUSY); + } else if (status != UCS_INPROGRESS) { + /* UCS_OK is handled here as well */ + ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); + return status; + } - return status; + /* the request was added to the UCT pending queue or to the UCT worker + * progress, need to return UCS_OK in order to remove the callback from + * the previous place (either UCT pending queue or UCT worker progress) + * to not invoke it several times */ + return UCS_OK; } static unsigned ucp_worker_discard_uct_ep_progress(void *arg) From 2a5888f0592a5a20536e53db213e6a59277b6da5 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 9 Sep 2020 06:37:56 +0000 Subject: [PATCH 31/33] UCP/GTEST: Fix review comments --- src/ucp/core/ucp_worker.c | 60 ++++++++++++++++--------------- src/ucp/rma/flush.c | 4 +-- test/gtest/ucp/test_ucp_worker.cc | 3 +- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index a62dc9c2acd..d578cd3cd4a 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -110,10 +110,6 @@ KHASH_IMPL(ucp_worker_discard_uct_ep_hash, uct_ep_h, char, 0, ucp_worker_discard_uct_ep_hash_key, kh_int64_hash_equal); -/* Forward declaration to use in the dicarding of UCT EP pending callback */ -static unsigned ucp_worker_discard_uct_ep_progress(void *arg); - - static ucs_status_t ucp_worker_wakeup_ctl_fd(ucp_worker_h worker, ucp_worker_event_fd_op_t op, int event_fd) @@ -2440,42 +2436,49 @@ ucp_worker_discard_uct_ep_flush_comp(uct_completion_t *self, static ucs_status_t ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) { - uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; ucp_request_t *req = ucs_container_of(self, ucp_request_t, send.uct); uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; - ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; ucs_status_t status; status = uct_ep_flush(uct_ep, req->send.discard_uct_ep.ep_flush_flags, &req->send.state.uct_comp); + if (status == UCS_INPROGRESS) { + return UCS_OK; + } + + /* UCS_OK is handled here as well */ + if (status != UCS_ERR_NO_RESOURCE) { + ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, + status); + } + return status; +} + +static unsigned ucp_worker_discard_uct_ep_progress(void *arg) +{ + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req = (ucp_request_t*)arg; + uct_ep_h uct_ep = req->send.discard_uct_ep.uct_ep; + ucp_worker_h worker = req->send.discard_uct_ep.ucp_worker; + ucs_status_t status; + + status = ucp_worker_discard_uct_ep_pending_cb(&req->send.uct); if (status == UCS_ERR_NO_RESOURCE) { status = uct_ep_pending_add(uct_ep, &req->send.uct, 0); ucs_assert((status == UCS_ERR_BUSY) || (status == UCS_OK)); if (status == UCS_ERR_BUSY) { + /* adding to the pending queue failed, schedule the UCT EP discard + * operation on UCT worker progress again */ uct_worker_progress_register_safe(worker->uct, ucp_worker_discard_uct_ep_progress, req, UCS_CALLBACKQ_FLAG_ONESHOT, &cb_id); } - } else if (status != UCS_INPROGRESS) { - /* UCS_OK is handled here as well */ - ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, status); - return status; - } - /* the request was added to the UCT pending queue or to the UCT worker - * progress, need to return UCS_OK in order to remove the callback from - * the previous place (either UCT pending queue or UCT worker progress) - * to not invoke it several times */ - return UCS_OK; -} - -static unsigned ucp_worker_discard_uct_ep_progress(void *arg) -{ - ucp_request_t *req = (ucp_request_t*)arg; - ucs_status_t status = ucp_worker_discard_uct_ep_pending_cb(&req->send.uct); + return 0; + } - return !UCS_STATUS_IS_ERR(status); + return 1; } static uct_ep_h @@ -2489,12 +2492,10 @@ ucp_worker_discard_wireup_ep(ucp_worker_h worker, int is_owner; ucs_assert(wireup_ep != NULL); - ucs_assert(purge_cb != NULL); - - uct_ep_pending_purge(&wireup_ep->super.super, purge_cb, purge_arg); if (wireup_ep->aux_ep != NULL) { - /* make sure that there is no WIREUP MSGs anymore */ + /* make sure that there are no WIREUP MSGs anymore that are scheduled + * on AUX EP, i.e. the purge callback hasn't be invoked here */ uct_ep_pending_purge(wireup_ep->aux_ep, (uct_pending_purge_callback_t) ucs_empty_function_do_assert, @@ -2530,6 +2531,9 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, int ret; ucs_assert(uct_ep != NULL); + ucs_assert(purge_cb != NULL); + + uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); if (ucp_wireup_ep_test(uct_ep)) { uct_ep = ucp_worker_discard_wireup_ep(worker, ucp_wireup_ep(uct_ep), @@ -2538,8 +2542,6 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, if (uct_ep == NULL) { return; } - } else if (purge_cb != NULL) { - uct_ep_pending_purge(uct_ep, purge_cb, purge_arg); } req = ucp_request_get(worker); diff --git a/src/ucp/rma/flush.c b/src/ucp/rma/flush.c index 8d86e2e4eaf..02ebd510fa7 100644 --- a/src/ucp/rma/flush.c +++ b/src/ucp/rma/flush.c @@ -440,8 +440,8 @@ static unsigned ucp_worker_flush_progress(void *arg) ucs_status_t status; ucp_ep_h ep; - if (!worker->flush_ops_count) /* all scheduled operations on worker - * were completed */ { + if (worker->flush_ops_count == 0) { + /* all scheduled progress operations on worker were completed */ status = ucp_worker_flush_check(worker); if ((status == UCS_OK) || (&next_ep->ep_list == &worker->all_eps)) { /* If all ifaces are flushed, or we finished going over all diff --git a/test/gtest/ucp/test_ucp_worker.cc b/test/gtest/ucp/test_ucp_worker.cc index 2f19157f03c..7c41104c247 100644 --- a/test/gtest/ucp/test_ucp_worker.cc +++ b/test/gtest/ucp/test_ucp_worker.cc @@ -202,7 +202,8 @@ class test_ucp_worker_discard : public ucp_test { /* check EP pending add counters */ if (ep_pending_add_func == ep_pending_add_func_return_ok_then_busy) { - EXPECT_EQ(3, test_info.pending_add_count); + /* pending_add has to be called only once per EP */ + EXPECT_EQ(1, test_info.pending_add_count); } } From d3cf051ba591e0ee838df369434cec53dd2da2a2 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 9 Sep 2020 14:27:38 +0000 Subject: [PATCH 32/33] UCP/WORKER: Fix review comments --- src/ucp/core/ucp_worker.c | 94 ++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 41 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index d578cd3cd4a..a9fdf2262bf 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2044,6 +2044,12 @@ void ucp_worker_destroy(ucp_worker_h worker) ucs_trace_func("worker=%p", worker); UCS_ASYNC_BLOCK(&worker->async); + if (worker->flush_ops_count != 0) { + ucs_warn("not all pending operations (%u) were flushed on worker %p " + "that is being destroyed", + worker->flush_ops_count, worker); + } + ucp_worker_destroy_eps(worker); ucp_worker_remove_am_handlers(worker); ucp_am_cleanup(worker); @@ -2444,14 +2450,14 @@ ucp_worker_discard_uct_ep_pending_cb(uct_pending_req_t *self) &req->send.state.uct_comp); if (status == UCS_INPROGRESS) { return UCS_OK; + } else if (status == UCS_ERR_NO_RESOURCE) { + return UCS_ERR_NO_RESOURCE; } /* UCS_OK is handled here as well */ - if (status != UCS_ERR_NO_RESOURCE) { - ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, - status); - } - return status; + ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp, + status); + return UCS_OK; } static unsigned ucp_worker_discard_uct_ep_progress(void *arg) @@ -2481,6 +2487,45 @@ static unsigned ucp_worker_discard_uct_ep_progress(void *arg) return 1; } +static void +ucp_worker_discard_tl_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, + unsigned ep_flush_flags) +{ + uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; + ucp_request_t *req; + int ret; + + req = ucp_request_get(worker); + if (ucs_unlikely(req == NULL)) { + ucs_error("unable to allocate request for discarding UCT EP %p " + "on UCP worker %p", uct_ep, worker); + return; + } + + ++worker->flush_ops_count; + kh_put(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, + uct_ep, &ret); + if (ret == UCS_KH_PUT_FAILED) { + ucs_fatal("failed to put %p UCT EP into the %p worker hash", + uct_ep, worker); + } else if (ret == UCS_KH_PUT_KEY_PRESENT) { + ucs_fatal("%p UCT EP is already present in the %p worker hash", + uct_ep, worker); + } + + ucs_assert(!ucp_wireup_ep_test(uct_ep)); + req->send.uct.func = ucp_worker_discard_uct_ep_pending_cb; + req->send.state.uct_comp.func = ucp_worker_discard_uct_ep_flush_comp; + req->send.state.uct_comp.count = 1; + req->send.discard_uct_ep.ucp_worker = worker; + req->send.discard_uct_ep.uct_ep = uct_ep; + req->send.discard_uct_ep.ep_flush_flags = ep_flush_flags; + uct_worker_progress_register_safe(worker->uct, + ucp_worker_discard_uct_ep_progress, + req, UCS_CALLBACKQ_FLAG_ONESHOT, + &cb_id); +} + static uct_ep_h ucp_worker_discard_wireup_ep(ucp_worker_h worker, ucp_wireup_ep_t *wireup_ep, @@ -2502,9 +2547,8 @@ ucp_worker_discard_wireup_ep(ucp_worker_h worker, NULL); /* discard the WIREUP EP's auxiliary EP */ - ucp_worker_discard_uct_ep(worker, wireup_ep->aux_ep, - ep_flush_flags, - purge_cb, purge_arg); + ucp_worker_discard_tl_uct_ep(worker, wireup_ep->aux_ep, + ep_flush_flags); ucp_wireup_ep_disown(&wireup_ep->super.super, wireup_ep->aux_ep); } @@ -2526,10 +2570,6 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, uct_pending_purge_callback_t purge_cb, void *purge_arg) { - uct_worker_cb_id_t cb_id = UCS_CALLBACKQ_ID_NULL; - ucp_request_t *req; - int ret; - ucs_assert(uct_ep != NULL); ucs_assert(purge_cb != NULL); @@ -2544,33 +2584,5 @@ void ucp_worker_discard_uct_ep(ucp_worker_h worker, uct_ep_h uct_ep, } } - req = ucp_request_get(worker); - if (ucs_unlikely(req == NULL)) { - ucs_error("unable to allocate request for discarding UCT EP %p " - "on UCP worker %p", uct_ep, worker); - return; - } - - ++worker->flush_ops_count; - kh_put(ucp_worker_discard_uct_ep_hash, &worker->discard_uct_ep_hash, - uct_ep, &ret); - if (ret == UCS_KH_PUT_FAILED) { - ucs_fatal("failed to put %p UCT EP into the %p worker hash", - uct_ep, worker); - } else if (ret == UCS_KH_PUT_KEY_PRESENT) { - ucs_fatal("%p UCT EP is already present in the %p worker hash", - uct_ep, worker); - } - - ucs_assert(!ucp_wireup_ep_test(uct_ep)); - req->send.uct.func = ucp_worker_discard_uct_ep_pending_cb; - req->send.state.uct_comp.func = ucp_worker_discard_uct_ep_flush_comp; - req->send.state.uct_comp.count = 1; - req->send.discard_uct_ep.ucp_worker = worker; - req->send.discard_uct_ep.uct_ep = uct_ep; - req->send.discard_uct_ep.ep_flush_flags = ep_flush_flags; - uct_worker_progress_register_safe(worker->uct, - ucp_worker_discard_uct_ep_progress, - req, UCS_CALLBACKQ_FLAG_ONESHOT, - &cb_id); + ucp_worker_discard_tl_uct_ep(worker, uct_ep, ep_flush_flags); } From ea18b512528147e968eed235d1a8fdc115708f98 Mon Sep 17 00:00:00 2001 From: dmitrygx Date: Wed, 9 Sep 2020 18:52:28 +0000 Subject: [PATCH 33/33] UCP/CORE: Let UCP Wireup EP be destroyed in case of error --- src/ucp/core/ucp_worker.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ucp/core/ucp_worker.c b/src/ucp/core/ucp_worker.c index a9fdf2262bf..523640438e2 100644 --- a/src/ucp/core/ucp_worker.c +++ b/src/ucp/core/ucp_worker.c @@ -2044,16 +2044,16 @@ void ucp_worker_destroy(ucp_worker_h worker) ucs_trace_func("worker=%p", worker); UCS_ASYNC_BLOCK(&worker->async); + ucp_worker_destroy_eps(worker); + ucp_worker_remove_am_handlers(worker); + ucp_am_cleanup(worker); + ucp_worker_close_cms(worker); + if (worker->flush_ops_count != 0) { ucs_warn("not all pending operations (%u) were flushed on worker %p " "that is being destroyed", worker->flush_ops_count, worker); } - - ucp_worker_destroy_eps(worker); - ucp_worker_remove_am_handlers(worker); - ucp_am_cleanup(worker); - ucp_worker_close_cms(worker); UCS_ASYNC_UNBLOCK(&worker->async); ucp_worker_destroy_ep_configs(worker);