Re: [PATCH] RDMA/rxe: Fix incomplete state save in rxe_requester

[Bob Pearson <rpearsonhpe@gmail.com>]

On 7/21/23 13:04, Bob Pearson wrote:
> If a send packet is dropped by the IP layer in rxe_requester()
> the call to rxe_xmit_packet() can fail with err == -EAGAIN.
> To recover, the state of the wqe is restored to the state before
> the packet was sent so it can be resent. However, the routines
> that save and restore the state miss a significnt part of the
> variable state in the wqe, the dma struct which is used to process
> through the sge table. And, the state is not saved before the packet
> is built which modifies the dma struct.
> 
> Under heavy stress testing with many QPs on a fast node sending
> large messages to a slow node dropped packets are observed and
> the resent packets are corrupted because the dma struct was not
> restored. This patch fixes this behavior and allows the test cases
> to succeed.
> 
> Fixes: 3050b9985024 ("IB/rxe: Fix race condition between requester and completer")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_req.c | 47 ++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 76d85731a45f..10ff2715d9bb 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -708,10 +708,11 @@ static void save_state(struct rxe_send_wqe *wqe,
>  		       struct rxe_send_wqe *rollback_wqe,
>  		       u32 *rollback_psn)
>  {
> -	rollback_wqe->state     = wqe->state;
> +	rollback_wqe->state = wqe->state;
>  	rollback_wqe->first_psn = wqe->first_psn;
> -	rollback_wqe->last_psn  = wqe->last_psn;
> -	*rollback_psn		= qp->req.psn;
> +	rollback_wqe->last_psn = wqe->last_psn;
> +	rollback_wqe->dma = wqe->dma;
> +	*rollback_psn = qp->req.psn;
>  }
>  
>  static void rollback_state(struct rxe_send_wqe *wqe,
> @@ -719,10 +720,11 @@ static void rollback_state(struct rxe_send_wqe *wqe,
>  			   struct rxe_send_wqe *rollback_wqe,
>  			   u32 rollback_psn)
>  {
> -	wqe->state     = rollback_wqe->state;
> +	wqe->state = rollback_wqe->state;
>  	wqe->first_psn = rollback_wqe->first_psn;
> -	wqe->last_psn  = rollback_wqe->last_psn;
> -	qp->req.psn    = rollback_psn;
> +	wqe->last_psn = rollback_wqe->last_psn;
> +	wqe->dma = rollback_wqe->dma;
> +	qp->req.psn = rollback_psn;
>  }
>  
>  static void update_state(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
> @@ -922,6 +924,9 @@ int rxe_requester(struct rxe_qp *qp)
>  		payload = mtu;
>  	}
>  
> +	/* save wqe state before we build and send packet */
> +	save_state(wqe, qp, &rollback_wqe, &rollback_psn);
> +
>  	skb = rxe_init_req_packet(qp, wqe, opcode, payload, &pkt);
>  	if (unlikely(!skb)) {
>  		rxe_err_qp(qp, "failed to create packet for %s",
> @@ -929,29 +934,29 @@ int rxe_requester(struct rxe_qp *qp)
>  		goto err;
>  	}
>  
> -	/*
> -	 * To prevent a race on wqe access between requester and completer,
> -	 * wqe members state and psn need to be set before calling
> -	 * rxe_xmit_packet().
> -	 * Otherwise, completer might initiate an unjustified retry flow.
> -	 */
> -	save_state(wqe, qp, &rollback_wqe, &rollback_psn);
> +	/* update wqe state as though we had sent it */
>  	update_wqe_state(qp, wqe, &pkt);
>  	update_wqe_psn(qp, wqe, &pkt, payload);
>  
>  	err = rxe_xmit_packet(qp, &pkt, skb);
>  	if (err) {
> -		qp->need_req_skb = 1;
> +		if (err != -EAGAIN) {
> +			wqe->status = IB_WC_LOC_QP_OP_ERR;
> +			goto err;
> +		}
>  
> +		/* the packet was dropped so reset wqe to the state
> +		 * before we sent it so we can try to resend
> +		 */
>  		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
>  
> -		if (err == -EAGAIN) {
> -			rxe_sched_task(&qp->req.task);
> -			goto exit;
> -		}
> +		/* force a delay until the dropped packet is freed and
> +		 * the send queue is drained below the low water mark
> +		 */
> +		qp->need_req_skb = 1;
>  
> -		wqe->status = IB_WC_LOC_QP_OP_ERR;
> -		goto err;
> +		rxe_sched_task(&qp->req.task);
> +		goto exit;
>  	}
>  
>  	update_state(qp, &pkt);

Never mind. Missing some prereq patches.