Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 `UCP_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_MIN_DST_VERSION_AM_PSN) {
return;
}

Expand Down
11 changes: 6 additions & 5 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, "
"remote peer version %d is not supported",
ep, ucp_ep_config(ep)->key.dst_version);
if (ucp_ep_config(ep)->key.dst_version < UCP_MIN_DST_VERSION_EP_RECOVERY) {
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 %u is not supported",
ep, UCP_MIN_DST_VERSION_EP_RECOVERY,
ucp_ep_config(ep)->key.dst_version);
return UCS_OK;
}

Expand Down Expand Up @@ -2734,7 +2735,7 @@ ucp_ep_tf_lane_supported(ucp_ep_h ep, ucp_lane_index_t lane, uct_ep_h uct_ep)
!ucp_ep_err_mode_eq(ep, UCP_ERR_HANDLING_MODE_FAILOVER) ||
ucp_ep_has_cm_lane(ep) ||
(ucp_ep_config(ep)->key.dst_version <
UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION) ||
UCP_MIN_DST_VERSION_WIREUP_ADDR_TOKEN) ||
(ucp_ep_get_lane(ep, lane) != uct_ep)) {
return 0;
}
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_MIN_DST_VERSION_RKEY_SYS_DEV) &&
(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_MIN_DST_VERSION_RKEY_SYS_DEV) &&
(rkey_config_key.sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN) &&
(buffer_end <= buffer)) {
buffer_end = (void*)UINTPTR_MAX;
Expand Down
35 changes: 35 additions & 0 deletions src/ucp/core/ucp_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,41 @@ 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 */
#define UCP_VERSION_LEGACY 0

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



/*
* Peer release versions (dst_version), ordered by version. A feature below is
* used with a peer only if dst_version of the peer is at least the given one,
* so peers of older releases keep working. The release version is packed in
* 4 bits, so dst_version of a peer more than 7 releases older can be wrong,
* see ucp_address_unpack_release_version().
*/
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_MIN_DST_VERSION_WIREUP_BW_DISTANCE = 17,
/* The AM lane is separated from the RMA BW lanes */
UCP_MIN_DST_VERSION_WIREUP_SEPARATE_AM_LANE = 17,
/* Up to UCP_MAX_LANES lanes instead of UCP_MAX_LANES_LEGACY */
UCP_MIN_DST_VERSION_WIREUP_MAX_LANES = 18,
/* No legacy bits in the MD index field of a v1 worker address */
UCP_MIN_DST_VERSION_ADDRESS_MD_INDEX = 19,
/* UCX_CONNECT_ALL_TO_ALL connects all local and remote device pairs */
UCP_MIN_DST_VERSION_WIREUP_CONNECT_ALL_TO_ALL = 19,
/* A non-empty remote key always packs the system device */
UCP_MIN_DST_VERSION_RKEY_SYS_DEV = 20,
/* The PSN protocol of multi-fragment AM */
UCP_MIN_DST_VERSION_AM_PSN = 21,
/* The endpoint recovery */
UCP_MIN_DST_VERSION_EP_RECOVERY = 22,
/* The rendezvous protocols of RMA operations */
UCP_MIN_DST_VERSION_PROTO_RMA_RNDV = 22,
/* The token trailer of the LANES_ADDR messages */
UCP_MIN_DST_VERSION_WIREUP_ADDR_TOKEN = 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
3 changes: 1 addition & 2 deletions 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 All @@ -36,7 +35,7 @@ ucp_proto_rma_rndv_probe_check(const ucp_proto_init_params_t *init_params,
ucp_proto_rndv_init_params_is_ppln_frag(init_params) ||
(sel_param->dt_class != UCP_DATATYPE_CONTIG) ||
(init_params->ep_config_key->dst_version <
UCP_PROTO_RMA_RNDV_MIN_DST_VERSION) ||
UCP_MIN_DST_VERSION_PROTO_RMA_RNDV) ||
(init_params->rkey_config_key == NULL)) {
return 0;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ucp/wireup/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ 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 `UCP_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,
see `UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION`: each token is prefixed by
see `UCP_MIN_DST_VERSION_WIREUP_ADDR_TOKEN`: each token is prefixed by
its own length, and the addresses stay last. Trailer lanes are taken
from `ucp_wireup_msg_lanes_info_t` rather than from maps of their own,
so keep the two in sync when changing either.
Expand Down
7 changes: 3 additions & 4 deletions src/ucp/wireup/address.c
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,7 @@ static void *ucp_address_unpack_header(const void *ptr,
if (*addr_version == UCP_OBJECT_VERSION_V1) {
*addr_flags = *addr_header >> UCP_ADDRESS_HEADER_SHIFT;
/* With address v1 it is packed after devices */
*dst_version = 0;
*dst_version = UCP_VERSION_LEGACY;
return UCS_PTR_TYPE_OFFSET(ptr, uint8_t);
}

Expand Down 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_MIN_DST_VERSION_ADDRESS_MD_INDEX)) {
return;
}

