Re: [PATCH for-rc] RDMA/rxe: Fix rnr retry behavior

[Yanjun Zhu <yanjun.zhu@linux.dev>]

在 2022/5/13 3:49, Bob Pearson 写道:
> Currently the completer tasklet when it sets the retransmit timer or the
> nak timer sets the same flag (qp->req.need_retry) so that if either
> timer fires it will attempt to perform a retry flow on the send queue.
> This has the effect of responding to an RNR NAK at the first retransmit
> timer event which does not allow for the requested rnr timeout.
> 
> This patch adds a new flag (qp->req.need_rnr_timeout) which, if set,
> prevents a retry flow until the rnr nak timer fires.
> 
> This patch fixes rnr retry errors which can be observed by running the
> pyverbs test suite 50-100X. With this patch applied they do not occur.

Do you mean that running run_tests.py for 50-100times can reproduce this 
bug? I want to reproduce this problem.

Thanks a lot.
Zhu Yanjun

> 
> Link: https://lore.kernel.org/linux-rdma/a8287823-1408-4273-bc22-99a0678db640@gmail.com/
> Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_comp.c  | 4 +---
>   drivers/infiniband/sw/rxe/rxe_qp.c    | 1 +
>   drivers/infiniband/sw/rxe/rxe_req.c   | 6 ++++--
>   drivers/infiniband/sw/rxe/rxe_verbs.h | 1 +
>   4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 138b3e7d3a5f..bc668cb211b1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -733,9 +733,7 @@ int rxe_completer(void *arg)
>   				if (qp->comp.rnr_retry != 7)
>   					qp->comp.rnr_retry--;
>   
> -				qp->req.need_retry = 1;
> -				pr_debug("qp#%d set rnr nak timer\n",
> -					 qp_num(qp));
> +				qp->req.need_rnr_timeout = 1;
>   				mod_timer(&qp->rnr_nak_timer,
>   					  jiffies + rnrnak_jiffies(aeth_syn(pkt)
>   						& ~AETH_TYPE_MASK));
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index 62acf890af6c..1c962468714e 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -513,6 +513,7 @@ static void rxe_qp_reset(struct rxe_qp *qp)
>   	atomic_set(&qp->ssn, 0);
>   	qp->req.opcode = -1;
>   	qp->req.need_retry = 0;
> +	qp->req.need_rnr_timeout = 0;
>   	qp->req.noack_pkts = 0;
>   	qp->resp.msn = 0;
>   	qp->resp.opcode = -1;
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index ae5fbc79dd5c..770ae4279f73 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -103,7 +103,8 @@ void rnr_nak_timer(struct timer_list *t)
>   {
>   	struct rxe_qp *qp = from_timer(qp, t, rnr_nak_timer);
>   
> -	pr_debug("qp#%d rnr nak timer fired\n", qp_num(qp));
> +	qp->req.need_retry = 1;
> +	qp->req.need_rnr_timeout = 0;
>   	rxe_run_task(&qp->req.task, 1);
>   }
>   
> @@ -624,10 +625,11 @@ int rxe_requester(void *arg)
>   		qp->req.need_rd_atomic = 0;
>   		qp->req.wait_psn = 0;
>   		qp->req.need_retry = 0;
> +		qp->req.need_rnr_timeout = 0;
>   		goto exit;
>   	}
>   
> -	if (unlikely(qp->req.need_retry)) {
> +	if (unlikely(qp->req.need_retry && !qp->req.need_rnr_timeout)) {
>   		req_retry(qp);
>   		qp->req.need_retry = 0;
>   	}
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
> index e7eff1ca75e9..ab3186478c3f 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.h
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
> @@ -123,6 +123,7 @@ struct rxe_req_info {
>   	int			need_rd_atomic;
>   	int			wait_psn;
>   	int			need_retry;
> +	int			need_rnr_timeout;
>   	int			noack_pkts;
>   	struct rxe_task		task;
>   };
> 
> base-commit: c5eb0a61238dd6faf37f58c9ce61c9980aaffd7a