[PATCH] IB/core: Don't drain the receive queue for srq attached queue-pair

[PATCH] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

SRQ attached QPs will fail receive work posts does not
have a receive queue that needs to be drained, the receive
queue is shared.

Fixes: 765d67748bcf ("IB: new common API for draining queues")
Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 769b000e8360..566bfb31cadb 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1897,6 +1897,7 @@ EXPORT_SYMBOL(ib_drain_rq);
 void ib_drain_qp(struct ib_qp *qp)
 {
 	ib_drain_sq(qp);
-	ib_drain_rq(qp);
+	if (!qp->srq)
+		ib_drain_rq(qp);
 }
 EXPORT_SYMBOL(ib_drain_qp);
-- 
1.9.1

--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Steve Wise]

On 4/26/2016 9:55 AM, Sagi Grimberg wrote:
> SRQ attached QPs will fail receive work posts does not
> have a receive queue that needs to be drained, the receive
> queue is shared.

NIT: I can't quite parse that sentence.

Maybe something like:

SRQ attached QPs don't have a private RQ and don't need to be drained.



>
> Fixes: 765d67748bcf ("IB: new common API for draining queues")
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>   drivers/infiniband/core/verbs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 769b000e8360..566bfb31cadb 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1897,6 +1897,7 @@ EXPORT_SYMBOL(ib_drain_rq);
>   void ib_drain_qp(struct ib_qp *qp)
>   {
>   	ib_drain_sq(qp);
> -	ib_drain_rq(qp);
> +	if (!qp->srq)
> +		ib_drain_rq(qp);
>   }
>   EXPORT_SYMBOL(ib_drain_qp);

Good catch!

Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Christoph Hellwig]

Just curious: what takes care of draining SRQs when we unregister them
after all queues are gone?
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Chuck Lever]

> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> 
> Just curious: what takes care of draining SRQs when we unregister them
> after all queues are gone?

Conversely, I would think that a consumer that uses SRQ
would want a similar drain mechanism to guarantee there
are no more posted Receives for the associated QP, and
thus it is safe to destroy it.


--
Chuck Lever



--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Steve Wise]

On 4/26/2016 10:55 AM, Chuck Lever wrote:
>
>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>
>> Just curious: what takes care of draining SRQs when we unregister them
>> after all queues are gone?
>
> Conversely, I would think that a consumer that uses SRQ
> would want a similar drain mechanism to guarantee there
> are no more posted Receives for the associated QP, and
> thus it is safe to destroy it.

I don't think there is anything now that handles draining an SRQ, nor 
ensuring a QP's RQEs are completed/consumed from its SRQ when draining 
the QP.  Sounds like work is needed here.

But are there any kernel SRQ consumers at this point?

Steve.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Bart Van Assche]

On 04/26/2016 09:00 AM, Steve Wise wrote:
> On 4/26/2016 10:55 AM, Chuck Lever wrote:
>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>>
>>> Just curious: what takes care of draining SRQs when we unregister them
>>> after all queues are gone?
>>
>> Conversely, I would think that a consumer that uses SRQ
>> would want a similar drain mechanism to guarantee there
>> are no more posted Receives for the associated QP, and
>> thus it is safe to destroy it.
> 
> I don't think there is anything now that handles draining an SRQ, nor 
> ensuring a QP's RQEs are completed/consumed from its SRQ when draining 
> the QP.  Sounds like work is needed here.
> 
> But are there any kernel SRQ consumers at this point?

I think the following two:

$ git grep -nHE '(ib|rdma)_create_srq' drivers/infiniband/ulp
drivers/infiniband/ulp/ipoib/ipoib_cm.c:1521:	priv->cm.srq = ib_create_srq(priv->pd, &srq_init_attr);
drivers/infiniband/ulp/srpt/ib_srpt.c:2723:	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);

Bart.

--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Steve Wise]

On 4/26/2016 11:04 AM, Bart Van Assche wrote:
> On 04/26/2016 09:00 AM, Steve Wise wrote:
>> On 4/26/2016 10:55 AM, Chuck Lever wrote:
>>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>>>
>>>> Just curious: what takes care of draining SRQs when we unregister them
>>>> after all queues are gone?
>>>
>>> Conversely, I would think that a consumer that uses SRQ
>>> would want a similar drain mechanism to guarantee there
>>> are no more posted Receives for the associated QP, and
>>> thus it is safe to destroy it.
>>
>> I don't think there is anything now that handles draining an SRQ, nor
>> ensuring a QP's RQEs are completed/consumed from its SRQ when draining
>> the QP.  Sounds like work is needed here.
>>
>> But are there any kernel SRQ consumers at this point?
>
> I think the following two:
>
> $ git grep -nHE '(ib|rdma)_create_srq' drivers/infiniband/ulp
> drivers/infiniband/ulp/ipoib/ipoib_cm.c:1521:	priv->cm.srq = ib_create_srq(priv->pd, &srq_init_attr);
> drivers/infiniband/ulp/srpt/ib_srpt.c:2723:	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);
>
> Bart.

