-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UCP/EP: Cleanup proto reqs on failure #5821
UCP/EP: Cleanup proto reqs on failure #5821
Conversation
5a5a837
to
6565d36
Compare
1875428
to
f09147b
Compare
69d47f5
to
592b80a
Compare
eee98d7
to
91014aa
Compare
@brminich @hoopoepg @evgeny-leksikov could you review pls? |
src/ucp/core/ucp_ep.c
Outdated
/* it means that purging started from a request responsible | ||
* for sending RTR, so a request responsible for copying | ||
* data from staging buffer is a receive request */ | ||
req->super_req->recv.remaining -= req->recv.length; | ||
} else { | ||
/* it means that purging started from a request responsible | ||
* for sending RTR, so a request responsible for copying | ||
* data from staging buffer is a send request */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls unite to single comment just pointing to difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ucp/core/ucp_request.inl
Outdated
@@ -50,17 +50,17 @@ | |||
|
|||
|
|||
/* defined as a macro to print the call site */ | |||
#define ucp_request_get(_worker) \ | |||
({ \ | |||
static inline ucp_request_t* ucp_request_get(ucp_worker_h _worker) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USC_F_ALWAYS_INLINE
- pls remove ending
\
s, no need in func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and move function below defines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also func names do not start with _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is leftover from debugging
rolled back to the previous state
src/ucp/rma/flush.c
Outdated
ucs_hlist_add_tail(&flush_state->reqs, | ||
&req->list_elem); | ||
ucs_trace_req("added flush request %p to ep remote completion " | ||
" queueu with sn %d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo queueu and extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ucp/rndv/rndv.c
Outdated
ucp_rndv_rtr_pack, | ||
sizeof(ucp_rndv_rtr_hdr_t) + | ||
packed_rkey_size); | ||
if ((status != UCS_OK) && (status != UCS_ERR_NO_RESOURCE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ucs_unlikely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ucp/rndv/rndv.c
Outdated
@@ -821,6 +802,10 @@ ucp_rndv_recv_frag_put_mem_type(ucp_request_t *rreq, ucp_request_t *rndv_req, | |||
ucp_ep_use_indirect_id(freq->send.ep)); | |||
} | |||
|
|||
ucs_assert(freq->send.ep != NULL); | |||
ucs_hlist_add_tail(&ucp_ep_ext_gen(freq->send.ep)->proto_reqs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to save fragments on the list for put? should not transport report failure? otherwise the request should stay on pending, rigth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is PUT operation from HOST staging buffer to GPU device buffer after receiving ATP from a peer on each fragment or after successful GET operation in pipelined RNDV
yes, we don't need this, since it is a local operation
removed
test/gtest/common/test_obj_size.cc
Outdated
@@ -48,7 +48,7 @@ UCS_TEST_F(test_obj_size, size) { | |||
#else | |||
EXPECTED_SIZE(ucp_ep_t, 64); | |||
/* TODO reduce request size to 240 or less after removing old protocols state */ | |||
EXPECTED_SIZE(ucp_request_t, 296); | |||
EXPECTED_SIZE(ucp_request_t, 312); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no way to avoid this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did two PRs to decrease UCP request size from 320 bytes to 296 bytes: #5829 and #5825
I have the privilege to use at least 16 bytes :)
unfortunately, no way to reduce this - I tried to refactor UCP request send
part, but it has ucp_wireup_msg_t
in the union that could be reduced and it is our limit...
src/ucp/core/ucp_request.inl
Outdated
@@ -50,17 +50,17 @@ | |||
|
|||
|
|||
/* defined as a macro to print the call site */ | |||
#define ucp_request_get(_worker) \ | |||
({ \ | |||
static inline ucp_request_t* ucp_request_get(ucp_worker_h _worker) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and move function below defines
src/ucp/core/ucp_worker.c
Outdated
#include "ucp_worker.h" | ||
#include "ucp_am.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by mistake
src/ucp/rma/flush.c
Outdated
ucs_trace_req("added flush request %p to ep remote completion " | ||
" queueu with sn %d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are 2 spaces in target string completion++queueU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some tests to make sure the functionality is covered?
src/ucp/core/ucp_ep.c
Outdated
static void ucp_ep_req_purge(ucp_request_t *req, ucs_status_t status, | ||
int recursive) | ||
{ | ||
if (req->flags & (UCP_REQUEST_FLAG_SEND_AM | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: seems to fit to 80 syms
(same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
but it doesn't fit 80 syms below
src/ucp/rndv/rndv.c
Outdated
/* don't release rndv request in case of success, since it was sent to | ||
* a peer as a remote request ID, and we will use the req to track an | ||
* user-exposed receive request (and request for copying data from staging | ||
* buffer in case of fragmented RNDV) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: maybe use pipelined
instead of fragmented
to avoid confusion with multirail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ucp/rndv/rndv.c
Outdated
@@ -1297,6 +1282,16 @@ UCS_PROFILE_FUNC(ucs_status_t, ucp_rndv_rts_handler, | |||
} | |||
} | |||
|
|||
static void ucp_rndv_ats_complete(ucp_request_t *sreq, ucs_status_t status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for separate func, which is called just once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ucp/core/ucp_request.inl
Outdated
@@ -50,17 +50,17 @@ | |||
|
|||
|
|||
/* defined as a macro to print the call site */ | |||
#define ucp_request_get(_worker) \ | |||
({ \ | |||
static inline ucp_request_t* ucp_request_get(ucp_worker_h _worker) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also func names do not start with _
bot:pipe:retest |
@evgeny-leksikov @hoopoepg @brminich is it ok now? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
failure is #5840 |
c5c92c5
to
c26fca8
Compare
6eeb26c
to
4488275
Compare
if (req->id != UCP_REQUEST_ID_INVALID) { | ||
ucp_request_id_release(req); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible some of the protocols cleanup flow would try to release the request it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, Yossi
currently, we don't have such protocols, but they could be implemented in the future
fixed this comment by moving checking/releasing of request ID at the end of the function, but need to implement some hack (save and remove RELEASE flag before calling request complete procedure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "extract" the id from the request to a local variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then protocols which expect that request ID is valid will fail ucp_request_id_release()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decided to not do it for now
src/ucp/core/ucp_ep.c
Outdated
if ((wireup_ep != NULL) && ucp_wireup_ep_test(wireup_ep)) { | ||
/* flush state is not valid yet */ | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we check some ep flags instead? a more "direct" check if flush state is valid or not?
src/ucp/core/ucp_request.inl
Outdated
@@ -839,8 +847,11 @@ ucp_request_id_release(ucp_request_t *req) | |||
ucp_request_id_reset(req); | |||
|
|||
ucs_assert(req->send.ep != NULL); | |||
ucs_hlist_del(&ucp_ep_ext_gen(req->send.ep)->proto_reqs, | |||
&req->send.list_elem); | |||
if (ucs_unlikely(ucp_ep_config(req->send.ep)->key.err_mode == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we unite it with the "if" inside ucs_ptr_map_del() which checks for direct/indirect id? to avoid extra branch on fast path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but user can set UCX_PROTO_INDIRECT_ID=n
in case of error handling too and vice versa (UCX_PROTO_INDIRECT_ID=y
in case of non-error handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i see the problem, need to discuss this offline
for now, i would add comment ion the code that we should optimize this
BTW it happens only for eager/sync, rndv, and sw-rma flow, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now, i would add comment ion the code that we should optimize this
added the comment
BTW it happens only for eager/sync, rndv, and sw-rma flow, right?
yes, it is right
src/ucp/core/ucp_ep.c
Outdated
/* Mark release to be not released from the ucp_request_complete() procedure | ||
* to be able do some actions with this request (e.g. check and release | ||
* request ID) after completing an operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's better to release the request id for each protocol explicitly. after we find which protocol is it according to req flags, we can decide whether need to release the id or not. and potentially unite code with normal-flow completion and id release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean to release ID from protocol completion callback?
but it is not possible sometimes, because some of them, e.g. RNDV PUT rely on the fact that request ID will be released upon receiving RTR packet, since ID is no longer needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean from the if/else/else ... code below
is it possible some protocols release the id inside the completion callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible some protocols release the id inside the completion callback?
no, currently we don't have such protocols.
but the existing protocols (or new ones) can be updated to release the ID inside its completion callback
error seems relevant:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error seems relevant:
yes, need to fix flush_state
src/ucp/core/ucp_ep.c
Outdated
if ((wireup_ep != NULL) && ucp_wireup_ep_test(wireup_ep)) { | ||
/* flush state is not valid yet */ | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed - I found that it isn't needed anymore
src/ucp/rndv/rndv.c
Outdated
@@ -846,7 +851,7 @@ UCS_PROFILE_FUNC_VOID(ucp_rndv_recv_frag_put_completion, (self), | |||
rreq->recv.remaining, freq->send.length); | |||
rreq->recv.remaining -= freq->send.length; | |||
if (rreq->recv.remaining == 0) { | |||
ucp_rndv_recv_req_complete(rreq, UCS_OK); | |||
ucp_request_complete_and_dereg_recv_rndv(rreq, UCS_OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ucp/core/ucp_request.inl
Outdated
@@ -839,8 +847,11 @@ ucp_request_id_release(ucp_request_t *req) | |||
ucp_request_id_reset(req); | |||
|
|||
ucs_assert(req->send.ep != NULL); | |||
ucs_hlist_del(&ucp_ep_ext_gen(req->send.ep)->proto_reqs, | |||
&req->send.list_elem); | |||
if (ucs_unlikely(ucp_ep_config(req->send.ep)->key.err_mode == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now, i would add comment ion the code that we should optimize this
added the comment
BTW it happens only for eager/sync, rndv, and sw-rma flow, right?
yes, it is right
src/ucp/core/ucp_ep.c
Outdated
/* Mark release to be not released from the ucp_request_complete() procedure | ||
* to be able do some actions with this request (e.g. check and release | ||
* request ID) after completing an operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible some protocols release the id inside the completion callback?
no, currently we don't have such protocols.
but the existing protocols (or new ones) can be updated to release the ID inside its completion callback
if (req->id != UCP_REQUEST_ID_INVALID) { | ||
ucp_request_id_release(req); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decided to not do it for now
src/ucp/wireup/ep_match.c
Outdated
* flush state won't be used (flush state is considered as valid, when | ||
* EP doesn't exist on matchong context and remoe EP ID is set) */ | ||
ucp_ep_update_flags(ep, 0, | ||
UCP_EP_FLAG_ON_MATCH_CTX | UCP_EP_FLAG_REMOTE_ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we set remote id here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we release the flag here instead to not use flush state
src/ucp/wireup/wireup.c
Outdated
} else { | ||
ucp_ep_update_remote_id(ep, msg->src_ep_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now, returned back - but changed other places
we rely on the fact that if REMOTE_ID is set then FLUSH state is valid
so, changed it to check FLUSH_STATE/ON_MACTCH_CTX flags when updating remote ID
src/ucp/core/ucp_ep.inl
Outdated
@@ -232,6 +232,7 @@ static inline void ucp_ep_flush_state_reset(ucp_ep_h ep) | |||
((flush_state->send_sn == 0) && | |||
(flush_state->cmpl_sn == 0) && | |||
ucs_hlist_is_empty(&flush_state->reqs))); | |||
ucs_assert(ep->flags & UCP_EP_FLAG_REMOTE_ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need this assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make sure the we reset flush state after updating a remote ID
so, we can rely on the flag
I think better to make updating remote ID dependant on flush state or matching context flags - done it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think better not introduce such dependency in this PR, since it can require more substantial refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think better not introduce such dependency in this PR, since it can require more substantial refactoring
I have already done it :)
just need to change the sequence
failure is #6568 |
59d2e6a
to
dc909eb
Compare
dc909eb
to
d8b364b
Compare
What
Clean up proto reqs on failure
Why ?
To fix UCP request leak when they are not UCT-managed (i.e. not send in-progress or not in pending)
How ?
Use UCP EP extension to save submitted UCP requests (only TAG sync send and TAG/AM RNDV) to hlist
Purge them from hlist when UCP EP failed