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: <1e9a8c0f-b4c3-3edf-33f1-33a2b7ca245a@gmail.com>
Date:   Sun, 26 Jun 2022 16:51:22 -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 1/2] RDMA/rxe: Update wqe_index for each wqe error
 completion
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-2-lizhijian@fujitsu.com>
From:   Bob Pearson <rpearsonhpe@gmail.com>
In-Reply-To: <20220516015329.445474-2-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:
> Previously, if user space keeps sending abnormal wqe, queue.prod will
> keep increasing while queue.index doesn't. Once
> queue.index==queue.prod in next round, req_next_wqe() will treat queue
> as empty. In such case, no new completion would be generated.
> 
> Update wqe_index for each wqe completion so that req_next_wqe() can get
> next wqe properly.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_req.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index a0d5e57f73c1..8bdd0b6b578f 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -773,6 +773,8 @@ int rxe_requester(void *arg)
>  	if (ah)
>  		rxe_put(ah);
>  err:
> +	/* update wqe_index for each wqe completion */
> +	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
>  	wqe->state = wqe_state_err
>  	__rxe_do_task(&qp->comp.task);
>  

This change looks plausible, but I am not sure if it will make a difference since the qp
will get transitioned to the error state very shortly.

In order for it to matter the requester must be a ways ahead of the completer in the send queue
and someone be actively posting new wqes which will reschedule the requester. Currently it
will fail on the same wqe again unless the error described above occurs but if we post a new valid
wqe it will get executed even though we have detected an error that should have stopped the qp.

It looks like the intent was to keep the qp in the non error state until all the old
wqes get completed before making the transition. But we should disable the requester
from processing new wqes in this case. That seems like a safer solution to the problem.

Bob