| Message ID | 1460046664-552-17-git-send-email-mustafa.ismail@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Apr 07, 2016 at 11:31:04AM -0500, Mustafa Ismail wrote: > Adding sq and rq drain functions, which block until all > previously posted wr-s in the specified queue have completed. > A completion object is signaled to unblock the thread, > when the last cqe for the corresponding queue is processed. > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > Signed-off-by: Faisal Latif <faisal.latif@intel.com> I want to bring back last conversation about need special function to drain QPs for iWARP devices. Come on guys, it is already third place there is the same implementation is introduced in last 2 months.
On Thu, Apr 07, 2016 at 05:46:38PM -0700, Leon Romanovsky wrote: > On Thu, Apr 07, 2016 at 11:31:04AM -0500, Mustafa Ismail wrote: > > Adding sq and rq drain functions, which block until all > > previously posted wr-s in the specified queue have completed. > > A completion object is signaled to unblock the thread, > > when the last cqe for the corresponding queue is processed. > > > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > > Signed-off-by: Faisal Latif <faisal.latif@intel.com> > > I want to bring back last conversation about need special function to > drain QPs for iWARP devices. > Come on guys, it is already third place there is the same implementation > is introduced in last 2 months. In order to move this into core, the core would need to determine if the sq and rq are empty. To do this it will need the driver's internal sq and rq information such as head and tail. Then if not empty it will need more information about the qp's cq processing. All this is device specific. It is just a few lines of code in each driver to check if its hw sq and rq are empty and if not, handle it from its cq processing. For this reason, it seems best left to each iWARP driver to handle. Thanks, Mustafa -- 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 Thu, Apr 07, 2016 at 05:46:38PM -0700, Leon Romanovsky wrote: > > On Thu, Apr 07, 2016 at 11:31:04AM -0500, Mustafa Ismail wrote: > > > Adding sq and rq drain functions, which block until all > > > previously posted wr-s in the specified queue have completed. > > > A completion object is signaled to unblock the thread, > > > when the last cqe for the corresponding queue is processed. > > > > > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com> > > > Signed-off-by: Faisal Latif <faisal.latif@intel.com> > > > > I want to bring back last conversation about need special function to > > drain QPs for iWARP devices. > > Come on guys, it is already third place there is the same implementation > > is introduced in last 2 months. > > In order to move this into core, the core would need to determine if the > sq and rq are empty. To do this it will need the driver's internal sq and > rq information such as head and tail. Then if not empty it will need > more information about the qp's cq processing. All this is device specific. > > It is just a few lines of code in each driver to check if its hw sq and rq > are empty and if not, handle it from its cq processing. > > For this reason, it seems best left to each iWARP driver to handle. > > Thanks, I agree. My cxgb4 drain functions need to check if the sq/rq are empty as well. And for cxgb4, that will require holding the qp spinlock. (I need to post this patch soon). So while they are all very similar, the guts for looking at the sq/rq are device-specific. 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
>>> I want to bring back last conversation about need special function to >>> drain QPs for iWARP devices. >>> Come on guys, it is already third place there is the same implementation >>> is introduced in last 2 months. >> >> In order to move this into core, the core would need to determine if the >> sq and rq are empty. To do this it will need the driver's internal sq and >> rq information such as head and tail. Then if not empty it will need >> more information about the qp's cq processing. All this is device specific. >> >> It is just a few lines of code in each driver to check if its hw sq and rq >> are empty and if not, handle it from its cq processing. >> >> For this reason, it seems best left to each iWARP driver to handle. >> >> Thanks, > > I agree. My cxgb4 drain functions need to check if the sq/rq are empty as well. > And for cxgb4, that will require holding the qp spinlock. (I need to post this > patch soon). > > So while they are all very similar, the guts for looking at the sq/rq are > device-specific. I actually agree here too. The iWARP folks needs some driver specific logic to get the QP drained correctly so I'm all for them having the freedom to do whats best for them. I do understand Leon when he sees 3 copies of this code, but all these copies are looking into the raw queue so I'm fine with it. Cheers, Sagi. -- 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 Sun, Apr 10, 2016 at 10:26:08PM +0300, Sagi Grimberg wrote: > I do understand Leon when he sees 3 copies of this code, but all these > copies are looking into the raw queue so I'm fine with it. Despite the fact that I understand the reasoning behind this specific implementation, it makes me wonder when will we see common implementation for this type of queues? We saw 3 similar implementations, in near future, we will get another one and I believe that it won't stop there.
diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c index 9da5361..fc0bace 100644 --- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c +++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c @@ -789,6 +789,8 @@ static struct ib_qp *i40iw_create_qp(struct ib_pd *ibpd, return ERR_PTR(err_code); } } + init_completion(&iwqp->sq_drained); + init_completion(&iwqp->rq_drained); return &iwqp->ibqp; error: @@ -1582,6 +1584,32 @@ static int i40iw_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_ne } /** + * i40iw_drain_sq - drain the send queue + * @ibqp: ib qp pointer + */ +static void i40iw_drain_sq(struct ib_qp *ibqp) +{ + struct i40iw_qp *iwqp = to_iwqp(ibqp); + struct i40iw_sc_qp *qp = &iwqp->sc_qp; + + if (I40IW_RING_MORE_WORK(qp->qp_uk.sq_ring)) + wait_for_completion(&iwqp->sq_drained); +} + +/** + * i40iw_drain_rq - drain the receive queue + * @ibqp: ib qp pointer + */ +static void i40iw_drain_rq(struct ib_qp *ibqp) +{ + struct i40iw_qp *iwqp = to_iwqp(ibqp); + struct i40iw_sc_qp *qp = &iwqp->sc_qp; + + if (I40IW_RING_MORE_WORK(qp->qp_uk.rq_ring)) + wait_for_completion(&iwqp->rq_drained); +} + +/** * i40iw_hwreg_mr - send cqp command for memory registration * @iwdev: iwarp device * @iwmr: iwarp mr pointer @@ -2216,6 +2244,7 @@ static int i40iw_poll_cq(struct ib_cq *ibcq, enum i40iw_status_code ret; struct i40iw_cq_uk *ukcq; struct i40iw_sc_qp *qp; + struct i40iw_qp *iwqp; unsigned long flags; iwcq = (struct i40iw_cq *)ibcq; @@ -2266,6 +2295,13 @@ static int i40iw_poll_cq(struct ib_cq *ibcq, qp = (struct i40iw_sc_qp *)cq_poll_info.qp_handle; entry->qp = (struct ib_qp *)qp->back_qp; entry->src_qp = cq_poll_info.qp_id; + iwqp = (struct i40iw_qp *)qp->back_qp; + if (iwqp->iwarp_state > I40IW_QP_STATE_RTS) { + if (!I40IW_RING_MORE_WORK(qp->qp_uk.sq_ring)) + complete(&iwqp->sq_drained); + if (!I40IW_RING_MORE_WORK(qp->qp_uk.rq_ring)) + complete(&iwqp->rq_drained); + } entry->byte_len = cq_poll_info.bytes_xfered; entry++; cqe_count++; @@ -2512,6 +2548,8 @@ static struct i40iw_ib_device *i40iw_init_rdma_device(struct i40iw_device *iwdev iwibdev->ibdev.query_device = i40iw_query_device; iwibdev->ibdev.create_ah = i40iw_create_ah; iwibdev->ibdev.destroy_ah = i40iw_destroy_ah; + iwibdev->ibdev.drain_sq = i40iw_drain_sq; + iwibdev->ibdev.drain_rq = i40iw_drain_rq; iwibdev->ibdev.alloc_mr = i40iw_alloc_mr; iwibdev->ibdev.map_mr_sg = i40iw_map_mr_sg; iwibdev->ibdev.iwcm = kzalloc(sizeof(*iwibdev->ibdev.iwcm), GFP_KERNEL); diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.h b/drivers/infiniband/hw/i40iw/i40iw_verbs.h index 0acb6c8..0069be8 100644 --- a/drivers/infiniband/hw/i40iw/i40iw_verbs.h +++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.h @@ -170,5 +170,7 @@ struct i40iw_qp { struct i40iw_pbl *iwpbl; struct i40iw_dma_mem q2_ctx_mem; struct i40iw_dma_mem ietf_mem; + struct completion sq_drained; + struct completion rq_drained; }; #endif