UCP/PROTO: Handle request status correctly and refactor #10473
Conversation
src/ucp/am/eager_multi.c
Outdated
| @@ -134,8 +134,8 @@ static UCS_F_ALWAYS_INLINE ucs_status_t ucp_am_eager_multi_bcopy_send_func( | |||
| packed_size = uct_ep_am_bcopy(uct_ep, UCP_AM_ID_AM_FIRST, | |||
| ucp_am_eager_multi_bcopy_pack_args_first, | |||
| &pack_ctx, 0); | |||
| status = ucp_proto_bcopy_send_func_status(packed_size); | |||
| status = ucp_proto_am_handle_user_header_send_status(req, status); | |||
| status = ucp_am_handle_user_header_send_status_common( | |||
There was a problem hiding this comment.
Is it possible to reproduce the original failure with unit test?
If so it would be helpful to add it
There was a problem hiding this comment.
I think it's a good idea but not for this PR because for now it's not practical.
To do this we need to add some hint to UCX mpool object that will cause allocation to fail, I don't think this is something this PR suppose to do but we can consider it for the future to test this case and other use cases that involve memory allocation.
src/ucp/proto/proto_am.inl
Outdated
| ucp_am_handle_user_header_send_status_common(ucp_request_t *req, | ||
| ucs_status_t status, int abort, | ||
| int release) |
There was a problem hiding this comment.
#10452 (comment) reference to initial conversation
ofirfarjun7
force-pushed
the
topic/fix-copy-header-status-handling
branch
from
2ef4c2c to
506195b
Compare
| @@ -136,7 +136,7 @@ static UCS_F_ALWAYS_INLINE ucs_status_t ucp_am_eager_multi_bcopy_send_func( | |||
| ucp_am_eager_multi_bcopy_pack_args_first, | |||
| &pack_ctx, 0); | |||
| status = ucp_proto_bcopy_send_func_status(packed_size); | |||
| status = ucp_proto_am_handle_user_header_send_status(req, status); | |||
| status = ucp_am_handle_user_header_send_status_or_release(req, status); | |||
There was a problem hiding this comment.
This is after formatting with clang...
I can force alignment.
There was a problem hiding this comment.
The = are clearly not aligned. Why clang suggests this?
Anyway, better force IMO
There was a problem hiding this comment.
Why? Code style - "Up to 80 columns"
|
@ofirfarjun7 please squash |
ofirfarjun7
force-pushed
the
topic/fix-copy-header-status-handling
branch
from
506195b to
e379499
Compare
ofirfarjun7
force-pushed
the
topic/fix-copy-header-status-handling
branch
from
e379499 to
a16c4dd
Compare
What?
Why?
In some flows of send operations we can reach
ucs_fatalbecause status is not handled properly.How?
Instead of returning unexpected status code we should abort operation and return
UCS_OK