IODEMO: non blocking connection establishment #6338

evgeny-leksikov · 2021-02-14T08:23:16Z

What

implement non-blocking connection establishment

Why ?

to improve scalability

mike-dubman · 2021-02-14T08:29:01Z

test/apps/iodemo/io_demo.cc

+                _client.active_servers_add(_server_idx);
+                LOG << "Connected to " << _client.server_name(_server_idx);
+            } else {
+                /* connection should be deleted be upper layer */


s/be upper/on upper

evgeny-leksikov · 2021-02-17T12:15:21Z

@gleon99 could you please review?

yosefe · 2021-02-17T13:10:27Z

bot:pipe:retest

test/apps/iodemo/ucx_wrapper.h

test/apps/iodemo/io_demo.cc

test/apps/iodemo/ucx_wrapper.h

test/apps/iodemo/ucx_wrapper.cc

test/apps/iodemo/ucx_wrapper.h

test/apps/iodemo/io_demo.cc

gleon99 · 2021-02-18T13:38:43Z

test/apps/iodemo/io_demo.cc

-            LOG << "Connected to " << server_name(server_index);
+            connect(server_index);
+            assert(server_info.conn != NULL);
+            assert(_status == OK);


Any chance _status can not be modified by a call to connect(), thus there's no point in checking it (after line 1359 check)?

IMHO there is a point until Connection implementation can affect UcxContext state
I replaced if condition with assert because connect is changed this way

i don't see where connect() can modify _status?

via callback

gleon99 · 2021-02-24T14:35:32Z

@yosefe , can review please?
@evgeny-leksikov , need to fix pipeline, then recheck clang-format warnings.

yosefe · 2021-02-28T10:42:21Z

test/apps/iodemo/io_demo.cc

+
+    private:
+        DemoClient &_client;
+        size_t _server_idx;


test/apps/iodemo/io_demo.cc

yosefe · 2021-02-28T10:45:15Z

test/apps/iodemo/io_demo.cc

@@ -1300,7 +1348,7 @@ class DemoClient : public P2pDemoCommon {
             ++server_index) {
            server_info_t& server_info = _server_info[server_index];
            if (server_info.conn != NULL) {
-                // Server is already connected
+                // Server is already connected or in progress


Already connecting to this server

yosefe · 2021-02-28T10:55:12Z

test/apps/iodemo/ucx_wrapper.cc

+        _connection.handle_connection_error(_status);
+    }
+
+    established(_status);


imo better call just one callback in case of error (either established of handle_connection_error) but not both

yosefe · 2021-02-28T10:56:37Z

test/apps/iodemo/ucx_wrapper.cc

+    _send_id_req = send_id_req;
+    _recv_id_req = recv_id_req;
+    _comp_count  = 2; // both send and recv requests should be completed


i think we need to manage requests inside UcxConnection class.
and remove UcxEstablishCallback
instead, UcxConnection will hold a pointer to UcxCallback to call when the connection is established

yosefe · 2021-02-28T10:59:12Z

test/apps/iodemo/ucx_wrapper.h

@@ -73,6 +104,16 @@ class UcxLog {
 * Holds UCX global context and worker
 */
 class UcxContext {
+    friend class UcxAcceptCallback;


can we avoid access to private methods?

yosefe · 2021-02-28T10:59:36Z

test/apps/iodemo/ucx_wrapper.h

+    static unsigned _num_instances;
+
+    UcxContext &_context;
+    UcxEstablishCallback *_establish_cb;
+    uint64_t _conn_id;
+    uint64_t _remote_conn_id;
+    char _log_prefix[MAX_LOG_PREFIX_SIZE];
+    ucp_ep_h _ep;
+    void *_close_request;
+    ucs_list_link_t _all_requests;
+    ucs_status_t _ucx_status;
+    bool _use_am;
+    bool _connected;


yosefe · 2021-02-28T10:59:54Z

test/apps/iodemo/ucx_wrapper.h

+    ucs_status_ptr_t _send_id_req;
+    ucs_status_ptr_t _recv_id_req;
+    unsigned _comp_count;
+    ucs_status_t _status;


align on column

test/apps/iodemo/io_demo.cc

yosefe · 2021-03-09T23:18:54Z

test/apps/iodemo/ucx_wrapper.cc

+    if (status == UCS_OK) {
+        _context.dispatch_connection_accepted(&_connection);
+    }


should we have some cleanup if it failed?

no, the conn will go thru common failed conn list (by ep err handler), the calback will be deleted outside in stream_recv_cb

test/apps/iodemo/ucx_wrapper.cc

yosefe · 2021-03-09T23:36:52Z

test/apps/iodemo/ucx_wrapper.cc


-    if (_connected) {


why removed this check?

the failed connection goes thru common failed list

do we avoid calling user callback if _connected is false?
if not, why not, since it was the case before this PR?

before this PR we handled failed connection before and after "connected" differently.
now _connected is needed only to handle active servers list on the app level

void UcxContext::progress_failed_connections() { while (!_failed_conns.empty()) { UcxConnection *conn = _failed_conns.front(); _failed_conns.pop_front(); if (conn->is_established) { dispatch_connection_error(conn); } else { conn->callback(error); } } }

test/apps/iodemo/ucx_wrapper.h

yosefe · 2021-03-10T16:12:10Z

test/apps/iodemo/ucx_wrapper.h

-    double                           _connect_timeout;
-    bool                             _use_am;
+    typedef std::map<uint64_t, UcxConnection*>            conn_map_t;
+    typedef std::list<std::pair<double, UcxConnection*> > timeout_conn_t;


many std::vector? to avoid malloc on every elem

yosefe · 2021-03-10T16:13:05Z

test/apps/iodemo/ucx_wrapper.cc


-    if (_connected) {


do we avoid calling user callback if _connected is false?
if not, why not, since it was the case before this PR?

yosefe · 2021-03-10T16:14:22Z

test/apps/iodemo/ucx_wrapper.cc

@@ -748,6 +750,8 @@ void UcxConnection::stream_recv_callback(void *request, ucs_status_t status,
    ucx_request *r      = reinterpret_cast<ucx_request*>(request);
    UcxConnection *conn = r->conn;

+    // need to check connection status first since it's already can be failed or
+    // disconnecting


can we check _connected field?

no, because it's always false here, conn->established(); will be called at line 757

yosefe · 2021-03-10T16:15:39Z

test/apps/iodemo/ucx_wrapper.cc

+        // send local connection id
+        void *sreq = ucp_stream_send_nb(_ep, &_conn_id, 1, dt_int,
+                                        stream_send_callback, 0);
+        // we can don't check status here, in case if the endpoint is failed


we do not have to check the status here, ...

yosefe · 2021-03-10T16:16:59Z

test/apps/iodemo/ucx_wrapper.cc

+    }
+
+    process_request("conn_id receive", rreq, callback);
+    if (UCS_PTR_IS_PTR(rreq)) {


if (UCS_PTR_IS_ERR(rreq)) {
return;
}

then can remove line 853

yosefe · 2021-03-10T16:18:16Z

test/apps/iodemo/ucx_wrapper.cc

+    _conn_id = reinterpret_cast<uint64_t>(_ep);
+    established();
+    process_request("connect AM", NULL, callback);
+    delete callback;


AFAIR in case of sen operation, the callback deletes itself. so in conn callback case, should be the same

connection keeps poiter to the callback, I would like to avoid freed poiter there, so IMO better to delete and NULL'ize the poiter in the same place.

after callback is called, set conn->callback = NULL, user should take care of its memory release

yosefe · 2021-03-11T12:12:10Z

test/apps/iodemo/ucx_wrapper.cc

+    _conn_id = reinterpret_cast<uint64_t>(_ep);
+    established();
+    process_request("connect AM", NULL, callback);
+    delete callback;


after callback is called, set conn->callback = NULL, user should take care of its memory release

yosefe · 2021-03-11T12:25:52Z

test/apps/iodemo/ucx_wrapper.cc


-    if (_connected) {


void UcxContext::progress_failed_connections() { while (!_failed_conns.empty()) { UcxConnection *conn = _failed_conns.front(); _failed_conns.pop_front(); if (conn->is_established) { dispatch_connection_error(conn); } else { conn->callback(error); } } }

yosefe · 2021-03-11T12:26:33Z

test/apps/iodemo/ucx_wrapper.h

@@ -224,6 +243,12 @@ class UcxConnection {
        return _ucx_status;
    }

+    bool is_established() const {
+        return _connected;


return _callback == NULL

remove _connected

yosefe · 2021-03-11T17:41:24Z

test/apps/iodemo/ucx_wrapper.cc

-    assert(conn->_establish_cb == r->callback);
-    common_request_callback(request, status);
-    delete conn->_establish_cb;
-    conn->_establish_cb = NULL;


maybe keep conn->_establish_cb = NULL;?

I moved it to established and handle_connection_error where the callback is invoked

yosefe · 2021-03-11T17:42:30Z

test/apps/iodemo/ucx_wrapper.cc

        return;
    }

    UCX_CONN_LOG << "detected error: " << ucs_status_string(status);
    _ucx_status = status;

    /* the upper layer should close the connection */
-    _context.handle_connection_error(this);
+    if (is_established()) {


isn't this some duplication of d71ac19#diff-d3eefc2ff6f38844ae09889a04bb916b5433731d5780f8a10bd9d6bf7862ab41R370?

no but I agree - naming looks confusing...

yosefe · 2021-03-12T15:08:50Z

test/apps/iodemo/ucx_wrapper.cc

 void UcxContext::dispatch_connection_accepted(UcxConnection* conn)
 {
 }

 void UcxContext::handle_connection_error(UcxConnection *conn)
 {
+    remove_connection_inprogress(conn);


do it only if not established?

yosefe · 2021-03-12T15:10:58Z

test/apps/iodemo/ucx_wrapper.cc

+        if (conn->is_established()) {
+            dispatch_connection_error(conn);
+        } else {
+            conn->handle_connection_error(conn->ucx_status());


maybe we should call a helper function here, and call this helper also from established():
PS. it's a bit confusing that "establish_cb" is both for success and error, while "established()" is called only in case of success

UcxConnection::connect_completed() { _context.remove_connection_inprogress(this); _context.add_connection(this); (*_establish_cb)(_status); _establish_cb = NULL; }

I dont think it's much better. The issue here that we have to handle failed connection differently outside depends on it's internal state. Maybe we should re-think how to advance connection state for simplicity - what should be done by app, context and connection itself

yosefe · 2021-03-21T14:46:41Z

test/apps/iodemo/ucx_wrapper.cc

+    } else {
+        established(UCS_PTR_STATUS(rreq));
+        if (rreq != NULL) {
+            return;


pls add comment /* failed to receive */

yosefe · 2021-03-21T14:48:36Z

test/apps/iodemo/ucx_wrapper.cc

-    if (_connected) {
-        /* the upper layer should close the connection */
+    /* the upper layer should close the connection */
+    if (_establish_cb != NULL) {


why not use is_established() ?

yosefe

pls squash

mike-dubman reviewed Feb 14, 2021

View reviewed changes

evgeny-leksikov requested a review from gleon99 February 15, 2021 09:08

gleon99 reviewed Feb 18, 2021

View reviewed changes

gleon99 approved these changes Feb 24, 2021

View reviewed changes

yosefe reviewed Feb 28, 2021

View reviewed changes

yosefe reviewed Mar 9, 2021

View reviewed changes

yosefe reviewed Mar 10, 2021

View reviewed changes

yosefe reviewed Mar 11, 2021

View reviewed changes

evgeny-leksikov requested a review from yosefe March 11, 2021 17:27

yosefe reviewed Mar 11, 2021

View reviewed changes

yosefe reviewed Mar 14, 2021

View reviewed changes

evgeny-leksikov force-pushed the iodemo_nonblock_connect branch from 4757a73 to 17f8007 Compare March 18, 2021 13:22

yosefe reviewed Mar 21, 2021

View reviewed changes

yosefe approved these changes Mar 22, 2021

View reviewed changes

IODEMO: non blocking connection establishment

9901186

evgeny-leksikov force-pushed the iodemo_nonblock_connect branch from d2bbaf1 to 9901186 Compare March 22, 2021 18:06

yosefe merged commit a59928d into openucx:master Mar 23, 2021

evgeny-leksikov mentioned this pull request Mar 23, 2021

IODEMO: non blocking connection establishment yosefe/ucx#128

Merged

IODEMO: non blocking connection establishment #6338

IODEMO: non blocking connection establishment #6338

Conversation

evgeny-leksikov commented Feb 14, 2021

What

Why ?

Choose a reason for hiding this comment

evgeny-leksikov commented Feb 17, 2021

yosefe commented Feb 17, 2021

gleon99 Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gleon99 commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe left a comment

Choose a reason for hiding this comment

gleon99 Feb 18, 2021 •

edited

Loading