[PATCH resend] rxe: fix broken receive queue draining

[PATCH resend] rxe: fix broken receive queue draining

[Sagi Grimberg]

From: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>

If we modified the qp to ERROR state, and
drained the recieve queue, post_recv must
trigger the responder task to complete
the drain work request.

Cc: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 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);
 }
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e74dfb..94af4d67988b 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -919,6 +919,9 @@ static int rxe_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 
 	spin_unlock_irqrestore(&rq->producer_lock, flags);
 
+	if (qp->resp.state == QP_STATE_ERROR)
+		rxe_run_task(&qp->resp.task, 1);
+
 err1:
 	return err;
 }
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend] rxe: fix broken receive queue draining

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Tue, Jun 27, 2017 at 12:19:38PM +0300, Sagi Grimberg wrote:
> From: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
>
> If we modified the qp to ERROR state, and
> drained the recieve queue, post_recv must
> trigger the responder task to complete
> the drain work request.
>
> Cc: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  drivers/infiniband/sw/rxe/rxe_resp.c  | 3 +++
>  drivers/infiniband/sw/rxe/rxe_verbs.c | 3 +++
>  2 files changed, 6 insertions(+)
>
It looks OK for me.

Moni ?

> 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);
>  }
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 83d709e74dfb..94af4d67988b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -919,6 +919,9 @@ static int rxe_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
>
>  	spin_unlock_irqrestore(&rq->producer_lock, flags);
>
> +	if (qp->resp.state == QP_STATE_ERROR)
> +		rxe_run_task(&qp->resp.task, 1);
> +
>  err1:
>  	return err;
>  }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH resend] rxe: fix broken receive queue draining

[Bart Van Assche]

On Tue, 2017-06-27 at 12:19 +0300, Sagi Grimberg wrote:
> From: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
> 
> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  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 <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend] rxe: fix broken receive queue draining

[Boyer, Andrew]

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.

-Andrew

On 7/11/17, 6:42 PM, "linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org on behalf of Bart
Van Assche" <linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org on behalf of
Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org> wrote:

>On Tue, 2017-06-27 at 12:19 +0300, Sagi Grimberg wrote:
>> From: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
>> 
>> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
>> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>> ---
>>  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 <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend] rxe: fix broken receive queue draining

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1138 bytes --]

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.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH resend] rxe: fix broken receive queue draining

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 770 bytes --]

On 6/27/2017 5:19 AM, Sagi Grimberg wrote:
> From: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
> 
> If we modified the qp to ERROR state, and
> drained the recieve queue, post_recv must
> trigger the responder task to complete
> the drain work request.
> 
> Cc: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vijay Immanuel <vijayi-R1PCaG5FPJG/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

This was accepted into 4.13-rc, thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]