Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/ucp/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ applications. Almost every meaningful UCX feature lives here.
- For data sent during wireup (worker addresses, capability bits, etc.),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: the wireup guide now takes the constant from core/ucp_types.h.

the pack/unpack lives in `wireup/address.c` — anything that needs to
cross the wire at handshake time must round-trip through it.
- Code that depends on what the peer release supports (wire formats, lane
selection, protocols, endpoint features) must keep the old behavior
with peers of older releases: gate it by a `*_MIN_DST_VERSION` constant
checked against `dst_version` of the peer, never by a raw release
number. Add the constant with a comment to the list of such constants
in `core/ucp_types.h`.
- Iterate TL bitmaps with `UCP_TL_BITMAP_FOR_EACH_BIT`, not
`UCS_STATIC_BITMAP_FOR_EACH_BIT`, and use `ucp_tl_bitmap_ffs` over
`UCS_STATIC_BITMAP_FFS`.
Expand Down
2 changes: 1 addition & 1 deletion src/ucp/am/eager_multi.c
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ static void ucp_am_eager_multi_zcopy_psn_proto_probe(
{
/* PSN protocol requires both sides to support it - disable if peer
* version is too old */
if (init_params->ep_config_key->dst_version < 21) {
if (init_params->ep_config_key->dst_version < UCP_AM_PSN_MIN_DST_VERSION) {
return;
}

Expand Down
7 changes: 4 additions & 3 deletions src/ucp/core/ucp_ep.c
Original file line number Diff line number Diff line change
Expand Up @@ -2417,10 +2417,11 @@ ucs_status_t ucp_ep_recovery_arm(ucp_ep_h ep)

ucs_assert(!(ep->flags & UCP_EP_FLAG_FAILED));

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, "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, %u.

"remote peer version %d is not supported",
ep, ucp_ep_config(ep)->key.dst_version);
ep, UCP_EP_RECOVERY_MIN_DST_VERSION,
ucp_ep_config(ep)->key.dst_version);
return UCS_OK;
}

