UCP/CORE: Implement flush+destroy for UCT EPs on UCP Worker by dmitrygx · Pull Request #5608 · openucx/ucx

dmitrygx · 2020-08-24T08:20:48Z

What

Implement flush+destroy for UCT EPs on UCP Worker.

Why ?

To be able to do flush(CANCEL/LOCAL) and then destroy UCT EP when UCP EP could be destroyed.
So, UCT EP will be destroyed on UCP Worker.
It fixes possible undone outstanding operation on some transports (e.g. there was an issue on RC).

How ?

Implement ucp_worker_discard_uct_ep() that's called instead of uct_ep_destroy()

src/ucp/core/ucp_worker.c

src/ucp/core/ucp_request.h

src/ucp/core/ucp_worker.c

dmitrygx · 2020-08-25T06:03:08Z

/azp run

azure-pipelines · 2020-08-25T06:03:21Z

Azure Pipelines successfully started running 1 pipeline(s).

src/ucp/core/ucp_worker.c

dmitrygx · 2020-08-25T07:41:37Z

@yosefe could you review pls?

yosefe

is it possible to add gtest?

src/ucp/core/ucp_worker.c

dmitrygx · 2020-08-25T11:57:01Z

is it possible to add gtest?

added the tests

src/ucp/core/ucp_worker.c

test/gtest/ucp/test_ucp_worker.cc

dmitrygx · 2020-08-26T04:48:12Z

bot:pipe:retest

…d all EPs flushed

…CANCEL)

dmitrygx · 2020-08-27T05:27:01Z

