rdma: Inject single-receive control messages - #1377
Conversation
| nccl_net_ofi_rdma_cq_rail_t *cq_rail, | ||
| uint32_t tclass); | ||
| uint32_t tclass, | ||
| bool is_control); |
There was a problem hiding this comment.
if we're going to put the is_control here, then the tclass doesn't belong here.
| /* Standard NIC info used to create data endpoints. */ | ||
| struct fi_info *info = nullptr; | ||
|
|
||
| /* NIC info satisfying the single-entry inject requirement, used only |
There was a problem hiding this comment.
we don't need two info objects stored. Just modify as needed during creation of the endpoint.
There was a problem hiding this comment.
Wanted to separate them. Now I have reverted it to be a single struct fi_info *info as it was.
| /* Provider list requested with a single-entry RMA inject-size hint. It is | ||
| * deliberately separate from the standard list used to build topology | ||
| * and data endpoints. */ | ||
| ofi_info_ptr control_provider_list; |
There was a problem hiding this comment.
same we shouldn't need this comment.
| static size_t max_control_write_inline_size = 0; | ||
| static bool is_max_write_inline_size_initialized = false; | ||
|
|
||
| static inline bool use_inline_control_write(uint16_t num_recvs, size_t ctrl_msg_len) |
There was a problem hiding this comment.
this should be a member of something.
There was a problem hiding this comment.
Now it is
bool nccl_net_ofi_rdma_ep_t::use_inline_control_write(uint16_t num_recvs,
size_t ctrl_msg_len) const
| void *context = rdma_req_get_ofi_context(this, rail_id); | ||
| ssize_t rc; | ||
|
|
||
| if (use_inline_control_write(ctrl_num_recvs, ctrl_msg_len)) { |
There was a problem hiding this comment.
this is a really big conditional for what should be only 2-3 fields being different.
| ret = 0; | ||
| } else { | ||
| NCCL_OFI_WARN("Failed to retrieve maximum write inline size"); | ||
| int ret = get_inject_rma_size_opt(ep, inline_size); |
There was a problem hiding this comment.
i no longer understand why this is a function, if it's just going to call one function.
| static inline int init_max_write_inline_size_if_not_initialized(nccl_net_ofi_rdma_device_t *device, | ||
| nccl_net_ofi_rdma_ep_t *ep) | ||
| { | ||
| if (is_max_write_inline_size_initialized) { |
There was a problem hiding this comment.
again, member functions, and then you shoulnd't need this multiple call stuff.
|
|
||
| nccl_net_ofi_rdma_plugin_t::nccl_net_ofi_rdma_plugin_t(struct fi_info *provider_list, nccl_ofi_topo_t *global_topo) | ||
| : nccl_net_ofi_plugin_t(global_topo) | ||
| static bool provider_info_matches_nic(const struct fi_info *a, const struct fi_info *b) |
There was a problem hiding this comment.
lots of unrelated changes here.
5ed9c6c to
6232077
Compare
d116cf7 to
c37502c
Compare
3ba85b1 to
536dda6
Compare
536dda6 to
001b403
Compare
| r_comm->remote_mailbox_addr + slot * sizeof(nccl_net_ofi_ctrl_msg_t), | ||
| r_comm->remote_mr_key[rail_id], | ||
| rdma_req_get_ofi_context(this, rail_id)); | ||
| uint64_t remote_addr = r_comm->remote_mailbox_addr + |
There was a problem hiding this comment.
This would be easier to review as two commits. The first switches from fi_write() to fi_write_msg() and the second adds the FI_INJECT bit. They're only tangentially related.
But I also wonder if this is all really worth not just having an
if (inject) {
fi_inject_write(....);
} else {
fi_write(...);
}
There was a problem hiding this comment.
Previously, I had a big if else and in one of previous reviews (#1377 (comment)) it was pointed out that it could be simplified. Please let me know if I understood that correctly.
There was a problem hiding this comment.
you had write vs. writemsg previously.
There was a problem hiding this comment.
Done. The PR is now two commits: the first converts the control-message post from fi_write to fi_writemsg with no functional change, and the second adds the endpoint negotiation and the FI_INJECT bit.
Regarding fi_inject_write: my understanding is that it suppresses the success completion.
fi_rma(3) says:
it "provides similar completion semantics as fi_inject"
and fi_msg(3) defines those as:
"no CQ entry will be written if the transfer completes successfully... the CQ entry will be suppressed even if the default behavior of the endpoint is to write CQ entries for all successful completions."
Do you think that our completion checks in rdma_recv_req::handle_completion() that complete the receive request and increments n_ctrl_delivered need to be thought out. I haven't looked into in as detail. However, I did try using fi_inject_write to experiment. A 2-node all_reduce hangs on the first collective.
Maybe I am completely off here.
The control message write passes its arguments positionally to fi_write, which provides no way to vary the flags of an individual post. Express the same write as an fi_msg_rma and post it with fi_writemsg, which takes a per-post flags argument. The message describes the same single-element local and remote regions and carries the same memory region descriptor and request context. No flags are passed, so the transfer itself is unchanged. Signed-off-by: Bibrak Qamar Chandio <bibracha@amazon.com>
A single-receive RDMA control message contains one 64-byte entry. The control message is currently sent from registered memory even when the provider can copy the entry into the endpoint during the post. The RDMA transport already creates separate endpoints for control and data traffic. Request provider information with enough inject space for one control entry and use it only when creating control endpoints. Data endpoints continue to use the standard provider information, so the control requirement cannot reduce their transmit queue capacity. Post single-entry control messages with FI_INJECT when the control endpoint reports sufficient RMA inject capacity. Providers that cannot satisfy the requested size, as well as grouped control messages, continue to post from registered memory. Signed-off-by: Bibrak Qamar Chandio <bibracha@amazon.com>
001b403 to
121facf
Compare
5 participants
Description of changes:
A single-receive RDMA control message contains one 64-byte entry. The control message is currently sent from registered memory even when the provider can copy the entry into the endpoint during the post.
The RDMA transport already creates separate endpoints for control and data traffic. Request provider information with enough inject space for one control entry and use it only when creating control endpoints. Data endpoints continue to use the standard provider information, so the control requirement cannot reduce their transmit queue capacity.
Post single-entry control messages with
FI_INJECTwhen the control endpoint reports sufficient RMA inject capacity. Providers that cannot satisfy the requested size, as well as grouped control messages, continue to use the registeredfi_writepath.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.