Repository navigation
Conversation
select.c compared dst_version of the remote address with the raw release numbers 17, 18 and 19. Define them as UCP_WIREUP_*_MIN_DST_VERSION constants in wireup.h, next to UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION, so that the peer release checks of the wireup are in one place. The values are not changed.
A change of the lanes picked by select.c must keep the old selection with peers of older releases, otherwise a new and an old peer pick different lanes. Document that such a change is gated by a *_MIN_DST_VERSION constant in wireup.h.
|
🤖 Starting review — findings will be posted here when done. |
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.
The peer release is checked not only by the lane selection, but also by the remote key unpack, the endpoint recovery and the protocols. Move the rule from wireup/AGENTS.md to src/ucp/AGENTS.md and make it cover any code that depends on what the peer release supports.
|
🤖 Starting review — findings will be posted here when done. |
| checked against `dst_version` of the peer, never by a raw release | ||
| number. Define the constant with a comment in the module that checks | ||
| it, e.g. `UCP_WIREUP_MAX_LANES_MIN_DST_VERSION` in `wireup/wireup.h` or | ||
| `UCP_AM_PSN_MIN_DST_VERSION` in `am/eager_multi.c`. |
There was a problem hiding this comment.
src/ucp/wireup/address.c:1642 (not in the diff, surfaced by this new rule): ucp_address_adjust_unpacked_md_index() still gates peer-dependent unpacking on a raw release number via a local const unsigned max_legacy_api_version = 18u;. The PR documents "never by a raw release number", so this is the one leftover that contradicts the new rule. Can we convert it to a *_MIN_DST_VERSION constant as well, so the new rule holds everywhere?
There was a problem hiding this comment.
Done, UCP_ADDRESS_MD_INDEX_MIN_DST_VERSION.
|
|
||
| /* Minimal peer release version which supports the endpoint recovery */ | ||
| #define UCP_EP_RECOVERY_MIN_DST_VERSION 22 | ||
|
|
There was a problem hiding this comment.
minor: the new define is separated by a single blank line from __KHASH_IMPL, while the two other defines added in this PR (am/eager_multi.c, core/ucp_rkey.c) use two blank lines. Pls add one more blank line before __KHASH_IMPL to match.
There was a problem hiding this comment.
No define is left in ucp_ep.c: the constants moved to one enum in ucp_types.h.
| #include <string.h> | ||
|
|
||
| /* Minimal peer release version which supports the endpoint recovery */ | ||
| #define UCP_EP_RECOVERY_MIN_DST_VERSION 22 |
There was a problem hiding this comment.
maybe move the list of constants to single place/enum ?
There was a problem hiding this comment.
Done: all the *_MIN_DST_VERSION constants are now one enum in ucp_types.h, ordered by version.
Keep all the *_MIN_DST_VERSION constants, including the existing UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION and UCP_PROTO_RMA_RNDV_MIN_DST_VERSION, in one enum in ucp_types.h, ordered by version, instead of next to their users. The AGENTS.md rule points to this list.
ucp_address_adjust_unpacked_md_index() compared dst_version with a local constant 18, the last release which packs legacy bits in the MD index field of a v1 address. Use UCP_ADDRESS_MD_INDEX_MIN_DST_VERSION (19) instead: "> 18" becomes ">= 19", dst_version is unsigned.
aadd74a to
628c4ad
Compare
|
🤖 Starting review — findings will be posted here when done. |
| @@ -60,6 +60,12 @@ applications. Almost every meaningful UCX feature lives here. | |||
| - For data sent during wireup (worker addresses, capability bits, etc.), | |||
There was a problem hiding this comment.
minor: can we also update wireup/AGENTS.md ("A payload section appended to an already released message must be gated by a *_MIN_DST_VERSION constant ...") to say the constant goes to the list in core/ucp_types.h? Now that UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION moved out of wireup.h, the wireup guide still describes *_MIN_DST_VERSION gating locally without saying where the constant belongs — otherwise the next wire-compat constant may again be defined locally in wireup.h.
There was a problem hiding this comment.
Done: the wireup guide now takes the constant from core/ucp_types.h.
| @@ -349,6 +349,36 @@ typedef enum { | |||
| } ucp_object_version_t; | |||
There was a problem hiding this comment.
Residual note: two version gates stay outside the new list by design (select.c:1527, select.c:2302, dst_version > UCP_RELEASE_LEGACY). If the intent is that all peer-release gating is discoverable from ucp_types.h, it may be worth a one-line comment here. Not raised as a blocking comment since behavior is unchanged and the constant is already symbolic.
There was a problem hiding this comment.
Added a sentence to the comment of the list: a peer which does not pack its release version has dst_version UCP_RELEASE_LEGACY, below all of the listed versions. The two > UCP_RELEASE_LEGACY checks stay as they are: they check whether the peer packs its release version at all (UCX 1.16 and later), and the constant is already named.
The wireup guide asks to gate a payload section appended to a released message by a *_MIN_DST_VERSION constant. Say that the constant comes from core/ucp_types.h, which now has UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION too, so that a new one is not defined in wireup.h again.
A peer which does not pack its release version has dst_version UCP_RELEASE_LEGACY. Say so in the comment of the *_MIN_DST_VERSION list: such a peer is below all of the versions there, and select.c checks dst_version > UCP_RELEASE_LEGACY to know whether the peer packs its release version at all.
|
🤖 Starting review — findings will be posted here when done. |
| * A feature is used with a peer only if the release version of the peer | ||
| * (dst_version) is at least the given one, so peers of older releases keep | ||
| * working. A peer which does not pack its release version has dst_version | ||
| * UCP_RELEASE_LEGACY, below all of these versions. |
There was a problem hiding this comment.
minor: UCP_RELEASE_LEGACY, which this comment refers to, is still defined in wireup/wireup.h — can we move it here too, so all peer-version constants live in one place?
There was a problem hiding this comment.
Done: UCP_RELEASE_LEGACY is now defined in core/ucp_types.h, right before the list.
UCP_RELEASE_LEGACY is the dst_version of a peer which does not pack its release version, and the comment of the *_MIN_DST_VERSION list refers to it. Define it in core/ucp_types.h next to the list instead of wireup/wireup.h, so all the peer version constants are in one place. No functional change.
|
🤖 Starting review — findings will be posted here when done. |
| } ucp_object_version_t; | ||
|
|
||
|
|
||
| /* The dst_version of a peer which does not pack its release version */ |
There was a problem hiding this comment.
minor: maybe make this the first member of the enum below (UCP_RELEASE_LEGACY = 0), so all peer-version constants are defined in one list?
There was a problem hiding this comment.
Done: UCP_RELEASE_LEGACY = 0 is now the first member of the list.
| UCP_PROTO_RMA_RNDV_MIN_DST_VERSION = 22, | ||
| /* The token trailer of the LANES_ADDR messages */ | ||
| UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION = 24 | ||
| }; |
There was a problem hiding this comment.
can we add UCS_STATIC_ASSERT(UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION <= UCP_API_MINOR) here? Since the list is ordered, asserting the last one would catch a typo that silently disables a feature even between same-release peers.
There was a problem hiding this comment.
UCS_STATIC_ASSERT is a switch statement, so it can only go inside a function, not next to the list, and asserting the last constant would have to be moved with every new one. I'd keep the list as it is.
| 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.
| ucs_diag("ep: %p: recovery support requires UCX 1.%d or later, " | |
| ucs_diag("ep: %p: recovery support requires UCX 1.%d or later, " | |
| "remote peer version %u is not supported", | |
| ep, UCP_EP_RECOVERY_MIN_DST_VERSION, | |
| ucp_ep_config(ep)->key.dst_version); |
minor: dst_version is unsigned.
Keep all the peer version constants in one enum: UCP_RELEASE_LEGACY, the dst_version of a peer which does not pack its release version, becomes its first member instead of a define next to it. The list is ordered by version, so the comment no longer has to say that UCP_RELEASE_LEGACY is below all the other versions. No functional change.
dst_version is unsigned.
|
🤖 Starting review — findings will be posted here when done. |
|
@evgeny-leksikov @guy-ealey-morag could you review pls? I guess now your comments fully addressed. |
| * used with a peer only if dst_version of the peer is at least the given one, | ||
| * so peers of older releases keep working. | ||
| */ | ||
| enum { |
There was a problem hiding this comment.
minor: as an enum I would expect the values to have the same prefix like UCP_MIN_DST_VERSION_*
It's not super important so your call
There was a problem hiding this comment.
Done: all of them are UCP_MIN_DST_VERSION_* now, UCP_RELEASE_LEGACY became UCP_MIN_DST_VERSION_LEGACY.
The constants are members of one enum, so give them a common prefix: UCP_<FEATURE>_MIN_DST_VERSION becomes UCP_MIN_DST_VERSION_<FEATURE>, and UCP_RELEASE_LEGACY becomes UCP_MIN_DST_VERSION_LEGACY. No functional change.
The code style check aligns the values of consecutive enum members, also across the comments between them.
|
🤖 Starting review — findings will be posted here when done. |
| */ | ||
| enum { | ||
| /* The dst_version of a peer which does not pack its release version */ | ||
| UCP_MIN_DST_VERSION_LEGACY = 0, |
There was a problem hiding this comment.
IMO this entry is not a feature minimum - both users check dst_version > UCP_MIN_DST_VERSION_LEGACY - so UCP_DST_VERSION_LEGACY would read better and match the comment above.
There was a problem hiding this comment.
I'd keep it: the reviewers asked for one prefix for all the values of this enum, and as its lowest value UCP_MIN_DST_VERSION_LEGACY is the minimal dst_version, that of a peer which does not pack its release version.
| */ | ||
| enum { | ||
| /* The dst_version of a peer which does not pack its release version */ | ||
| UCP_MIN_DST_VERSION_LEGACY = 0, |
There was a problem hiding this comment.
minor: ucp_address_unpack_header() still does *dst_version = 0 for a peer that does not pack its release version, can we use this constant there too?
ucp_address_unpack_header() sets dst_version of a v1 address to 0 until the release version is found after the devices, as for a peer which does not pack it. Use the named constant. No functional change.
The release version is packed in 4 bits, and ucp_address_unpack_release_version() takes the value closest to our release, so a peer more than 7 releases older can get a wrong, newer dst_version (with release 24, a 1.16 peer is seen as 32). Say so in the comment of the list.
|
🤖 Starting review — findings will be posted here when done. |
UCP_MIN_DST_VERSION_LEGACY is not the minimal peer version of a feature: it is the dst_version of a peer which does not pack its release version, and select.c only checks that dst_version is above it. Define it as UCP_VERSION_LEGACY next to the enum instead. No functional change.
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
|
|
||
| /* The dst_version of a peer which does not pack its release version */ | ||
| #define UCP_VERSION_LEGACY 0 |
There was a problem hiding this comment.
minor: can we call it UCP_DST_VERSION_LEGACY and move it into the enum below? Its own comment says it is a dst_version value, so it would be good to keep all of them in one place with the same prefix. As it stands, the renamed legacy constant is the only dst_version constant that neither carries the DST_VERSION prefix nor lives in the new enum, so the grouping this PR introduces is slightly inconsistent.
|
🤖 CI Triage Agent — TL;DR: The ASAN/BlueField gtest shard 2/4 hung for ~11 minutes in Full analysisSummary: Root cause: Log timeline shows a hang, not slowness:
The backtrace pins the stuck request precisely: Prime suspect in that path: Nothing in the PR under test touches RMA/flush: commit Implicated commit: not the PR commit File: Suggested fix:
Related: PR #12233 (build under test, unrelated), #11923, #11947, #11888
|
What?
Name the peer release versions that UCP compares
dst_versionwith. One commented enum ofUCP_MIN_DST_VERSION_*constants incore/ucp_types.hreplaces the raw numbers and the scattered defines, and the UCP agent guides ask to gate such checks by these constants. No functional change.Why?
A raw release number does not say which peer capability it gates, and a new check can easily break compatibility with older peers. This came up in the review of #12206.
How?
The constants keep the old values: checks like
> 19become>= 20(dst_versionis unsigned). The enum also takes the existing constants fromwireup.handrma_rndv.c, renamed to the same prefix, andUCP_RELEASE_LEGACYmoves next to it asUCP_VERSION_LEGACY. Includes the changes of the closed #12234.