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

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Sat 2026-11-14 11:05:39 -0800, Zhu Yanjun wrote:
> 
> 在 2026/11/13 15:08, 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;
> > +	}
> 
> I understand the need for this check to prevent already-completed local 
> operations from
> 
> being re-executed after a retry rewinds the WQE index.
> 
> However, this check is currently placed in the common `rxe_requester()` 
> path, so it is evaluated
> 
> for every WQE, including normal SEND and RDMA WRITE operations.
> 
> Since this condition is only relevant when a retry causes the requester 
> to revisit an already-completed
> 
> local operation, it is expected to be very rare in the normal execution 
> path.
> 
> Could we move this check into the existing `WR_LOCAL_OP_MASK` handling 
> path, or at least mark
> 
> the condition with `unlikely()`?
> 
> I would prefer moving it into the local operation handling path, if the 
> control flow allows it, so that normal wire
> 
> operations do not pay the cost of an additional state check.
> 
> This would keep the fix while minimizing its impact on the common fast path.

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 12d03f390b09..8bd1f9b1ce06 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -707,6 +707,13 @@ int rxe_requester(struct rxe_qp *qp)
 	}
 
 	if (wqe->mask & WR_LOCAL_OP_MASK) {
+		/* Retry rewinds wqe_index, skip it if already done */
+		if (unlikely(wqe->state == wqe_state_done)) {
+			qp->req.wqe_index = queue_next_index(q,
+						qp->req.wqe_index);
+			goto done;
+		}
+
 		err = rxe_do_local_ops(qp, wqe);
 		if (unlikely(err))
 			goto err;

Does something like the diff above work?

Also, sashiko marked a high severity issue [1] in this version of the
patch. I am not familiar with this codebase enough to know if this is a
real issue introduced by this change or not.

Is the issue reported by sashiko real?
If yes, is it introduced by this patch?

1- https://lore.kernel.org/all/sashiko-outbox-213867@kernel.org/

Thanks,
Mohamed Khalfella