[resend] rxe: fix broken receive queue draining

Commit Message

Andrew Boyer July 19, 2017, 3:56 p.m. UTC

To make this work, it also needs to handle the case where a pkt is on the
req_pkts list but the QP is no longer valid.



You could also combine the two checks into one conditional.

For that matter, what is the Œnotify¹ argument supposed to do? I would
think Œnotify == true¹ would mean advancing the consumer pointer, not the
other way around.

-Andrew

On 7/11/17, 6:42 PM, "[e-mail] on behalf of Bart
Van Assche" <[e-mail] on behalf of
[e-mail]> wrote:

>On Tue, 2017-06-27 at 12:19 +0300, Sagi Grimberg wrote:
>> From: Vijay Immanuel <[e-mail]>
>> 
>> If we modified the qp to ERROR state, and
>> drained the recieve queue, post_recv must
>              ^^^^^^^
>              receive?
>> trigger the responder task to complete
>> the drain work request.
>> 
>> Cc: Bart Van Assche <[e-mail]>
>> Signed-off-by: Vijay Immanuel <[e-mail]>
>> Signed-off-by: Sagi Grimberg <[e-mail]>
>> ---
>>  drivers/infiniband/sw/rxe/rxe_resp.c  | 3 +++
>>  drivers/infiniband/sw/rxe/rxe_verbs.c | 3 +++
>>  2 files changed, 6 insertions(+)
>> 
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c
>>b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index 23039768f541..f66b48c114ac 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -1217,6 +1217,9 @@ void rxe_drain_req_pkts(struct rxe_qp *qp, bool
>>notify)
>>  		kfree_skb(skb);
>>  	}
>>  
>> +	if (notify)
>> +		return;
>> +
>>  	while (!qp->srq && qp->rq.queue && queue_head(qp->rq.queue))
>>  		advance_consumer(qp->rq.queue);
>
>Since an "if (...) return" statement is added, please consider moving the
>qp->srq test out of the while-loop and into the if-expression. Anyway:
>
>Reviewed-by: Bart Van Assche <[e-mail]>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to [e-mail]
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Doug Ledford July 22, 2017, 6:14 p.m. UTC | #1

On 7/19/2017 11:56 AM, Boyer, Andrew wrote:
> To make this work, it also needs to handle the case where a pkt is on the
> req_pkts list but the QP is no longer valid.
> 
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -594,8 +594,10 @@ int rxe_requester(void *arg)
>  	rxe_add_ref(qp);
>  
>  next_wqe:
> -	if (unlikely(!qp->valid))
> +	if (unlikely(!qp->valid)) {
> +		rxe_drain_req_pkts(qp, true);
>  		goto exit;
> +	}
>  
>  	if (unlikely(qp->req.state == QP_STATE_ERROR)) {
>  		rxe_drain_req_pkts(qp, true);
> 
> 
> You could also combine the two checks into one conditional.
> 
> For that matter, what is the Œnotify¹ argument supposed to do? I would
> think Œnotify == true¹ would mean advancing the consumer pointer, not the
> other way around.

This will have to be done in a follow up patch.  I had already accepted
the original patch before you made this review comment.

Patch

--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -594,8 +594,10 @@  int rxe_requester(void *arg)
 	rxe_add_ref(qp);
 
 next_wqe:
-	if (unlikely(!qp->valid))
+	if (unlikely(!qp->valid)) {
+		rxe_drain_req_pkts(qp, true);
 		goto exit;
+	}
 
 	if (unlikely(qp->req.state == QP_STATE_ERROR)) {
 		rxe_drain_req_pkts(qp, true);