UCP/CORE: Reduce size of generic EP extension for further usage #5848

dmitrygx · 2020-10-28T15:54:14Z

What

Reduce the size of generic EP extension for further usage

Why ?

Generic EP extension will be used by #5821 to have ucs_hlist_head_t for tracking UCP requests.

How ?

Introduce a pointer to a control extension that is placed in the generic extension. Control extension includes:

local and remote IDs
error callback
union for close_req and listener
other fields are in generic extension, since they are could not be moved to a control extension (e.g. we have to have a mechanism to translate flush_state or conn_match to generic extension, or even to EP)

dmitrygx · 2020-10-28T19:31:15Z

/azp run

azure-pipelines · 2020-10-28T19:31:30Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-10-29T13:36:59Z

@hoopoepg @brminich @evgeny-leksikov could you review pls?

dmitrygx · 2020-10-29T14:31:35Z

@yosefe could you review pls?

brminich · 2020-10-29T15:08:41Z

src/ucp/core/ucp_ep.c

+    ucp_ep_ext_gen(ep)->control_ext    = ucs_calloc(
+                                             1, sizeof(ucp_ep_ext_control_t),
+                                             "ep_control_ext");


indentation

brminich · 2020-10-29T15:09:05Z

src/ucp/core/ucp_ep.c

