Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prov/efa: Check p2p support to use rdma read #10352

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

jiaxiyan
Copy link
Contributor

@jiaxiyan jiaxiyan commented Aug 30, 2024

p2p is required for rdma read because the user buffer has to be registered to efa device.
Add p2p check for subprotocols that use RDMA read.

@jiaxiyan jiaxiyan requested a review from a team August 30, 2024 19:59
@shijin-aws
Copy link
Contributor

Do you need similar check for the write path?
https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/rdm/efa_rdm_rma.c#L393-L396

@@ -90,7 +90,8 @@ int efa_rdm_msg_select_rtm(struct efa_rdm_ep *efa_rdm_ep, struct efa_rdm_ope *tx

if (txe->total_len >= hmem_info[iface].min_read_msg_size &&
efa_rdm_interop_rdma_read(efa_rdm_ep, txe->peer) &&
(txe->desc[0] || efa_is_cache_available(efa_rdm_ep_domain(efa_rdm_ep))))
(txe->desc[0] || efa_is_cache_available(efa_rdm_ep_domain(efa_rdm_ep))) &&
use_p2p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make more sense to refactor the whole conditional s.t. use_p2p is first? That way we can short-circuit and avoid the remaining (potentially expensive) logic.

prov/efa/src/rdm/efa_rdm_rma.c Outdated Show resolved Hide resolved
p2p is required for rdma read because the user buffer has to be
registered to efa device.
Add p2p check for subprotocols that use RDMA read.

Signed-off-by: Jessie Yang <[email protected]>
@shijin-aws
Copy link
Contributor

AWS CI failure is unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants