| Message ID | 1461682538-19647-1-git-send-email-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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@grimberg.me> > --- > 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@opengridcomputing.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/26/2016 10:55 AM, Chuck Lever wrote: > >> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> On Apr 26, 2016, at 11:43 AM, Christoph Hellwig <hch@lst.de> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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);
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@grimberg.me> --- drivers/infiniband/core/verbs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)