If we make the requirement that the ib_drain_rq() caller must consume 
all completions for all QPs attached to an SRQ if they are outstanding, 
then I think we can modify ib_drain_rq() to post the drain recv WR to 
the SRQ.  It should work, right?

Steve



--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Bart Van Assche]

On 04/26/2016 09:28 AM, Steve Wise wrote:
> On 4/26/2016 11:04 AM, Bart Van Assche wrote:
>> On 04/26/2016 09:00 AM, Steve Wise wrote:
>>> On 4/26/2016 10:55 AM, Chuck Lever wrote:
>>>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>>>>
>>>>> Just curious: what takes care of draining SRQs when we unregister them
>>>>> after all queues are gone?
>>>>
>>>> Conversely, I would think that a consumer that uses SRQ
>>>> would want a similar drain mechanism to guarantee there
>>>> are no more posted Receives for the associated QP, and
>>>> thus it is safe to destroy it.
>>>
>>> I don't think there is anything now that handles draining an SRQ, nor
>>> ensuring a QP's RQEs are completed/consumed from its SRQ when draining
>>> the QP.  Sounds like work is needed here.
>>>
>>> But are there any kernel SRQ consumers at this point?
>>
>> I think the following two:
>>
>> $ git grep -nHE '(ib|rdma)_create_srq' drivers/infiniband/ulp
>> drivers/infiniband/ulp/ipoib/ipoib_cm.c:1521:    priv->cm.srq =
>> ib_create_srq(priv->pd, &srq_init_attr);
>> drivers/infiniband/ulp/srpt/ib_srpt.c:2723:    sdev->srq =
>> ib_create_srq(sdev->pd, &srq_attr);
>
> If we make the requirement that the ib_drain_rq() caller must consume
> all completions for all QPs attached to an SRQ if they are outstanding,
> then I think we can modify ib_drain_rq() to post the drain recv WR to
> the SRQ.  It should work, right?

At least the ib_srpt driver already guarantees that no further receive 
completions will be generated before ib_destroy_qp() is called. But 
posting an additional receive WR on the SRQ from inside ib_drain_rq() 
shouldn't hurt.

Bart.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Steve Wise]

On 4/26/2016 12:27 PM, Bart Van Assche wrote:
>>>>> Conversely, I would think that a consumer that uses SRQ
>>>>> would want a similar drain mechanism to guarantee there
>>>>> are no more posted Receives for the associated QP, and
>>>>> thus it is safe to destroy it.
>>>>
>>>> I don't think there is anything now that handles draining an SRQ, nor
>>>> ensuring a QP's RQEs are completed/consumed from its SRQ when draining
>>>> the QP.  Sounds like work is needed here.
>>>>
>>>> But are there any kernel SRQ consumers at this point?
>>>
>>> I think the following two:
>>>
>>> $ git grep -nHE '(ib|rdma)_create_srq' drivers/infiniband/ulp
>>> drivers/infiniband/ulp/ipoib/ipoib_cm.c:1521:    priv->cm.srq =
>>> ib_create_srq(priv->pd, &srq_init_attr);
>>> drivers/infiniband/ulp/srpt/ib_srpt.c:2723:    sdev->srq =
>>> ib_create_srq(sdev->pd, &srq_attr);
>>
>> If we make the requirement that the ib_drain_rq() caller must consume
>> all completions for all QPs attached to an SRQ if they are outstanding,
>> then I think we can modify ib_drain_rq() to post the drain recv WR to
>> the SRQ.  It should work, right?
>
> At least the ib_srpt driver already guarantees that no further receive
> completions will be generated before ib_destroy_qp() is called. But
> posting an additional receive WR on the SRQ from inside ib_drain_rq()
> shouldn't hurt.

Actually, after thinking more about this, posting anything to the SRQ 
will not force it to complete.  The only reason that works for a QP with 
an RQ is that the QP is in ERR state and thus the RECV WR gets completed 
with FLUSHED status.

So SRQ drain would require some other method...

Steve.



--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

> Actually, after thinking more about this, posting anything to the SRQ
> will not force it to complete.  The only reason that works for a QP with
> an RQ is that the QP is in ERR state and thus the RECV WR gets completed
> with FLUSHED status.

Exactly...

> So SRQ drain would require some other method...

I still don't understand the meaning of draining a SRQ.
There are no QP associated post recvs, the QP and contexts
(wc->qp) are assigned at completion processing time. When
we have a SRQ the recv contexts are not bound to a
session/connection/QP...

Can you well-define what are you trying to do?
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Steve Wise]

On 4/26/2016 2:24 PM, Sagi Grimberg wrote:
>
>> Actually, after thinking more about this, posting anything to the SRQ
>> will not force it to complete.  The only reason that works for a QP with
>> an RQ is that the QP is in ERR state and thus the RECV WR gets completed
>> with FLUSHED status.
>
> Exactly...
>
>> So SRQ drain would require some other method...
>
> I still don't understand the meaning of draining a SRQ.
> There are no QP associated post recvs, the QP and contexts
> (wc->qp) are assigned at completion processing time. When
> we have a SRQ the recv contexts are not bound to a
> session/connection/QP...
>
> Can you well-define what are you trying to do?