Expand Down
20 changes: 12 additions & 8 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_MIN_DST_VERSION_WIREUP_MAX_LANES) ?
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_MIN_DST_VERSION_WIREUP_BW_DISTANCE) {
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_MIN_DST_VERSION_WIREUP_CONNECT_ALL_TO_ALL);
}

static double
Expand Down Expand Up @@ -1524,7 +1527,7 @@ ucp_wireup_mem_reg_cost(ucp_context_t *context,
return md_attr->reg_cost;
}

if (remote_addr->dst_version > UCP_RELEASE_LEGACY) {
if (remote_addr->dst_version > UCP_VERSION_LEGACY) {
return ucs_linear_func_make(UCP_RCACHE_OVERHEAD_DEFAULT, 0);
}

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_MIN_DST_VERSION_WIREUP_BW_DISTANCE) {
bw_local = ucp_tl_iface_bandwidth(worker->context,
&wiface->attr.bandwidth);
} else {
Expand Down Expand Up @@ -2299,7 +2302,7 @@ ucp_wireup_add_rma_bw_lanes(const ucp_wireup_select_params_t *select_params,
bw_info.criteria.local_cmpt_flags = 0;

if (context->config.ext.proto_enable &&
(select_params->address->dst_version > UCP_RELEASE_LEGACY)) {
(select_params->address->dst_version > UCP_VERSION_LEGACY)) {
bw_info.max_lanes = ucp_wireup_max_lanes(select_params,
bw_info.criteria.lane_type);
} else {
Expand All @@ -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_MIN_DST_VERSION_WIREUP_SEPARATE_AM_LANE) &&
(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
2 changes: 1 addition & 1 deletion src/ucp/wireup/wireup.c
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ ucp_wireup_msg_prepare(ucp_ep_h ep, uint8_t type,
static int ucp_wireup_ep_supports_tokens(ucp_ep_h ep)
{
return ucp_ep_config(ep)->key.dst_version >=
UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION;
UCP_MIN_DST_VERSION_WIREUP_ADDR_TOKEN;
}

static size_t
Expand Down
11 changes: 1 addition & 10 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 Expand Up @@ -168,7 +159,7 @@ typedef struct ucp_wireup_msg_lanes_info_t {
* present, these are also the TX-token lanes. */
ucp_lane_map_t provided_lane_map;
/* @ref ucp_wireup_msg_tokens_info_t (only towards peers of release version
* UCP_WIREUP_ADDR_TOKEN_MIN_DST_VERSION and above), then packed addresses
* UCP_MIN_DST_VERSION_WIREUP_ADDR_TOKEN and above), then packed addresses
* follow */
} UCS_S_PACKED ucp_wireup_msg_lanes_info_t;

Expand Down
Loading