+    ep->cfg_index                      = UCP_WORKER_CFG_INDEX_NULL;
+    ep->worker                         = worker;
+    ep->am_lane                        = UCP_NULL_LANE;
+    ep->flags                          = 0;
+    ep->conn_sn                        = UCP_EP_MATCH_CONN_SN_MAX;
+    ucp_ep_ext_gen(ep)->user_data      = NULL;
+    ucp_ep_ext_gen(ep)->control_ext    = ucs_calloc(


can reduce number of spaces

brminich · 2020-10-29T15:12:05Z

src/ucp/core/ucp_ep.c

    UCS_STATIC_ASSERT(sizeof(ucp_ep_ext_gen(ep)->ep_match) >=
                      sizeof(ucp_ep_ext_gen(ep)->flush_state));
+    UCS_STATIC_ASSERT(sizeof(ucp_ep_ext_control(ep)->listener) ==


why is it required?

it's not needed
removed

brminich · 2020-10-29T15:14:18Z

src/ucp/core/ucp_ep.h

-    /* Endpoint match context and remote completion status are mutually exclusive,
-     * since remote completions are counted only after the endpoint is already
-     * matched to a remote peer.
+    /* Endpoint match context is mutually exclusive with:


is this comment relevant now? match is not here and they are not in the union anymore

yes, should remove it
and use the old one

brminich · 2020-10-29T15:15:09Z

src/ucp/core/ucp_ep.inl

+    ucs_queue_head_init(&flush_state->reqs);
    flush_state->send_sn = 0;
    flush_state->cmpl_sn = 0;
-    ucs_queue_head_init(&flush_state->reqs);
-    ep->flags |= UCP_EP_FLAG_FLUSH_STATE_VALID;
+    ep->flags           |= UCP_EP_FLAG_FLUSH_STATE_VALID;


dmitrygx · 2020-10-29T16:04:38Z

@yosefe could you review pls?

dmitrygx · 2020-10-29T18:53:44Z

the failure is #5856

dmitrygx · 2020-10-29T18:53:49Z

/azp run

azure-pipelines · 2020-10-29T18:54:05Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-10-30T13:29:15Z

/azp run

azure-pipelines · 2020-10-30T13:29:27Z

Azure Pipelines successfully started running 1 pipeline(s).

brminich · 2020-10-31T07:45:21Z

/azp run

azure-pipelines · 2020-10-31T07:45:32Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-11-02T07:54:42Z

bot:pipe:retest

dmitrygx · 2020-11-02T07:54:49Z

/azp run

azure-pipelines · 2020-11-02T07:55:01Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-11-02T17:37:50Z

src/ucp/core/ucp_ep.h

+ * Endpoint extension for control data path
+ */
+typedef struct {
+    ucp_ep_ids_t                  ids;           /* Local and remote IDs */


extra pointer deref for rndv/am protocols?

I see that remote_id is used for AMO/RMA/Stream. Is it used for rndv/am as well?
looks like we have to have it in ucp_ep_ext_gen_t

btw, it was a double pointer deref before this PR, since IDs were allocated on a heap
ucp_ep_ext_gen_t* -> ucp_ep_ids_t*
with this PR we have:
ucp_ep_ext_gen_t* -> ucp_ep_ext_control_t* and offset to the [0] or [1] ID

but if we would like to avoid it
we need to allocate 3 strides instead of having a pointer to control extension in general extension + we could move ucp_ep_match_elem_t field there (it is on general extension now, since we have to have an access to EP in UCP/EP_MATCH code)
wdyt?

can we move ep id's to a stride extension and all the rest to the heap?

decoupled local and remote IDs as dicussed

dmitrygx · 2020-11-04T20:21:05Z

bot:pipe:retest

dmitrygx · 2020-11-05T10:40:05Z

@yosefe could you review pls?

dmitrygx · 2020-11-05T11:42:33Z

/azp run

azure-pipelines · 2020-11-05T11:42:47Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-11-05T12:12:06Z

@yosefe do we need to restart IO demo and RTD somehow?

yosefe · 2020-11-05T12:13:16Z

@yosefe do we need to restart IO demo and RTD somehow?

no need

yosefe · 2020-11-05T12:13:20Z

/azp run

azure-pipelines · 2020-11-05T12:13:34Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-11-05T12:43:50Z

failure is #5875
bot:pipe:retest

dmitrygx · 2020-11-05T19:22:51Z

IO demo and RTD failures are unrelated
@yosefe ok to merge?

dmitrygx force-pushed the topic/ucp/ep_ext_gen_reduce branch from ed12e2d to 04084a0 Compare October 29, 2020 07:36

evgeny-leksikov approved these changes Oct 29, 2020

View reviewed changes

brminich reviewed Oct 29, 2020

View reviewed changes

brminich approved these changes Oct 29, 2020

View reviewed changes

hoopoepg approved these changes Oct 29, 2020

View reviewed changes

yosefe reviewed Nov 2, 2020

View reviewed changes

yosefe approved these changes Nov 5, 2020

View reviewed changes

UCP/CORE: Reduce size of generic EP extension for further usage

23b62ba

dmitrygx force-pushed the topic/ucp/ep_ext_gen_reduce branch from 3e8d837 to 23b62ba Compare November 5, 2020 11:04

yosefe merged commit d669d54 into openucx:master Nov 5, 2020

UCP/CORE: Reduce size of generic EP extension for further usage #5848

UCP/CORE: Reduce size of generic EP extension for further usage #5848

Conversation

dmitrygx commented Oct 28, 2020

What

Why ?

How ?

dmitrygx commented Oct 28, 2020

azure-pipelines bot commented Oct 28, 2020

dmitrygx commented Oct 29, 2020

dmitrygx commented Oct 29, 2020

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

dmitrygx commented Oct 29, 2020

dmitrygx commented Oct 29, 2020

dmitrygx commented Oct 29, 2020

azure-pipelines bot commented Oct 29, 2020

dmitrygx commented Oct 30, 2020

azure-pipelines bot commented Oct 30, 2020

brminich commented Oct 31, 2020

azure-pipelines bot commented Oct 31, 2020

dmitrygx commented Nov 2, 2020

dmitrygx commented Nov 2, 2020

azure-pipelines bot commented Nov 2, 2020

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

dmitrygx commented Nov 4, 2020

dmitrygx commented Nov 5, 2020 • edited Loading

dmitrygx commented Nov 5, 2020

azure-pipelines bot commented Nov 5, 2020

dmitrygx commented Nov 5, 2020

yosefe commented Nov 5, 2020

yosefe commented Nov 5, 2020

azure-pipelines bot commented Nov 5, 2020

dmitrygx commented Nov 5, 2020

dmitrygx commented Nov 5, 2020

dmitrygx commented Nov 5, 2020 •

edited

Loading