UCT/UCP: Fixes for am_zcopy sends #125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@brminich @evgeny-leksikov can you pls take a look? it fixes am_zcopy flow which is apparently not tested because ZCOPY_THRESH==RNDV_THRESH in mtt |
brminich
reviewed
Jan 19, 2021
src/uct/ib/rc/accel/rc_mlx5_ep.c
Outdated
| @@ -221,6 +220,7 @@ ucs_status_t uct_rc_mlx5_ep_put_zcopy(uct_ep_h tl_ep, const uct_iov_t *iov, size | |||
| UCT_CHECK_LENGTH(uct_iov_total_length(iov, iovcnt), 0, UCT_IB_MAX_MESSAGE_SIZE, | |||
| "put_zcopy"); | |||
| UCT_RC_CHECK_NUM_RDMA_READ(iface); | |||
| UCT_RC_CHECK_RES(iface, &ep->super); | |||
There was a problem hiding this comment.
should it be done before UCT_RC_CHECK_NUM_RDMA_READ? Or how does it help?
brminich
reviewed
Jan 19, 2021
src/uct/ib/rc/accel/rc_mlx5_ep.c
Outdated
| @@ -269,6 +269,7 @@ ucs_status_t uct_rc_mlx5_ep_get_zcopy(uct_ep_h tl_ep, const uct_iov_t *iov, size | |||
| iface->super.super.config.max_inl_resp + 1, | |||
| iface->super.config.max_get_zcopy, "get_zcopy"); | |||
| UCT_RC_CHECK_NUM_RDMA_READ(&iface->super); | |||
| UCT_RC_CHECK_RES(&iface->super, &ep->super); | |||
There was a problem hiding this comment.
should it be done before UCT_RC_CHECK_NUM_RDMA_READ? Or how does it help?
dmitrygx
reviewed
Jan 19, 2021
src/uct/ib/rc/accel/rc_mlx5_ep.c
Outdated
Comment on lines
271
to
272
| UCT_RC_CHECK_NUM_RDMA_READ(&iface->super); | ||
| UCT_RC_CHECK_RES(&iface->super, &ep->super); |
There was a problem hiding this comment.
UCT_RC_CHECK_RMA_RES instead of both macros?
src/uct/ib/rc/accel/rc_mlx5_ep.c
Outdated
Comment on lines
222
to
223
| UCT_RC_CHECK_NUM_RDMA_READ(iface); | ||
| UCT_RC_CHECK_RES(iface, &ep->super); |
There was a problem hiding this comment.
UCT_RC_CHECK_RMA_RES instead of both macros?
src/ucp/proto/proto_am.inl
Outdated
| @@ -391,6 +391,8 @@ ucs_status_t ucp_do_am_zcopy_multi(uct_pending_req_t *self, uint8_t am_id_first, | |||
| } | |||
| ucs_assert(status == UCS_INPROGRESS); | |||
| return UCS_OK; | |||
| } else { | |||
There was a problem hiding this comment.
no need for else
i.e. can fix it in the same way as in master:
https://github.com/openucx/ucx/blob/8d3801b94512e91c508a7d2a357b58bb9e5d4b6c/src/ucp/proto/proto_am.inl#L444
1. ucp_do_am_zcopy_multi() did not return UCS_ERR_NO_RESOURCE when such status was returned from uct_ep_am_zcopy, as a result the send request was removed from pending queue and did not progress (io_demo test stuck) 2. rc_mlx5 am_zcopy checked pending queue assertion before checking send resources in uct_rc_mlx5_ep_zcopy_post() which leads to wrong assertion. Move resource checking from zcopy_post to calling functions. 3. Invalid send flags passed in zcopy_post: SOLICITED was not set when comp != NULL.
yosefe
force-pushed
the
topic/mlx5-send-fixes
branch
from
20aacb7
to
7df1246
Compare
dmitrygx
approved these changes
Jan 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
status was returned from uct_ep_am_zcopy, as a result the send
request was removed from pending queue and did not progress (io_demo
test stuck)
send resources in uct_rc_mlx5_ep_zcopy_post() which leads to wrong
assertion. Move resource checking from zcopy_post to calling
functions.
comp != NULL.