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/PROTO: Simplify RNDV_SCHEME env logic #9815
base: master
Are you sure you want to change the base?
UCP/PROTO: Simplify RNDV_SCHEME env logic #9815
Conversation
test/gtest/ucp/test_ucp_tag_xfer.cc
Outdated
UCS_TEST_P(test_ucp_tag_xfer, send_contig_recv_generic_unexp_sync_rndv, | ||
"RNDV_THRESH=1000", "ZCOPY_THRESH=1248576") { | ||
UCP_TEST_TAG_RNDV_GENERIC(test_ucp_tag_xfer, send_contig_recv_generic_unexp_sync_rndv, | ||
"ZCOPY_THRESH=1248576") { |
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: aligment
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.
test/gtest/ucp/ucp_test.h
Outdated
@@ -333,6 +333,9 @@ class ucp_test : public ucp_test_base, | |||
// Return context parameters of the current test variant | |||
const ucp_params_t& get_variant_ctx_params() const; | |||
|
|||
// Check whether sender and receiver supports certain caps | |||
bool supports_caps(uint64_t rndv_modes) const; |
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 name it support_caps
or is_support_caps
?
It's unusual to have s
suffix for verbs in function names. E.g. free_memory
, not frees_memory
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.
@@ -30,7 +30,7 @@ ucp_proto_rndv_get_common_init(const ucp_proto_init_params_t *init_params, | |||
ucp_context_t *context = init_params->worker->context; | |||
ucp_proto_multi_init_params_t params = { | |||
.super.super = *init_params, | |||
.super.cfg_thresh = ucp_proto_rndv_cfg_thresh(context, rndv_modes), | |||
.super.cfg_thresh = UCS_MEMUNITS_AUTO, |
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.
Q: with older implementation, the resulting threshold could be 0 or AUTO, while with the new one it is always AUTO.
Is this change on purpose? May it introduce regressions?
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 change is why this patch was created. In master branch if RNDV_SHCEME is set to certain RNDV mode, and this protocol is unavailable, UCX falls back to other RNDV.
It was decide to remove this fall back logic and just don't initialize RNDV modes which are not set in RNDV_SCHEME.
What
Fail protocol initialization if
RNDV_SCHEME
doesn't contain it's RNDV mode.Why
This simplifies proto initialization logic which significantly affects logic for #9728