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

[Zhu Yanjun]

在 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@xxxxxxxxxxxxxxx
> Signed-off-by: Mohamed Khalfella <mkhalfella@xxxxxxxxxxxxxxx>
> ---
>   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.

Thanks a lot.

Yanjun.Zhu

> +
>   	if (rxe_wqe_is_fenced(qp, wqe)) {
>   		qp->req.wait_fence = 1;
>   		goto exit;

--
Best Regards,
Yanjun.Zhu