Repository navigation
UCT/IB/MLX5/RC: Implement put_sgl_zcopy - #11677
Conversation
79fd323 to
754dd7f
Compare
3989009 to
0e7195a
Compare
| "put_sgl_zcopy count(%zu) should be limited by %zu", count, | ||
| max_count); | ||
| ucs_assert(ep->super.flags & UCT_RC_EP_FLAG_CONNECTED); | ||
| ucs_assert(count > 0); |
There was a problem hiding this comment.
In release the function will continue executing and underflow on count - 1.
I think it's best to check for count == 0 and return UCS_OK (probably best after the UCS_ERR_UNSUPPORTED check)
There was a problem hiding this comment.
Used UCT_SKIP_ZERO_LENGTH(count) to be consistent with the sge_cnt check in RC verbs send path (rc_verbs_ep.c).
| for (i = 0; i < count; i++) { | ||
| UCT_CHECK_LENGTH(lengths[i], 0, UCT_IB_MAX_MESSAGE_SIZE, | ||
| "put_sgl_zcopy"); | ||
| } |
There was a problem hiding this comment.
It would be better to do this validity check before line 213, because otherwise it might return UCS_ERR_NO_RESOURCE on an operation that would never succeed anyway
| txwq->super.qp_num, fm_ce_se, 0, wqe_size); | ||
|
|
||
| addr = remote_addrs[i]; | ||
| if (fence_flag != 0) { |
There was a problem hiding this comment.
should this check the return value of uct_rc_ep_fm() instead of fence_flag?
similar to uct_rc_mlx5_ep_fence_put()
There was a problem hiding this comment.
Good catch. The return value of uct_rc_ep_fm() is indeed the correct condition for choosing between the atomic and direct rkey paths. However, unlike uct_rc_mlx5_ep_fence_put(), this function handles multiple WQEs, so it should be called only once before the loop because it updates fence_beat, its result is then reused for the entire batch.
| { | ||
| size_t max_count = ucs_min((size_t)UCT_RC_MLX5_PUT_SGL_ZCOPY_MAX_COUNT, | ||
| iface->super.config.tx_qp_len); | ||
|
|
There was a problem hiding this comment.
| max_count = ucs_min(max_count, (size_t)(iface->super.config.tx_cq_len - 2)); |
In src/uct/ib/rc/base/rc_iface.c:590 it says:
/* Prevent title CQE overwriting */
self->tx.cq_available = tx_cq_size - 2;so the 2 reserved spaces need to be taken into account here too
There was a problem hiding this comment.
Good catch. By default, the SQ length is 256 and the TX CQ length is 4096, so the SQ is normally exhausted first. However, a user can configure a smaller CQ using UCX_RC_TX_CQ_LEN and expose a corner case where an SGL requires more credits than are currently available while cq_available is still nonzero, causing pending_add() to return BUSY and spin without progress.
However, your suggestion of hardcoding tx_cq_len - 2 would duplicate the reserved-CQ magic number. Instead, I added a shared helper as the single source of truth and a per-EP CQ reservation, similar to txqp_reserve in #11361, and ran the SGL gtests to confirm the fix.
1af61f3
What?
Implement
put_sgl_zcopyfor RC over mlx5.Why?
Posts all SGL elements in one submission.
How?
uct_rc_mlx5_base_ep_put_sgl_zcopywrites one RDMA-write WQE per element, then accounts the batch once and rings the doorbell viauct_ib_mlx5_txwq_ring_doorbell.