Repository navigation
Conversation
ucp_rkey.c, ucp_ep.c and am/eager_multi.c compared dst_version of the peer with raw release numbers. Define them as local *_MIN_DST_VERSION constants, like UCP_PROTO_RMA_RNDV_MIN_DST_VERSION in rma_rndv.c: - UCP_RKEY_SYS_DEV_MIN_DST_VERSION (20): the peer always packs the system device of a non-empty remote key. "> 19" becomes ">= 20", dst_version is unsigned. - UCP_EP_RECOVERY_MIN_DST_VERSION (22): the peer supports the endpoint recovery. The diagnostic message prints it instead of a literal 1.22. - UCP_AM_PSN_MIN_DST_VERSION (21): the peer supports the PSN protocol. No functional change.
|
🤖 Starting review — findings will be posted here when done. |
| if (ucp_ep_config(ep)->key.dst_version < 22) { | ||
| ucs_diag("ep: %p: recovery support requires UCX 1.22 or later, " | ||
| if (ucp_ep_config(ep)->key.dst_version < UCP_EP_RECOVERY_MIN_DST_VERSION) { | ||
| ucs_diag("ep: %p: recovery support requires UCX 1.%d or later, " |
There was a problem hiding this comment.
minor: while touching this message, pls print the peer version with %u — key.dst_version is declared unsigned (ucp_ep.h:283), but the reflowed message still prints it with %d. Impact: cosmetic/format-type mismatch only.
|
soft suggestion — PR scope: the same kind of raw version gate remains in |
This was referenced Nov 4, 2026
This file contains hidden or 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
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.
What?
Replace the raw peer release numbers compared with
dst_versioninucp_rkey.c,ucp_ep.candam/eager_multi.cwith named constants. No functional change.ucp_rkey.c:UCP_RKEY_SYS_DEV_MIN_DST_VERSION(20), the peer always packs the system device of a non-empty remote key, and the lane distances if the device is known. Used inucp_rkey_extract_sys_dev()anducp_rkey_proto_resolve().dst_version > 19becomesdst_version >= UCP_RKEY_SYS_DEV_MIN_DST_VERSION, which is the same for the unsigneddst_version.ucp_ep.c:UCP_EP_RECOVERY_MIN_DST_VERSION(22), the peer supports the endpoint recovery. Used inucp_ep_recovery_arm(), whose diagnostic message prints the constant instead of a literal1.22, so the text stays the same.am/eager_multi.c:UCP_AM_PSN_MIN_DST_VERSION(21), the peer supports the PSN protocol. Used inucp_am_eager_multi_zcopy_psn_proto_probe().The constants are local to their only users, like
UCP_PROTO_RMA_RNDV_MIN_DST_VERSIONinrma_rndv.c.Why?
A raw release number does not say which peer capability it checks. A named constant documents it in one place, so the check is easy to find and keep consistent with the code it protects. The lane selection checks in
select.care named in #12233; after both, nodst_versioncheck insrc/ucpcompares with a raw number.How?
The values are not changed: with the constants replaced by their values (
>= 20as> 19, and1.%dwith 22 as1.22), each file is the same as on master token by token.grep -rnE 'dst_version *(<|>|<=|>=|==|!=) *[0-9]' src/prints only theselect.cchecks handled by #12233.