Answer Christoph's question. :)

I'll climb out of my rat hole now... :D:D:D


--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

>> If we make the requirement that the ib_drain_rq() caller must consume
>> all completions for all QPs attached to an SRQ if they are outstanding,
>> then I think we can modify ib_drain_rq() to post the drain recv WR to
>> the SRQ.  It should work, right?

That won't work because it won't FLUSH (SRQ is stateless and does not
FLUSH errors).

> At least the ib_srpt driver already guarantees that no further receive
> completions will be generated before ib_destroy_qp() is called. But
> posting an additional receive WR on the SRQ from inside ib_drain_rq()
> shouldn't hurt.

It doesn't have any meaning either...
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

>>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>>>
>>>> Just curious: what takes care of draining SRQs when we unregister them
>>>> after all queues are gone?
>>>
>>> Conversely, I would think that a consumer that uses SRQ
>>> would want a similar drain mechanism to guarantee there
>>> are no more posted Receives for the associated QP, and
>>> thus it is safe to destroy it.
>>
>> I don't think there is anything now that handles draining an SRQ, nor
>> ensuring a QP's RQEs are completed/consumed from its SRQ when draining
>> the QP.  Sounds like work is needed here.
>>
>> But are there any kernel SRQ consumers at this point?
>
> I think the following two:
>
> $ git grep -nHE '(ib|rdma)_create_srq' drivers/infiniband/ulp
> drivers/infiniband/ulp/ipoib/ipoib_cm.c:1521:	priv->cm.srq = ib_create_srq(priv->pd, &srq_init_attr);
> drivers/infiniband/ulp/srpt/ib_srpt.c:2723:	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);

And the nvme-fabrics one has a SRQ mode.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>>>
>>> Just curious: what takes care of draining SRQs when we unregister them
>>> after all queues are gone?
>>
>> Conversely, I would think that a consumer that uses SRQ
>> would want a similar drain mechanism to guarantee there
>> are no more posted Receives for the associated QP, and
>> thus it is safe to destroy it.
>
> I don't think there is anything now that handles draining an SRQ, nor
> ensuring a QP's RQEs are completed/consumed from its SRQ when draining
> the QP.

There is no "QP's RQEs" in s SRQ, this is a misconception.
A SRQ is just a bunch of receive buffers. The QP is assigned
and completion time (when the incoming recv is processed).
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

>> Just curious: what takes care of draining SRQs when we unregister them
>> after all queues are gone?
>
> Conversely, I would think that a consumer that uses SRQ
> would want a similar drain mechanism to guarantee there
> are no more posted Receives for the associated QP, and
> thus it is safe to destroy it.

Umm, not sure if that is completely true.

The consumer that uses a SRQ will usually detach the recv buffers and
contexts from the session/connection so I'm not sure if the consumer
will want/need a similar drain mechanism.

Also, there is no draining a SRQ, it doesn't move to ERROR state
and the (shared) receive completions are coming only from incoming
receives (no FLUSH errors).
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

Hi Christoph,

> Just curious: what takes care of draining SRQs when we unregister them
> after all queues are gone?

So unlike QPs, we can (and probably should) destroy SRQs after
destroying the CQs and that's a sufficient barrier to destroy the SRQs.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Bart Van Assche]

On 04/26/2016 07:55 AM, Sagi Grimberg wrote:
> SRQ attached QPs will fail receive work posts does not
> have a receive queue that needs to be drained, the receive
> queue is shared.

The patch itself looks fine to me but the description of this patch is 
very confusing. How about explaining that either an SRQ or an RQ is 
attached to a QP and hence that if an SRQ has been attached that there 
is no RQ attached and hence that it should not be drained?

Bart.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Sagi Grimberg]

>> SRQ attached QPs will fail receive work posts does not
>> have a receive queue that needs to be drained, the receive
>> queue is shared.
>
> The patch itself looks fine to me but the description of this patch is
> very confusing.

Sorry for the poor change log :) I was in a hurry (just now got back)
and quickly wrote description and sent out the patch without giving the
phrasing any thought.

> How about explaining that either an SRQ or an RQ is
> attached to a QP and hence that if an SRQ has been attached that there
> is no RQ attached and hence that it should not be drained?

I'll send out a better description, thanks.
--
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] IB/core: Don't drain the receive queue for srq attached queue-pair

[Doug Ledford]

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

On 4/26/2016 3:11 PM, Sagi Grimberg wrote:
> 
>>> SRQ attached QPs will fail receive work posts does not
>>> have a receive queue that needs to be drained, the receive
>>> queue is shared.
>>
>> The patch itself looks fine to me but the description of this patch is
>> very confusing.
> 
> Sorry for the poor change log :) I was in a hurry (just now got back)
> and quickly wrote description and sent out the patch without giving the
> phrasing any thought.
> 
>> How about explaining that either an SRQ or an RQ is
>> attached to a QP and hence that if an SRQ has been attached that there
>> is no RQ attached and hence that it should not be drained?
> 
> I'll send out a better description, thanks.

I already picked the patch up and reworded the description myself.  See
if you are happy with it.  If not, I'll reword it again before sending a
pull request.



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