Repository navigation
UCT: outstanding purge implementation - #11668
Conversation
Rebuild the transport implementation on top of the API-only failover_ext_uct branch: - align naming to the finalized "purge" API (outstanding purge callback, params and field mask), dropping the intermediate "extract"/flags shape - switch the mlx5 external plugin path from qp_query to ep_query, with RX token derivation performed by the iface query - re-add the uct_ep_outstanding_purge()/uct_ep_failover_enable() prototypes and the uct_ib_mlx5_ext_ep_outstanding_purge() dispatch wrapper - update gtests (test_ib_mlx5_ext, test_uct_outstanding_purge) accordingly
| return UCS_ERR_INVALID_PARAM; | ||
| } | ||
|
|
||
| ucs_list_for_each(plugin, &uct_ib_mlx5_ext_plugins, list) { |
There was a problem hiding this comment.
iface_query, ep_query, and purge each select a provider independently. Can we bind this whole flow to one provider? @evgeny-leksikov wdyt?
There was a problem hiding this comment.
this is an open question, see my comment #11668 (comment)
| .ep_get_device_ep = (uct_ep_get_device_ep_func_t)ucs_empty_function_return_unsupported, | ||
| .ep_put_sgl_zcopy = uct_ib_mlx5_ext_ep_put_sgl_zcopy | ||
| .ep_put_sgl_zcopy = uct_ib_mlx5_ext_ep_put_sgl_zcopy, | ||
| .ep_outstanding_purge = uct_ib_mlx5_ext_ep_outstanding_purge |
There was a problem hiding this comment.
How does the caller discover purge support? QUERY_TOKEN can be advertised even when ep_query or purge is missing. Should we add a capability for purge, or require the complete query/derive/purge set?
There was a problem hiding this comment.
Currently, we only need 1 cap for all token related queries and functions.
I added a helper uct_ib_mlx5_ext_find_plugin(uct_iface_h iface, uint64_t cap_flags).
When token related mask is set, we will choose the plugin with QUERY_TOKEN set in iface cap only.
| } | ||
|
|
||
| if (ext_attr.field_mask != 0) { | ||
| status = uct_ib_mlx5_ext_iface_query(iface, &ext_attr); |
There was a problem hiding this comment.
Without a plugin, requesting token lengths now fails the entire query. Other transports return success with zero lengths and no capability bit..
There was a problem hiding this comment.
I changed uct_iface_base_query_v2.
Now it returns unsupport if trying to query tokens lengh/rx token.
Therefore, without a TOKEN_QUERY supported plugin, all token related query/func will be unsupported.
| iface_attr->max_put_sgl_zcopy_count = max_sgl; | ||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN) { | ||
| ext_attr.field_mask |= UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_TX_TOKEN; | ||
| ext_attr.tx_token = (void*)iface_attr->tx_token; |
There was a problem hiding this comment.
This casts away the const from the public TX-token input. Can we make the plugin attribute const void * instead?
# Conflicts: # src/uct/ib/mlx5/rc/rc_mlx5_iface.c
|
@jeynmann as discussed, please make the tests much shorter and simpler, as they seem to be an overkill at the moment. |
| #define UCT_IFACE_FLAG_V2_QUERY_TOKEN UCS_BIT(1) /**< Interface supports TX/RX token | ||
| length query, RX token derivation | ||
| from a peer TX token, endpoint TX | ||
| token query, and outstanding | ||
| operation purge. */ |
There was a problem hiding this comment.
maybe just to reference to APIs that must be supported if this bit is present?
| * The endpoint must be quiesced by the caller: no new operations may be posted | ||
| * on @a ep while this function is running, and ownership of the endpoint's |
There was a problem hiding this comment.
what does it mean? any UCT API is not thread safe by default
| * On success, callbacks are invoked in the original endpoint posting order for | ||
| * operations classified as undelivered. A callback may replay the operation | ||
| * immediately, including before this function returns. |
There was a problem hiding this comment.
it looks like a certain use-case rather then description of what this function is doing
| * | ||
| * If this function returns an error, callbacks might already have been invoked | ||
| * for some operations. Those callbacks remain owned by the caller; the provider | ||
| * reports failure for the first operation whose state could not be classified or |
There was a problem hiding this comment.
no need to mention any implementation specifics, e.g. provider's behavior
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_CAP_FLAGS) { | ||
| iface_attr->cap.flags = UCT_IFACE_FLAG_V2_PUT_SGL_ZCOPY; | ||
| ext_attr.field_mask |= UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_CAP_FLAGS; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN_LENGTH) { | ||
| ext_attr.field_mask |= | ||
| UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_TX_TOKEN_LEN; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN_LENGTH) { | ||
| ext_attr.field_mask |= | ||
| UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_RX_TOKEN_LEN; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_MAX_PUT_SGL_ZCOPY_COUNT) { | ||
| iface_attr->max_put_sgl_zcopy_count = max_sgl; | ||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN) { | ||
| ext_attr.field_mask |= UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_TX_TOKEN; | ||
| ext_attr.tx_token = iface_attr->tx_token; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN) { | ||
| ext_attr.field_mask |= UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_RX_TOKEN; | ||
| ext_attr.rx_token = iface_attr->rx_token; | ||
| } |
There was a problem hiding this comment.
filling of ext_attrcould be a separate function
| uct_ib_mlx5_ext_iface_query_attr_t attr = { | ||
| .field_mask = UCT_IB_MLX5_EXT_IFACE_QUERY_ATTR_FIELD_CAP_FLAGS | ||
| }; |
There was a problem hiding this comment.
attr is "in/out-parameter", need to re-initialize on each call
| if (ucs_unlikely(uct_ib_mlx5_ext_is_unsupported_op( | ||
| (const void*)plugin->ops.iface_query))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
do we still need this check if we have the capability?
There was a problem hiding this comment.
Removing this check means we are making a strong assumption here: every plugin must provide iface query, and the capability flag is provided only by iface query. Is this ok?
There was a problem hiding this comment.
IMO ucs_assert would be enough, like for ep_query
| plugin = uct_ib_mlx5_ext_find_plugin(ep->iface, | ||
| UCT_IFACE_FLAG_V2_QUERY_TOKEN); | ||
| if ((plugin == NULL) || (uct_ib_mlx5_ext_is_unsupported_op( | ||
| (const void*)plugin->ops.ep_query))) { |
There was a problem hiding this comment.
according to documentation UCT_IFACE_FLAG_V2_QUERY_TOKEN must guarantee ep_query with TX token, right?
There was a problem hiding this comment.
Yes. Just in case the open source plugin doesn't follow the convention. Removed.
| plugin = uct_ib_mlx5_ext_find_plugin(ep->iface, | ||
| UCT_IFACE_FLAG_V2_QUERY_TOKEN); | ||
| if ((plugin == NULL) || | ||
| (uct_ib_mlx5_ext_is_unsupported_op( | ||
| (const void*)plugin->ops.ep_outstanding_purge))) { | ||
| return UCS_ERR_UNSUPPORTED; | ||
| } | ||
|
|
||
| status = plugin->ops.ep_outstanding_purge(ep, params); |
There was a problem hiding this comment.
we could safely call plugin->ops.ep_outstanding_purge(ep, params); on all plugins if token contains a signature and/or versioning, then each plugin could purge its ops or return error if signature does not match
| uct_rc_mlx5_iface_common_t *mlx5_iface; | ||
| uct_ib_mlx5_ext_iface_query_attr_t ext_attr = {}; |
There was a problem hiding this comment.
code style: initialized variables go first
| uct_rc_mlx5_iface_common_t *mlx5_iface; | |
| uct_ib_mlx5_ext_iface_query_attr_t ext_attr = {}; | |
| uct_ib_mlx5_ext_iface_query_attr_t ext_attr = { 0 }; | |
| uct_rc_mlx5_iface_common_t *mlx5_iface; |
| if (ext_attr.field_mask != 0) { | ||
| status = uct_ib_mlx5_ext_iface_query(tl_iface, &ext_attr); | ||
| if (status != UCS_OK) { | ||
| return status; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_CAP_FLAGS) { | ||
| iface_attr->cap.flags |= ext_attr.cap.flags; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN_LENGTH) { | ||
| iface_attr->tx_token_length = ext_attr.tx_token_len; | ||
| } | ||
|
|
||
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN_LENGTH) { | ||
| iface_attr->rx_token_length = ext_attr.rx_token_len; | ||
| } | ||
| } | ||
|
|
||
| return UCS_OK; |
There was a problem hiding this comment.
| if (ext_attr.field_mask != 0) { | |
| status = uct_ib_mlx5_ext_iface_query(tl_iface, &ext_attr); | |
| if (status != UCS_OK) { | |
| return status; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_CAP_FLAGS) { | |
| iface_attr->cap.flags |= ext_attr.cap.flags; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN_LENGTH) { | |
| iface_attr->tx_token_length = ext_attr.tx_token_len; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN_LENGTH) { | |
| iface_attr->rx_token_length = ext_attr.rx_token_len; | |
| } | |
| } | |
| return UCS_OK; | |
| if (ext_attr.field_mask == 0) { | |
| return UCS_OK | |
| } | |
| status = uct_ib_mlx5_ext_iface_query(tl_iface, &ext_attr); | |
| if (status != UCS_OK) { | |
| return status; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_CAP_FLAGS) { | |
| iface_attr->cap.flags |= ext_attr.cap.flags; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_TX_TOKEN_LENGTH) { | |
| iface_attr->tx_token_length = ext_attr.tx_token_len; | |
| } | |
| if (iface_attr->field_mask & UCT_IFACE_ATTR_FIELD_RX_TOKEN_LENGTH) { | |
| iface_attr->rx_token_length = ext_attr.rx_token_len; | |
| } | |
| return UCS_OK; |
| if (ucs_unlikely(uct_ib_mlx5_ext_is_unsupported_op( | ||
| (const void*)plugin->ops.iface_query))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
IMO ucs_assert would be enough, like for ep_query
|
@evgeny-leksikov plz re-review |
| ucs_status_t | ||
| uct_ib_mlx5_ext_ep_query(uct_ep_h ep, uct_ib_mlx5_ext_ep_query_attr_t *attr) | ||
| { | ||
| const uint64_t token_mask = UCT_IB_MLX5_EXT_EP_QUERY_ATTR_FIELD_TX_TOKEN; |
What?
Implemente uct outstanding purge api.
Why?
How?
Add param checks in uct api.
Call ext plugin in uct api.
Add gtest cases for plugin stubs.