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 3 potential
issues to consider:
- [High] RDMA/rxe: commit message is a raw email quote lacking
  Signed-off-by and Fixes
- [High] RDMA/rxe: send queue deadlock in rxe_requester() on fenced
  local op retry
- [High] RDMA/rxe: rxe_completer() consumer index can overtake wqe_index
  on retry
--

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

> RDMA/rxe: Do not re-execute completed local ops after a retry
> 
> 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.

[Severity: High]
The commit message is a raw quoted email reply containing the v1 diff and
review discussion, and its Signed-off-by: and Fixes: trailers are buried
inside quoted text where git and kernel tooling ignore them.  Please replace
the body with an unquoted changelog ending with unquoted Fixes: 001345339f4c
("RDMA/rxe: Separate HW and SW l/rkeys") and Signed-off-by: trailers.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 6e01fb18893bf..25e4bd2bc51ab 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -787,6 +787,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)) {

[Severity: High]
Can checking wqe->state == wqe_state_done after rxe_wqe_is_fenced() deadlock
the send queue on retry when a completed local op has IB_SEND_FENCE?

Suppose the send queue has WQE 0 (SEND), WQE 1 (REG_MR or BIND_MW with
IB_SEND_FENCE), and WQE 2 (RDMA READ or atomic).  On the initial pass, WQE 1
executes and enters wqe_state_done, and WQE 2 calls check_init_depth(), which
decrements qp->req.rd_atomic and sets wqe->has_rd_atomic = 1.  If a retry
then rewinds qp->req.wqe_index to 0, req_retry() resets WQE 0 and WQE 2 to
wqe_state_posted while leaving WQE 2 holding its rd_atomic credit.

After WQE 0 is resent, rxe_requester() visits WQE 1 and evaluates
rxe_wqe_is_fenced() before reaching WR_LOCAL_OP_MASK:

drivers/infiniband/sw/rxe/rxe_req.c:rxe_wqe_is_fenced() {
	return (wqe->wr.send_flags & IB_SEND_FENCE) &&
		atomic_read(&qp->req.rd_atomic) != qp->attr.max_rd_atomic;
}

Because WQE 2 still holds an rd_atomic credit, rxe_wqe_is_fenced() sets
qp->req.wait_fence = 1 and exits.  qp->req.wqe_index stays stuck at WQE 1
waiting for WQE 2's credit, while WQE 2 sits at index 2 in wqe_state_posted
and can never be resent to release that credit.  Similarly, if the QP is in
IB_QPS_SQD, req_next_wqe() returns NULL because WQE 1 is not in
wqe_state_processing, preventing the drain from advancing past WQE 1.

> +			qp->req.wqe_index = queue_next_index(q,
> +						qp->req.wqe_index);
> +			goto done;
> +		}

[Severity: High]
Can the send queue consumer index in rxe_completer() overtake
qp->req.wqe_index after req_retry() rewinds qp->req.wqe_index across
completed local ops?

rxe_sender() alternates between rxe_requester() and rxe_completer().  Each
call to rxe_requester() processes or skips only one WQE before jumping to
done, whereas rxe_completer() loops through COMPST_GET_WQE -> COMPST_COMP_WQE
and drains all consecutive wqe_state_done WQEs at the consumer head in one
call:

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;
    ...
}

For example, if WQE 0 (SEND, PSN 100) is followed by WQE 1 (REG_MR,
wqe_state_done) and WQE 2 (SEND, PSN 101), req_retry() rewinds
qp->req.wqe_index to 0.  rxe_requester() resends WQE 0, advances
qp->req.wqe_index to 1, and returns.  If rxe_completer() then processes a
cumulative ACK or PSN sequence error NAK for PSN 101, it completes WQE 0
(consumer = 1) and immediately loops in the same invocation to complete WQE 1
because WQE 1 is in wqe_state_done, advancing the consumer index to 2 while
qp->req.wqe_index is still 1.  The same overtake happens without a cumulative
ACK whenever two completed local ops (such as LOCAL_INV + REG_MR) sit back to
back after WQE 0 completes: rxe_requester() skips WQE 1 and returns with
qp->req.wqe_index at 2, and rxe_completer() completes both WQE 1 and WQE 2,
advancing the consumer to 3.

Once the consumer index is ahead of qp->req.wqe_index, the slots in
[qp->req.wqe_index, consumer) are freed for producer reuse.  If the producer
fills the ring up to qp->req.wqe_index, __req_next_wqe() sees index == prod
and returns NULL on a full queue, stalling the QP; if the producer wraps past
qp->req.wqe_index, rxe_requester() executes the newly posted WQE out of order
and then a second time when qp->req.wqe_index wraps around.

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