Skip to content

prov/efa: avoid an extra ep_list_lock acquire per cq progress - #13178

Merged
charlesstoll merged 1 commit into
ofiwg:mainfrom
charlesstoll:ep_list_lock_perf_improvement
Nov 10, 2026
Merged

charlesstoll merged 1 commit into
ofiwg:mainfrom
charlesstoll:ep_list_lock_perf_improvement

Conversation

@charlesstoll

Copy link
Copy Markdown
Contributor

During efa_rdm_cq_progress(), we loop through all cqs that are connected to the inputted cq and progress them all. This means both the tx/rx cqs that comprise the inputted cq as well as all cqs bound to any eps that are bound to this cq. This patch removes a single lock/unlock for each progress call by using the fact that we can just hold onto the ep list lock for our directly bound ep through both its ep list iteration and through its cq poll function.

This brings the total number of locking/release events from O(N)->O(N-1), which for simple cases where each cq is bound only to a single ep, means locking events go from 3->2.

@charlesstoll
charlesstoll requested a review from jiaxiyan November 9, 2026 19:38
Comment thread prov/efa/src/rdm/efa_rdm_cq.c Resolved
@charlesstoll
charlesstoll force-pushed the ep_list_lock_perf_improvement branch from 593a860 to fc2de16 Compare November 9, 2026 23:10

@darrylabbate darrylabbate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure anything is wrong with the change at a high level. A gain is a gain, after all. But assuming the perf is the same, I think this is slightly easier to reason about (and you drop own_lock_held):

EFA_GENLOCK_LOCK(&efa_rdm_cq->ibv_cq_poll_list_lock, efa_ibv_cq_poll_list_lock_sym);
EFA_GENLOCK_LOCK(&cq->ep_list_lock, efa_cq_ep_list_lock_sym);

if (efa_rdm_cq->need_to_scan_ep_list) {
	// unchanged
}

// Poll own IBV CQ once
(void) efa_rdm_cq_poll_ibv_cq(efa_env.efa_cq_read_size, &efa_rdm_cq->efa_cq.ibv_cq);

// Unlock own EP list lock
EFA_GENLOCK_UNLOCK(&cq->ep_list_lock, efa_cq_ep_list_lock_sym);

// Poll each other IBV CQ under its EP list lock; skipping own
dlist_foreach(&efa_rdm_cq->ibv_cq_poll_list, item) {
	poll_list_entry = container_of(item, struct efa_ibv_cq_poll_list_entry, entry);
	efa_cq = container_of(poll_list_entry->cq, struct efa_cq, ibv_cq);
	if (efa_cq == &efa_rdm_cq->efa_cq)
		continue;
	EFA_GENLOCK_LOCK(&efa_cq->util_cq.ep_list_lock, efa_cq_ep_list_lock_sym);
	(void) efa_rdm_cq_poll_ibv_cq(efa_env.efa_cq_read_size, poll_list_entry->cq);
	EFA_GENLOCK_UNLOCK(&efa_cq->util_cq.ep_list_lock, efa_cq_ep_list_lock_sym);
}

EFA_GENLOCK_UNLOCK(&efa_rdm_cq->ibv_cq_poll_list_lock, efa_ibv_cq_poll_list_lock_sym);

I believe you can also keep OFI_TSA_NO_ANALYSIS

During efa_rdm_cq_progress(), we loop through all cqs that are connected
to the inputted cq and progress them all. This means both the tx/rx cqs
that comprise the inputted cq as well as all cqs bound to any eps that
are bound to this cq. This patch removes a single lock/unlock for each
progress call by using the fact that we can just hold onto the ep list
lock for our directly bound ep through both its ep list iteration and
through its cq poll function.

This brings the total number of locking/release events from O(N)->O(N-1),
which for simple cases where each cq is bound only to a single ep, means
locking events go from 3->2.

Signed-off-by: Charles Stoll <stollcha@amazon.com>
@charlesstoll
charlesstoll force-pushed the ep_list_lock_perf_improvement branch from fc2de16 to f9c3825 Compare November 10, 2026 17:58

@darrylabbate darrylabbate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$O(N)$ is the same as $O(N-1)$ btw 😛

@charlesstoll
charlesstoll merged commit 8384be5 into ofiwg:main Nov 10, 2026
27 checks passed
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.

3 participants