UCP/WIREUP: Introduce address request/reply wireup messages #10684
Conversation
…tor lane access functions - Refactored lane access functions to separate raw lane retrieval from the common access method. - Added new wireup message types: ADDR_REQUEST and ADDR_REPLY. - Refacored KA reply EP to be reused for ADDR_REPLY.
| if (type == UCP_WIREUP_MSG_ADDR_REQUEST) { | ||
| /* Reuse KA reply EP for simplicity of EP lifetime management */ | ||
| msg_hdr->dst_ep_id = UCS_PTR_MAP_KEY_INVALID; | ||
| } else if (ep->flags & UCP_EP_FLAG_REMOTE_ID) { |
There was a problem hiding this comment.
When no EP on remote side need to create temporary EP to reply, the logic is the same as in case of KA
There was a problem hiding this comment.
the handler support the case of existing EP in peer (line 993).
no need to set msg_hdr->dst_ep_id to INVALID.
There was a problem hiding this comment.
leftover, removed for simplicity
There was a problem hiding this comment.
So better block ep_remote_id initialization in ADDR_REPLY handler (rather than block here).
There was a problem hiding this comment.
it's blocked on both sides: sender does not send, receiver checks this with an assertion
There was a problem hiding this comment.
it's not required here.
Instead:
void handle_address_reply()
{
/* don't call set_remote_id() */
}
There was a problem hiding this comment.
ADDR_REPLY handler is not impplemented in this PR and this message must contain remote EP id bcz it targets specific EP
There was a problem hiding this comment.
but then we create dependency between on-demand lanes feature and keepalive feature.
what if we use the same logic as for ucp_ep_resolve_remote_id?
or, for to-iface case, we can extract the remote iface address from existing (pairwise) lanes and use it to connect all-to-all lanes?
| ucs_assert(ep == NULL); | ||
| ucp_wireup_send_reply_on_onetime_ep(worker, msg, |
There was a problem hiding this comment.
can't EP be non-NULL? for example if some p2p lanes were previously connected
There was a problem hiding this comment.
It can but it leads to other bugs which are currently not addressed, so for simplicity I decided always send ack on temporary EP, re-usage of existing EP could be a good next step optimization
| if (type == UCP_WIREUP_MSG_ADDR_REQUEST) { | ||
| /* Reuse KA reply EP for simplicity of EP lifetime management */ | ||
| msg_hdr->dst_ep_id = UCS_PTR_MAP_KEY_INVALID; | ||
| } else if (ep->flags & UCP_EP_FLAG_REMOTE_ID) { |
There was a problem hiding this comment.
So better block ep_remote_id initialization in ADDR_REPLY handler (rather than block here).
What?
Why?
Part of #10640 to minimize its size