Re: [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe

[Leon Romanovsky <leon@kernel.org>]

On Thu, Mar 31, 2022 at 08:02:45PM +0800, Xiao Yang wrote:
> Current rxe_requester() doesn't generate a completion on error after
> getting a wqe. Fix the issue by calling rxe_completer() on error.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_req.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index ae5fbc79dd5c..01ae400e5481 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -648,26 +648,24 @@ int rxe_requester(void *arg)
>  		psn_compare(qp->req.psn, (qp->comp.psn +
>  				RXE_MAX_UNACKED_PSNS)) > 0)) {
>  		qp->req.wait_psn = 1;
> -		goto exit;
> +		goto qp_op_err;
>  	}
>  
>  	/* Limit the number of inflight SKBs per QP */
>  	if (unlikely(atomic_read(&qp->skb_out) >
>  		     RXE_INFLIGHT_SKBS_PER_QP_HIGH)) {
>  		qp->need_req_skb = 1;
> -		goto exit;
> +		goto qp_op_err;
>  	}
>  
>  	opcode = next_opcode(qp, wqe, wqe->wr.opcode);
> -	if (unlikely(opcode < 0)) {
> -		wqe->status = IB_WC_LOC_QP_OP_ERR;
> -		goto exit;
> -	}
> +	if (unlikely(opcode < 0))
> +		goto qp_op_err;
>  
>  	mask = rxe_opcode[opcode].mask;
>  	if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
>  		if (check_init_depth(qp, wqe))
> -			goto exit;
> +			goto qp_op_err;
>  	}
>  
>  	mtu = get_mtu(qp);
> @@ -706,26 +704,26 @@ int rxe_requester(void *arg)
>  	av = rxe_get_av(&pkt, &ah);
>  	if (unlikely(!av)) {
>  		pr_err("qp#%d Failed no address vector\n", qp_num(qp));
> -		wqe->status = IB_WC_LOC_QP_OP_ERR;
>  		goto err_drop_ah;
>  	}
>  
>  	skb = init_req_packet(qp, av, wqe, opcode, payload, &pkt);
>  	if (unlikely(!skb)) {
>  		pr_err("qp#%d Failed allocating skb\n", qp_num(qp));
> -		wqe->status = IB_WC_LOC_QP_OP_ERR;
>  		goto err_drop_ah;
>  	}
>  
>  	ret = finish_packet(qp, av, wqe, &pkt, skb, payload);
>  	if (unlikely(ret)) {
>  		pr_debug("qp#%d Error during finish packet\n", qp_num(qp));
> +		if (ah)

No, ah can't be NULL. This is why I proposed to clean rxe_get_av() too.
**ahp is not NULL, as an input to that function and in all flows it is
updated with new ah pointer. If it can't update, the NULL will be
returned and rxe_requester() will exit.

Thanks

> +			rxe_put(ah);
>  		if (ret == -EFAULT)
>  			wqe->status = IB_WC_LOC_PROT_ERR;
>  		else
>  			wqe->status = IB_WC_LOC_QP_OP_ERR;
>  		kfree_skb(skb);
> -		goto err_drop_ah;
> +		goto err;
>  	}
>  
>  	if (ah)
> @@ -751,8 +749,7 @@ int rxe_requester(void *arg)
>  			goto exit;
>  		}
>  
> -		wqe->status = IB_WC_LOC_QP_OP_ERR;
> -		goto err;
> +		goto qp_op_err;
>  	}
>  
>  	update_state(qp, wqe, &pkt);
> @@ -762,6 +759,9 @@ int rxe_requester(void *arg)
>  err_drop_ah:
>  	if (ah)
>  		rxe_put(ah);
> +
> +qp_op_err:
> +	wqe->status = IB_WC_LOC_QP_OP_ERR;
>  err:
>  	wqe->state = wqe_state_error;
>  	__rxe_do_task(&qp->comp.task);
> -- 
> 2.25.4
> 
> 
>