Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <40582262-9d58-b38a-5a0e-7c32d1efadbe@gmail.com>
Date:   Sun, 26 Jun 2022 17:42:11 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v3 2/2] RDMA/rxe: Generate error completion for error
 requester QP state
Content-Language: en-US
To:     Li Zhijian <lizhijian@fujitsu.com>,
        Yanjun Zhu <yanjun.zhu@linux.dev>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Haakon Bugge <haakon.bugge@oracle.com>,
        Cheng Xu <chengyou@linux.alibaba.com>,
        linux-rdma@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org
References: <20220516015329.445474-1-lizhijian@fujitsu.com>
 <20220516015329.445474-3-lizhijian@fujitsu.com>
From:   Bob Pearson <rpearsonhpe@gmail.com>
In-Reply-To: <20220516015329.445474-3-lizhijian@fujitsu.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/15/22 20:53, Li Zhijian wrote:
> As per IBTA specification, all subsequent WQEs while QP is in error
> state should be completed with a flush error.
> 
> Here we check QP_STATE_ERROR after req_next_wqe() so that rxe_completer()
> has chance to be called where it will set CQ state to FLUSH ERROR and the
> completion can associate with its WQE.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V3: unlikely() optimization # Cheng Xu <chengyou@linux.alibaba.com>
>     update commit log # Haakon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_req.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 8bdd0b6b578f..c1f1c19f26b2 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -624,7 +624,7 @@ int rxe_requester(void *arg)
>  	rxe_get(qp);
>  
>  next_wqe:
> -	if (unlikely(!qp->valid || qp->req.state == QP_STATE_ERROR))
> +	if (unlikely(!qp->valid))
>  		goto exit;
>  
>  	if (unlikely(qp->req.state == QP_STATE_RESET)) {
> @@ -646,6 +646,14 @@ int rxe_requester(void *arg)
>  	if (unlikely(!wqe))
>  		goto exit;
>  
> +	if (unlikely(qp->req.state == QP_STATE_ERROR)) {
> +		/*
> +		 * Generate an error completion so that user space is able to
> +		 * poll this completion.
> +		 */
> +		goto err;
> +	}
> +
>  	if (wqe->mask & WR_LOCAL_OP_MASK) {
>  		ret = rxe_do_local_ops(qp, wqe);
>  		if (unlikely(ret))

There may be issues with moving this after the retry check since the retransmit timer can
fire at any time and may race with the completer setting the error state and result in
a retry flow occurring while you are trying to flush out all the wqes. Perhaps better
to to duplicate setting wqe in the error state check.

Bob