prov/efa: always use p2p for system memory

[jiaxiyan]

For system memory, it should use p2p even with unregistered buffer. Otherwise it will affect performance.

[shijin-aws]

I am concerned that deaa841 may also be impacted by this issue. You checked

efa_rdm_ep_use_p2p(ep, rxe->desc[0]);

to determine if we can do p2p for the receive buffer. If receive buffer is unregistered and on host, we should allow rdma read as well.

I think instead of adding such iface check everywhere, we should investigate whether we can just make efa_rdm_ep_use_p2p return true for unregistered buffer. You need to audit whether that change can cause any behavior change. My most concern is here https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/rdm/efa_rdm_pke_utils.c#L69. But it looks it is called when we have a valid iov_mr, so it shouldn't cause a problem if we make efa_rdm_ep_use_p2p return true for NULL mr.

[shijin-aws]

Better to add a assert(efa_mr) here

[shijin-aws]

This comment is not correct. We are not always sending from host buffers if efa_mr is NULL. For eager send, we will do a copy from host buffer to registered bounce buffer.

We can just change it to

P2p is always available for host memory (Unregistered buffer will be regarded as host memory as EFA provider requires FI_MR_HMEM)

[shijin-aws]

Need to backport

[darrylabbate]

I'm a little dubious of this change and can see it easily becoming a future bug. If the null txe->desc[0] being passed here was netting an undesired result, wouldn't it limit the blast radius to simply pass true for use_p2p in efa_rdm_msg_select_rtm() when txe->desc[0] is null?

I see that we've walked through every caller of efa_rdm_ep_use_p2p(), but changing the logic for the null parameter just strikes me as something that will surprise (and not delight) us later.

[shijin-aws]

I'm a little dubious of this change and can see it easily becoming a future bug. If the null txe->desc[0] being passed here was netting an undesired result, wouldn't it limit the blast radius to simply pass true for use_p2p in efa_rdm_msg_select_rtm() when txe->desc[0] is null?

I see that we've walked through every caller of efa_rdm_ep_use_p2p(), but changing the logic for the null parameter just strikes me as something that will surprise (and not delight) us later.

I don't think it can become a bug, efa_rdm_ep_use_p2p should be interpreted as whether p2p can be used for a given MR. It is a valid deduction that NULL desc should be host memory, and we should always support p2p for host memory, I don't think it is wrong to make this function return true for NULL desc. It also saves changes everywhere to check desc[0] explicitly before calling efa_rdm_ep_use_p2p()