Re: [PATCH] RDMA/rxe: Do not re-execute completed local ops after a retry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] RDMA/rxe: completer consumer index can overtake wqe_index in
  rxe_requester()
--

commit d3f6a5448675a7f592bfd812042efdb600a6a02c
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

RDMA/rxe: Do not re-execute completed local ops after a retry

This commit skips local operation WQEs that are already in wqe_state_done
when rxe_requester() walks the send queue again after a retry rewind.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 6e01fb18893bf..ee89468435902 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -781,6 +781,20 @@ int rxe_requester(struct rxe_qp *qp)
>  		goto err;
>  	}
>  
> +	/*
> +	 * A retry rewinds wqe_index to the send queue consumer. Local
> +	 * operations that were already executed may still sit in the
> +	 * queue behind an unacked wqe waiting for in-order completion.
> +	 * Re-executing them fails (e.g. REG_MR on an MR that is no
> +	 * longer free) and would move the QP to the error state, so
> +	 * skip them instead.
> +	 */
> +	if ((wqe->mask & WR_LOCAL_OP_MASK) &&
> +	    wqe->state == wqe_state_done) {
> +		qp->req.wqe_index = queue_next_index(q, qp->req.wqe_index);
> +		goto done;
> +	}
> +
>  	if (rxe_wqe_is_fenced(qp, wqe)) {
>  		qp->req.wait_fence = 1;
>  		goto exit;

[Severity: High]
Can the send queue consumer index in rxe_completer() overtake
qp->req.wqe_index after a retry?

rxe_sender() alternates one call to rxe_requester(), which advances
qp->req.wqe_index by at most one WQE per call before jumping to done,
with one call to rxe_completer(), where complete_wqe() loops back to
COMPST_GET_WQE and completes all consecutive wqe_state_done WQEs at the
head of the send queue in a single pass:

drivers/infiniband/sw/rxe/rxe_comp.c:get_wqe() {
	...
	/* WQE does not require an ack */
	if (wqe->state == wqe_state_done)
		return COMPST_COMP_WQE;
	...
}

After req_retry() rewinds qp->req.wqe_index on a queue with SEND(A)
followed by REG_MR(B) in wqe_state_done and SEND(B), rxe_requester()
resends SEND(A) and advances qp->req.wqe_index to REG_MR(B).  If
rxe_completer() then processes a cumulative ACK or PSN sequence error NAK
that completes SEND(A), its loop immediately completes REG_MR(B) as well,
advancing the consumer index past qp->req.wqe_index (the same happens when
two completed local ops sit back to back).  Once the consumer index
overtakes qp->req.wqe_index, newly posted WQEs that wrap the ring can
either stall the send queue when __req_next_wqe() sees index == prod or be
fetched and executed out of order and a second time when qp->req.wqe_index
catches up.

In addition, because this check is placed after req_next_wqe(), if the QP
is in IB_QPS_SQD when qp->req.wqe_index reaches a wqe_state_done local op,
req_next_wqe() returns NULL because wqe->state != wqe_state_processing,
preventing req_check_sq_drain_done() from seeing index == cons.

Could req_next_wqe() instead loop past wqe_state_done local ops before the
IB_QPS_SQD check so qp->req.wqe_index always stays ahead of the completer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113230831.2682071-1-mkhalfella@purestorage.com?part=1