diff mbox series

[v2,2/2] RDMA/rxe: Generate error completion for error requester state

Message ID 20220511023030.229212-3-lizhijian@fujitsu.com (mailing list archive)
State Superseded
Headers show
Series RDMA/rxe: Fix no completion event issue | expand

Commit Message

Zhijian Li (Fujitsu) May 11, 2022, 2:30 a.m. UTC
SoftRoCE always returns success when user space is posting a new wqe where
it usually just enqueues a wqe.

Once the requester state becomes QP_STATE_ERROR, we should generate error
completion for all subsequent wqe. So the user is able to poll the
completion event to check if the former wqe is handled correctly.

Here we check QP_STATE_ERROR after req_next_wqe() so that the completion
can associate with its wqe.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/infiniband/sw/rxe/rxe_req.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Cheng Xu May 11, 2022, 3:44 a.m. UTC | #1
On 5/11/22 10:30 AM, Li Zhijian wrote:
> SoftRoCE always returns success when user space is posting a new wqe where
> it usually just enqueues a wqe.
> 
> Once the requester state becomes QP_STATE_ERROR, we should generate error
> completion for all subsequent wqe. So the user is able to poll the
> completion event to check if the former wqe is handled correctly.
> 
> Here we check QP_STATE_ERROR after req_next_wqe() so that the completion
> can associate with its wqe.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.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..ed6a486c4343 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 (qp->req.state == QP_STATE_ERROR) {
> +		/*
> +		 * Generate an error completion so that user space is able to
> +		 * poll this completion.
> +		 */
> +		goto err;
> +	}
> +

Should this still use unlikely(...) ? Because the original judgement has
a unlikely surrounded.

Cheng Xu

>   	if (wqe->mask & WR_LOCAL_OP_MASK) {
>   		ret = rxe_do_local_ops(qp, wqe);
>   		if (unlikely(ret))
Zhijian Li (Fujitsu) May 11, 2022, 8:36 a.m. UTC | #2
On 11/05/2022 11:44, Cheng Xu wrote:
>
>
> On 5/11/22 10:30 AM, Li Zhijian wrote:
>> SoftRoCE always returns success when user space is posting a new wqe where
>> it usually just enqueues a wqe.
>>
>> Once the requester state becomes QP_STATE_ERROR, we should generate error
>> completion for all subsequent wqe. So the user is able to poll the
>> completion event to check if the former wqe is handled correctly.
>>
>> Here we check QP_STATE_ERROR after req_next_wqe() so that the completion
>> can associate with its wqe.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.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..ed6a486c4343 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 (qp->req.state == QP_STATE_ERROR) {
>> +        /*
>> +         * Generate an error completion so that user space is able to
>> +         * poll this completion.
>> +         */
>> +        goto err;
>> +    }
>> +
>
> Should this still use unlikely(...) ? Because the original judgement has
> a unlikely surrounded.

Good catch. it sounds good :)


Thanks
Zhijian



>
> Cheng Xu
>
>>       if (wqe->mask & WR_LOCAL_OP_MASK) {
>>           ret = rxe_do_local_ops(qp, wqe);
>>           if (unlikely(ret))
Haakon Bugge May 11, 2022, 12:50 p.m. UTC | #3
> On 11 May 2022, at 10:36, lizhijian@fujitsu.com wrote:
> 
> 
> 
> On 11/05/2022 11:44, Cheng Xu wrote:
>> 
>> 
>> On 5/11/22 10:30 AM, Li Zhijian wrote:
>>> SoftRoCE always returns success when user space is posting a new wqe where
>>> it usually just enqueues a wqe.
>>> 
>>> Once the requester state becomes QP_STATE_ERROR, we should generate error
>>> completion for all subsequent wqe. So the user is able to poll the
>>> completion event to check if the former wqe is handled correctly.

This is not correct. You shall be able to post new send work requests. They shall be completed with FLUSHED_IN_ERROR. As per IBTA C10-42:

Work Requests subsequent to that which caused the Completion Error leading to the transition into the Error state, including those submitted after the transition, must return the Flush Error completion status through the Completion Queue.


Thxs, Håkon

>>> 
>>> Here we check QP_STATE_ERROR after req_next_wqe() so that the completion
>>> can associate with its wqe.
>>> 
>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.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..ed6a486c4343 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 (qp->req.state == QP_STATE_ERROR) {
>>> +        /*
>>> +         * Generate an error completion so that user space is able to
>>> +         * poll this completion.
>>> +         */
>>> +        goto err;
>>> +    }
>>> +
>> 
>> Should this still use unlikely(...) ? Because the original judgement has
>> a unlikely surrounded.
> 
> Good catch. it sounds good :)
> 
> 
> Thanks
> Zhijian
> 
> 
> 
>> 
>> Cheng Xu
>> 
>>>       if (wqe->mask & WR_LOCAL_OP_MASK) {
>>>           ret = rxe_do_local_ops(qp, wqe);
>>>           if (unlikely(ret))
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 8bdd0b6b578f..ed6a486c4343 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 (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))