prov/efa & shm: Move srx lock from domain to ep - #12739
Conversation
3cba77d to
29db9f3
Compare
| uint64_t msg_id; | ||
| struct smr_region *volatile region; | ||
| struct fid_peer_srx *srx; | ||
| struct ofi_genlock *srx_lock; |
There was a problem hiding this comment.
What is the impact of such insertion on the cache locality of this struct members? I know it caused regressions when we insert members in the middle for shm
There was a problem hiding this comment.
I will move it to the end to see if there is a difference. I only tested SHM with FI_THREAD_SAFE and saw improvements.
There was a problem hiding this comment.
I don't see a difference when I move it to the end. It is a 8 byte pointer and naturally aligned. I will leave it next to srx where it is used with.
| efa_perfset_start(efa_rdm_ep, perf_efa_tx); | ||
|
|
||
| assert(ofi_genlock_held(&efa_rdm_ep_rdm_domain(efa_rdm_ep)->srx_lock)); | ||
| ofi_genlock_lock(&efa_rdm_ep->srx_lock); |
There was a problem hiding this comment.
Curious what is the advantage to move the lock from the caller to the callee?
There was a problem hiding this comment.
Only the callee need to lock now and the critical section is clearer
| * some idle endpoints and never poll completions for them. Move these initial posts to | ||
| * the first cq read call before having a long term fix. | ||
| */ | ||
| if (efa_rdm_cq->need_to_scan_ep_list) { |
There was a problem hiding this comment.
You need to move this lock before the check of need_to_scan_ep_list, as it should be protected in the race with the call that will update need_to_scan_ep_list in ep_enable, which is in the ep_list_lock too
| } | ||
|
|
||
| dlist_foreach(&efa_rdm_cq->ibv_cq_poll_list, item) { | ||
| poll_list_entry = container_of(item, struct efa_ibv_cq_poll_list_entry, entry); |
There was a problem hiding this comment.
This should be be protected by ep list lock right? isn't it?
| { | ||
| struct efa_cq *tx_cq, *rx_cq; | ||
|
|
||
| tx_cq = efa_base_ep_get_tx_cq(base_ep); |
There was a problem hiding this comment.
is it still cyclic include that force you to move this function?
| * order against the CQ read path and deadlock. | ||
| */ | ||
| if (efa_rdm_ep->base_ep.efa_qp_enabled) | ||
| efa_rdm_ep_wait_send(efa_rdm_ep); |
There was a problem hiding this comment.
I like to check if it was intended to make wait_send in the same lock with qp destruction.. I remember it used to be in a separate locking block and I moved it here...
| struct efa_rdm_domain { | ||
| struct efa_domain efa_domain; | ||
|
|
||
| struct ofi_genlock srx_lock; /* shared among peer providers */ |
There was a problem hiding this comment.
Please keep the domain SRX lock until all call sites have migrated to the endpoint lock.
Recommendation: Move the removal of srx_lock, together with its initialization and cleanup, to a separate commit between 3cbc93390 and 29db9f392.
Otherwise, checking out this commit during a git bisect results in a compilation failure.
There was a problem hiding this comment.
I split into multiple commits for easier review. Will squash into one commit.
| assert(msg->iov_count <= ep->base_ep.info->tx_attr->iov_limit); | ||
|
|
||
| efa_perfset_start(ep, perf_efa_tx); | ||
| ofi_genlock_lock(&ep->srx_lock); |
There was a problem hiding this comment.
Per-EP locks do not serialize updates to the CQ-shared progress_ep_list. Concurrent sends from different endpoints sharing the same CQ might corrupt the list when both are queued pending handshake.
Suggest to protect all accesses with a CQ-owned lock and release it before acquiring ep->srx_lock.
There was a problem hiding this comment.
good catch, updated
Use srx lock to serialize SHM's datapath and rx matching. When SHM is a peer provider, use the owner-provided srx lock so that the owner providers do not need to hold a domain level lock across fi_cq_read since they do not know SHM's ep lock. When SHM owns the SRX, default the srx lock to util_ep lock so there is no behavioral change. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
Multiple endpoints on the same cq could modify progress_ep_list concurrently. Protect it by adding efa_rdm_cq->progress_ep_list_lock. We cannot reuse util_cq.ep_list_lock because the lock order is ep_list_lock -> srx_lock, and the send path already holds srx_lock when it enqueues the progress list. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
FI_THREAD_SAFE applications contend on the domain srx lock. Move it to ep level so the send path is fully parallel across threads. It protects all the EP bufpools, peer lists, counters, etc. Remove the domain srx lock around the whole cq/cntr progress. Take its owning ep_list_lock to serialize efa_ibv_cq and take the ep srx lock of each CQE to serialize SRX recv-matching and per-ep ope/pke state. We cannot reuse util_ep.lock because efa_base_ep_close_util_ep calls ofi_endpoint_close, which destroys the lock but we need to hold a lock throughout the whole EP teardown. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
The data-path-direct wrid_idx_pool must be serialzied by the same lock that the post and completion paths hold. EFA RDM protocol uses efa_rdm_ep->srx_lock. EFA direct uses util_ep.lock. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
|
@zachdworkin Could you review the SHM commit? Thanks! |
3 participants
FI_THREAD_SAFE applications contend on the domain srx lock. Move it to ep level so the send path is fully parallel across threads.
Use srx lock to serialize SHM's datapath and rx matching.