@evgeny-leksikov could you review pls?
I've changed the implementation where we changed the order to check TX window emptiness before checking the ability to do TX in EP flush. This breaks some tests: data corruptions for the tests that check the data that was received (or places using RMA primitives). It happens, seems we reported UCS_OK from EP flush when detected that TX window is empty now, but UD isn't really able to send data.
So, the fix proposed in the last changes in this PR:
reset tx::max_psn to tx::psn + ca:cwnd in uct_ep_flush(CANCEL) - so it means that we are able to send, and uct_iface_flush()/uct_ep_flush(LOCAL) will return UCS_OK.
Before this fix, we reported UCS_NO_RESOURCE from uct_iface_flush()/uct_ep_flush(LOCAL), since tx::max_psn==tx::psn`

dmitrygx · 2020-08-27T07:17:37Z

bot:pipe:retest

brminich · 2020-09-07T06:32:17Z

src/ucp/rma/flush.c

-        /* Error returned from uct iface flush */
-        ucp_worker_flush_complete_one(req, status, 1);
-    } else if (worker->context->config.ext.flush_worker_eps) {
+    if (ucp_worker_flush_ops_count_check(worker)) {


why not leaving this check inside ucp_worker_flush_check? seems unnecessary
also checking !worker->flush_ops_count looks more clear than ucp_worker_flush_ops_count_check name

brminich · 2020-09-07T06:34:41Z

src/ucp/wireup/wireup_ep.c

+    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


brminich · 2020-09-07T06:36:52Z

src/ucp/wireup/wireup_ep.c

+    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                              =


minor: imo, using single line without alignment by = would be more readable in this case

brminich · 2020-09-07T06:37:49Z

src/ucp/wireup/wireup_ep.c

+    /* 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


brminich · 2020-09-07T06:40:37Z

test/gtest/ucp/test_ucp_worker.cc

+            pending_reqs[base + i] = req;
+
+            if (func == ucp_wireup_msg_progress) {
+


dmitrygx · 2020-09-07T11:34:18Z

bot:pipe:retest

dmitrygx · 2020-09-07T18:01:26Z

@evgeny-leksikov @brminich could you review pls?

evgeny-leksikov · 2020-09-07T18:57:54Z

test/gtest/ucp/test_ucp_worker.cc

        m_destroyed_ep_count++;
    }

+    static ep_test_info_t* ep_test_info_get(uct_ep_h ep) {


return type can be reference ep_test_info_t &

evgeny-leksikov · 2020-09-07T18:58:24Z

test/gtest/ucp/test_ucp_worker.cc

+        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 = {};


since it's c++ struct, better to implement a constructor

evgeny-leksikov · 2020-09-07T19:02:03Z

test/gtest/ucp/test_ucp_worker.cc


-            std::vector<uct_pending_req_t*> *req_vec = &it->second;
+        for (unsigned i = 0; i < m_pending_purge_reqs_count; i++) {
+            std::vector<uct_pending_req_t*> *req_vec = &test_info->pending_reqs;


std::vector<uct_pending_req_t*> &req_vec = ...

evgeny-leksikov · 2020-09-07T19:09:01Z

src/ucp/core/ucp_worker.c

+            /* need to remove from the pending queue */
+            status = UCS_OK;
+        } else {
+            ucs_assert(status != UCS_ERR_BUSY);


this assert looks wrong since status value is returned by flush here

this is not wrong, just to make sure that we will not enter the endless loop if uct_ep_flush() returns UCS_ERR_BUSY
added the comment

but uct_ep_flush doc does not say that it cannot return UCS_ERR_BUSY

but uct_ep_flush doc does not say that it cannot return UCS_ERR_BUSY

yes, will handle it

@yosefe what do you think to use progress_register() instead of this loop?
seems to be error-prone or overcomplicated if it will have several retrun/break/continue in the loop

evgeny-leksikov · 2020-09-07T19:09:41Z

src/ucp/core/ucp_worker.c

+        } else if (status == UCS_INPROGRESS) {
+            /* need to remove from the pending queue */
+            status = UCS_OK;
+        } else {


UCS_OK is not handled?

it is handled in the else section

brminich · 2020-09-07T19:51:04Z

src/ucp/core/ucp_worker.c

+    }
+
+    ++worker->flush_ops_count;
+    iter = kh_put(ucp_worker_discard_uct_ep_hash,


minor: maybe just kh_put(ucp_worker_discard_uct_ep_hash,...? Then no need in unused iter var

brminich · 2020-09-07T19:51:42Z

src/ucp/rma/flush.c

    ucp_worker_iface_t *wiface;
    ucs_status_t status;

-    if (worker->flush_ops_count) {


why is removed here and moved outside?

we want to check for flush_ops count separately from uct_iface_flush() that we check in this function

why, seems can just leave it returning UCS_INPROGRESS?

why, seems can just leave it returning UCS_INPROGRESS?

added a comment to make it clear
if it returns UCS_INPROGESS, but going over all EPs were done, we complete with UCS_OK, but we shouldn't

brminich · 2020-09-07T19:52:14Z

src/ucp/wireup/wireup_ep.c

 {
-    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);


single unrelated change in the file

brminich · 2020-09-08T04:34:05Z

src/ucp/core/ucp_worker.c

            /* need to remove from the pending queue */
            status = UCS_OK;
        } else {
+            /* make sure that uct_ep_flush() doenst return UCS_ERR_BUSY to not


dmitrygx · 2020-09-08T05:45:10Z

bot:pipe:retest

evgeny-leksikov · 2020-09-08T07:11:33Z

test/gtest/ucp/test_ucp_worker.cc

        unsigned                           pending_add_count;
+
+        ep_test_info_t() {
+            pending_reqs.clear();


not needed, std::vector has its own constructor.

evgeny-leksikov · 2020-09-08T07:14:35Z

test/gtest/ucp/test_ucp_worker.cc

+
+        ep_test_info_t() {
+            pending_reqs.clear();
+            flush_count       = 0;


minor: better to use initializer list

ep_test_info_t() : flush_count(0), pending_add_count(0) { }

evgeny-leksikov · 2020-09-08T07:17:12Z

test/gtest/ucp/test_ucp_worker.cc

+        ep_test_info_t &test_info = ep_test_info_get(ep);
+        test_info.flush_count++;
+        return test_info.flush_count;


minor: can be shorter
return ++ep_test_info_get(ep).flush_count;

evgeny-leksikov · 2020-09-08T07:19:29Z

src/ucp/wireup/wireup_ep.c

+    ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct);
+    uct_pending_req_t *req = proxy_req->send.proxy.req;


pls align on '='

it is not my code, I returned it back in the same state as it was

dmitrygx · 2020-09-08T13:15:54Z

@evgeny-leksikov is it ok now?

dmitrygx · 2020-09-08T13:54:29Z

@yosefe could you review pls?

src/ucp/core/ucp_worker.c

src/ucp/rma/flush.c

dmitrygx · 2020-09-09T11:58:31Z

@yosefe could you review pls?

yosefe · 2020-09-09T13:01:54Z

src/ucp/core/ucp_worker.c

+        ucp_worker_discard_uct_ep_flush_comp(&req->send.state.uct_comp,
+                                             status);
+    }
+    return status;


if status != NO_RESOURCE should also return UCS_OK

if flush returned ok, who will call the completion?

if status != NO_RESOURCE should also return UCS_OK

why? we should put to the pending and wait for the callback invocation

if flush returned ok, who will call the completion?

it will be done in ucp_worker_discard_uct_ep_flush_comp()

if the status is error which is not NO_RESOURCE, the flush failed, we cannot retry

ok

if the status is error which is not NO_RESOURCE, the flush failed, we cannot retry

ah, I see. fixed

yosefe · 2020-09-09T13:08:53Z

src/ucp/rma/flush.c

-        /* Error returned from uct iface flush */
-        ucp_worker_flush_complete_one(req, status, 1);
-    } else if (worker->context->config.ext.flush_worker_eps) {
+    if (worker->flush_ops_count == 0) {


BTW, do we require iface_flush completion today before destroying UCP worker (AFAIK not)?
since it can leak some endpoints which are still being flushed

BTW, do we require iface_flush completion today before destroying UCP worker (AFAIK not)?

we don't require, but now we require flushing all ops to be done and or iface_flush is completed or going over all EPs is done

need to check for leaks in worker destroy

yosefe · 2020-09-09T13:11:47Z

src/ucp/core/ucp_worker.c

+        uct_ep = ucp_worker_discard_wireup_ep(worker, ucp_wireup_ep(uct_ep),
+                                              ep_flush_flags,
+                                              purge_cb, purge_arg);


seems weird recursion: ucp_worker_discard_uct_ep->ucp_worker_discard_wireup_ep->ucp_worker_discard_uct_ep

maybe separate the 2nd part of this function to a helper:

discard_uct_ep()
if wireup:
discard_wireup_ep()
if next_ep_owner:
discard_uct_ep_helper(next_ep) [works only on regular uct_ep, not wireup]
else:
discard_uct_ep_helper(uct_ep)

but what the problem with this recursion?
ucp_worker_discard_uct_ep->ucp_worker_discard_wireup_ep for WIREUP EP
and ->ucp_worker_discard_uct_ep for AUX EP

imo would be better /wo recursion, since we know next_ep cannot be wireup_ep

dmitrygx · 2020-09-10T05:09:46Z

@yosefe could you review pls?

changchengx · 2022-02-15T08:42:02Z

Ported to yosefe#223

dmitrygx mentioned this pull request Aug 24, 2020

UCP/WIREUP: Implement multi-lane/shm support for CM #5582

Closed

evgeny-leksikov reviewed Aug 24, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

evgeny-leksikov reviewed Aug 24, 2020

View reviewed changes

src/ucp/core/ucp_request.h Outdated Show resolved Hide resolved

evgeny-leksikov approved these changes Aug 24, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

brminich reviewed Aug 24, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

brminich reviewed Aug 25, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

evgeny-leksikov reviewed Aug 25, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

brminich approved these changes Aug 25, 2020

View reviewed changes

evgeny-leksikov approved these changes Aug 25, 2020

View reviewed changes

dmitrygx force-pushed the topic/ucp/worker_discard branch from 1f909f2 to 6c0082f Compare August 25, 2020 07:29

yosefe reviewed Aug 25, 2020

View reviewed changes

brminich reviewed Aug 25, 2020

View reviewed changes

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

yosefe reviewed Aug 25, 2020

View reviewed changes

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

yosefe reviewed Aug 25, 2020

View reviewed changes

test/gtest/ucp/test_ucp_worker.cc Outdated Show resolved Hide resolved

yosefe reviewed Aug 25, 2020

View reviewed changes

test/gtest/ucp/test_ucp_worker.cc Show resolved Hide resolved

yosefe approved these changes Aug 25, 2020

View reviewed changes

UCP/CORE: Implement flush+destroy for UCT EPs on UCP Worker

f7b0e3c

dmitrygx force-pushed the topic/ucp/worker_discard branch from 7f94236 to f7b0e3c Compare August 25, 2020 20:00

dmitrygx added 4 commits August 26, 2020 08:57

UCP/GTEST: Complete worker_flus operation if there is no flush ops an…

627a8a5

…d all EPs flushed

UCT/IB: Fix uct_iface_flush()/uct_ep_flush(LOCAL) after uct_ep_flush(…

e9d3b1d

…CANCEL)

UCT/IB/UD: Reset max_psn instead of stopping TX

ecd8663

UCM/GTEST: Fix leftovers

8d4a9f9

brminich reviewed Sep 7, 2020

View reviewed changes

dmitrygx added 2 commits September 7, 2020 10:39

UCP/GTEST: Fix review comments

509dd6c

UCP/RMA: Remove flush_ops_count_check

07b8bb3

evgeny-leksikov reviewed Sep 7, 2020

View reviewed changes

brminich reviewed Sep 7, 2020

View reviewed changes

dmitrygx added 2 commits September 8, 2020 04:18

UCP/GTEST: Fix review comments

43c3e92

UCP/CORE: Added useful comments

9153e50

brminich reviewed Sep 8, 2020

View reviewed changes

UCP/RMA: Add more comments for flush to make it clear + fix typo

9c346d6

brminich approved these changes Sep 8, 2020

View reviewed changes

evgeny-leksikov reviewed Sep 8, 2020

View reviewed changes

dmitrygx added 2 commits September 8, 2020 09:50

GTEST/UCP: Fix review comments

465be85

UCP/CORE: Use progress instead of loop over ERR_BUSY

cdb8875

evgeny-leksikov approved these changes Sep 8, 2020

View reviewed changes

yosefe reviewed Sep 8, 2020

View reviewed changes

src/ucp/core/ucp_worker.c Show resolved Hide resolved

src/ucp/core/ucp_worker.c Show resolved Hide resolved

src/ucp/core/ucp_worker.c Outdated Show resolved Hide resolved

src/ucp/rma/flush.c Outdated Show resolved Hide resolved

src/ucp/rma/flush.c Outdated Show resolved Hide resolved

UCP/GTEST: Fix review comments

2a5888f

yosefe reviewed Sep 9, 2020

View reviewed changes

dmitrygx added 2 commits September 9, 2020 14:27

UCP/WORKER: Fix review comments

d3cf051

UCP/CORE: Let UCP Wireup EP be destroyed in case of error

ea18b51

yosefe approved these changes Sep 10, 2020

View reviewed changes

yosefe merged commit 7ede1ba into openucx:master Sep 10, 2020

changchengx mentioned this pull request Nov 22, 2021

discard unused uct_ep when the pre-created uct_ep more than needed yosefe/ucx#223

Merged

		pending_reqs[base + i] = req;

		if (func == ucp_wireup_msg_progress) {

		ucp_request_t *proxy_req = ucs_container_of(self, ucp_request_t, send.uct);
		uct_pending_req_t *req = proxy_req->send.proxy.req;

Conversation

dmitrygx commented Aug 24, 2020

What

Why ?

How ?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitrygx commented Aug 25, 2020

Uh oh!

azure-pipelines bot commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

dmitrygx commented Aug 25, 2020

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitrygx commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitrygx commented Aug 26, 2020

Uh oh!

dmitrygx commented Aug 27, 2020

Uh oh!

dmitrygx commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx commented Sep 7, 2020

Uh oh!

dmitrygx commented Sep 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brminich Sep 8, 2020 •

edited

Loading