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

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

[Mohamed Khalfella]

Local operations (IB_WR_REG_MR, IB_WR_LOCAL_INV, IB_WR_BIND_MW) are
executed inline by rxe_requester() through rxe_do_local_ops(), which
marks the wqe wqe_state_done and advances qp->req.wqe_index past it.
Unlike wire operations, nothing is posted to the network and no ack is
expected.

The completer only consumes wqes from the head of the send queue, in
order. A done local op wqe therefore stays in the queue until every
older wqe has completed, so it can sit behind a SEND or RDMA WRITE that
is still waiting for an ack. With nvme-rdma, which posts REG_MR + SEND
for every I/O, the queue regularly looks like:

  SEND(A) pending, REG_MR(B) done, SEND(B) pending, REG_MR(C) done ...

When a retransmit timeout fires or a NAK arrives, req_retry() rewinds
qp->req.wqe_index to the consumer so that every unacked wqe is resent.
Its loop skips wqe_state_done entries when resetting DMA state, but
rxe_requester() itself has no such check. After the rewind it walks
forward through the queue, reaches REG_MR(B), sees WR_LOCAL_OP_MASK and
calls rxe_do_local_ops() on it a second time.

rxe_reg_fast_mr() rejects the repeated registration because the MR is
already RXE_MR_STATE_VALID, not FREE. The wqe is given
IB_WC_LOC_QP_OP_ERR and the QP is moved to the error state. The
completer then flushes the entire send queue with IB_WC_WR_FLUSH_ERR,
including the wqe that failed, so the consumer never sees the real
error status. nvme-rdma logs only "SEND ... failed with status WR
flushed (5)" and tears down the controller.

Any packet loss that triggers a retry while a later request is queued
is enough to hit this. On a soft-RoCE setup with a few hundred lost
frames per run every controller reconnect traced back to the same
rxe_dbg_mr() line:

  rxe1: mr#776 rxe_reg_fast_mr:  mr->lkey = 0x3084c not free

Skip local op wqes that are already done when the requester reaches
them again after a rewind. Advance wqe_index past them exactly as a
successful execution would and continue with the next wqe. The MR keeps
its registered state and the completer still reports the wqe in order
with its original status. Only local ops can be in wqe_state_done ahead
of the consumer on an RC QP, since wire operations stay pending until
acked, so the check cannot match anything else.

Fixes: 001345339f4c ("RDMA/rxe: Separate HW and SW l/rkeys")
Cc: stable@vger.kernel.org
Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/infiniband/sw/rxe/rxe_req.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 4f353abd76f5..fdb55366d668 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -723,6 +723,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;
-- 
2.56.0

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

[sashiko-bot]

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