Expand Down
5 changes: 3 additions & 2 deletions src/ucp/core/ucp_rkey.c
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,8 @@ ucp_rkey_extract_sys_dev(const ucp_ep_config_t *ep_config, ucp_rkey_h rkey,
ucp_rkey_config_key_t *rkey_config_key)
{
if ((*buffer_p < buffer_end) ||
((ep_config->key.dst_version > 19) && (rkey->md_map != 0))) {
((ep_config->key.dst_version >= UCP_RKEY_SYS_DEV_MIN_DST_VERSION) &&
(rkey->md_map != 0))) {
rkey_config_key->sys_dev = *ucs_serialize_next(buffer_p, const uint8_t);
} else {
rkey_config_key->sys_dev = UCS_SYS_DEVICE_ID_UNKNOWN;
Expand Down Expand Up @@ -891,7 +892,7 @@ UCS_PROFILE_FUNC(ucs_status_t, ucp_rkey_proto_resolve,
/* Starting with UCX v1.20, lane distances are always packed if sys_dev is
* not UNKNOWN. Even if the rkey length is not explicitly passed to the API,
* we can safely assume that lane distances are included. */
if ((ep_config->key.dst_version > 19) &&
if ((ep_config->key.dst_version >= UCP_RKEY_SYS_DEV_MIN_DST_VERSION) &&
(rkey_config_key.sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN) &&
(buffer_end <= buffer)) {
buffer_end = (void*)UINTPTR_MAX;
Expand Down
34 changes: 34 additions & 0 deletions src/ucp/core/ucp_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,40 @@ typedef enum {
} ucp_object_version_t;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dst_version of a peer which does not pack its release version */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: UCP_RELEASE_LEGACY = 0 is now the first member of the list.

#define UCP_RELEASE_LEGACY 0


/*
* Minimal peer release versions of the features below, ordered by version.
* 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. UCP_RELEASE_LEGACY is below all of these versions.
*/
Comment thread
dmitrygx marked this conversation as resolved.
Resolved
enum {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for renamimg

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: all of them are UCP_MIN_DST_VERSION_* now, UCP_RELEASE_LEGACY became UCP_MIN_DST_VERSION_LEGACY.

/* The lane bandwidth includes the local distance */
UCP_WIREUP_BW_DISTANCE_MIN_DST_VERSION = 17,
/* The AM lane is separated from the RMA BW lanes */
UCP_WIREUP_SEPARATE_AM_LANE_MIN_DST_VERSION = 17,
/* Up to UCP_MAX_LANES lanes instead of UCP_MAX_LANES_LEGACY */
UCP_WIREUP_MAX_LANES_MIN_DST_VERSION = 18,
/* No legacy bits in the MD index field of a v1 worker address */
UCP_ADDRESS_MD_INDEX_MIN_DST_VERSION = 19,
/* UCX_CONNECT_ALL_TO_ALL connects all local and remote device pairs */
UCP_WIREUP_CONNECT_ALL_TO_ALL_MIN_DST_VERSION = 19,
/* A non-empty remote key always packs the system device */
UCP_RKEY_SYS_DEV_MIN_DST_VERSION = 20,
/* The PSN protocol of multi-fragment AM */
UCP_AM_PSN_MIN_DST_VERSION = 21,
/* The endpoint recovery */
UCP_EP_RECOVERY_MIN_DST_VERSION = 22,
/* The rendezvous protocols of RMA operations */
UCP_PROTO_RMA_RNDV_MIN_DST_VERSION = 22,
/* The token trailer of the LANES_ADDR messages */
UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION = 24
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



/**
* Active message tracer.
*/
Expand Down
1 change: 0 additions & 1 deletion src/ucp/rma/rma_rndv.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@


#define UCP_PROTO_RMA_RNDV_RTS_NAME "RMA_RTS"
#define UCP_PROTO_RMA_RNDV_MIN_DST_VERSION 22
#define UCP_PROTO_RMA_RNDV_ZERO_GET_PENALTY 1e-3


Expand Down
2 changes: 1 addition & 1 deletion src/ucp/wireup/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ exchanged out-of-band) and connection-manager wireup (sockaddr listener).
`UCP_WIREUP_MSG_*` constants in `wireup.h` and the matching pack/unpack
in `wireup.c`.
- A payload section appended to an already released message must be gated
by a `*_MIN_DST_VERSION` constant checked against
by a `*_MIN_DST_VERSION` constant from `core/ucp_types.h` checked against
`ucp_ep_config(ep)->key.dst_version`. Variable-length pieces go before
the rest of the payload so an older peer, which never sees them, still
finds the original contents. `LANES_ADDR` token trailers work this way,
Expand Down
5 changes: 2 additions & 3 deletions src/ucp/wireup/address.c
Original file line number Diff line number Diff line change
Expand Up @@ -1638,12 +1638,11 @@ static ucp_rsc_index_t ucp_address_get_remote_device_index(
static void
ucp_address_adjust_unpacked_md_index(ucp_unpacked_address_t *unpacked_address)
{
/* Last version that still packs legacy bits in MD index field */
const unsigned max_legacy_api_version = 18u;
size_t i;

if ((unpacked_address->addr_version > UCP_OBJECT_VERSION_V1) ||
(unpacked_address->dst_version > max_legacy_api_version)) {
(unpacked_address->dst_version >=
UCP_ADDRESS_MD_INDEX_MIN_DST_VERSION)) {
return;
}

Expand Down
16 changes: 10 additions & 6 deletions src/ucp/wireup/select.c
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,10 @@ static void ucp_wireup_select_transport_tiebreak(
static size_t
ucp_wireup_bw_max_lanes(const ucp_wireup_select_params_t *select_params)
{
return (select_params->address->dst_version < 18) ? UCP_MAX_LANES_LEGACY :
UCP_MAX_LANES;
return (select_params->address->dst_version <
Comment thread
dmitrygx marked this conversation as resolved.
Resolved
UCP_WIREUP_MAX_LANES_MIN_DST_VERSION) ?
UCP_MAX_LANES_LEGACY :
UCP_MAX_LANES;
Comment thread
dmitrygx marked this conversation as resolved.
Resolved
}

static size_t
Expand Down Expand Up @@ -1103,7 +1105,7 @@ ucp_wireup_iface_score_bandwidth(const ucp_worker_iface_t *wiface,
{
double local_bw;

if (unpacked_addr->dst_version < 17) {
if (unpacked_addr->dst_version < UCP_WIREUP_BW_DISTANCE_MIN_DST_VERSION) {
local_bw = ucp_tl_iface_bandwidth(wiface->worker->context,
&wiface->attr.bandwidth);
} else {
Expand Down Expand Up @@ -1454,7 +1456,8 @@ ucp_wireup_is_connect_all_to_all(const ucp_worker_h worker,
{
return worker->context->config.ext.connect_all_to_all &&
!(worker->uuid == unpacked_addr->uuid) && /* Not on self */
(unpacked_addr->dst_version >= 19);
(unpacked_addr->dst_version >=
UCP_WIREUP_CONNECT_ALL_TO_ALL_MIN_DST_VERSION);
}

static double
Expand Down Expand Up @@ -1710,7 +1713,7 @@ static double ucp_wireup_get_lane_bw(ucp_worker_h worker,
ucp_worker_iface_t *wiface = ucp_worker_iface(worker, sinfo->rsc_index);
double bw_local, bw_remote;

if (address->dst_version < 17) {
if (address->dst_version < UCP_WIREUP_BW_DISTANCE_MIN_DST_VERSION) {
bw_local = ucp_tl_iface_bandwidth(worker->context,
&wiface->attr.bandwidth);
} else {
Expand Down Expand Up @@ -2319,7 +2322,8 @@ ucp_wireup_add_rma_bw_lanes(const ucp_wireup_select_params_t *select_params,
* use of fence operations usually don't request the TAG feature, hence the
* check.
*/
if ((select_params->address->dst_version >= 17) &&
if ((select_params->address->dst_version >=
UCP_WIREUP_SEPARATE_AM_LANE_MIN_DST_VERSION) &&
(context->config.features & (UCP_FEATURE_TAG | UCP_FEATURE_AM))) {
ucs_carray_for_each(lane_desc, select_ctx->lane_descs,
select_ctx->num_lanes) {
Expand Down
9 changes: 0 additions & 9 deletions src/ucp/wireup/wireup.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ typedef struct {
* packed in the worker address */
#define UCP_WIREUP_EMPTY_PEER_NAME "<no debug data>"

#define UCP_RELEASE_LEGACY 0

#define UCP_WIREUP_UCT_EVENT_CAP_FLAGS \
(UCT_IFACE_FLAG_EVENT_SEND_COMP | UCT_IFACE_FLAG_EVENT_RECV)

Expand All @@ -59,13 +57,6 @@ enum {
};


/*
* Minimal peer release version which understands the token trailer of the
* LANES_ADDR messages. Older peers get the messages without any token.
*/
#define UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION 24


/**
* Calculates a score of a potential transport. Used both for the primary
* selection score and for the tiebreak score.
Expand Down