* [PATCH for-rc 0/6] vmw_pvrdma fixes for 4.15 @ 2017-12-08 18:58 Bryan Tan [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-08 18:58 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Do some cleanup and fixes related to the comments received during SRQ support upstreaming that applied to existing code. Also add in two missed macros used by the userlevel library for SRQs, a missed ib_umem_release, and an incorrect usage of the new refcount_t type. Bryan Tan (6): RDMA/vmw_pvrdma: Clarify QP is_kernel logic RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file drivers/infiniband/hw/vmw_pvrdma/pvrdma.h | 4 ++-- drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 6 +++--- drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 19 ++++++++----------- drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 20 +++++++++++++------- drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++ 6 files changed, 30 insertions(+), 25 deletions(-) -- 1.8.5.6 -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-08 19:00 ` Bryan Tan [not found] ` <20171208190010.GA31023-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:01 ` [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path Bryan Tan ` (4 subsequent siblings) 5 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:00 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Be more consistent in checking is_kernel flag for QPs. Testing Done: ibv_rc_pingpong, rping, perftests. Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c index 10420a1..b932b7e 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, init_waitqueue_head(&qp->wait); qp->state = IB_QPS_RESET; + qp->is_kernel = !(pd->uobject && udata); - if (pd->uobject && udata) { + if (!qp->is_kernel) { dev_dbg(&dev->pdev->dev, "create queuepair from user space\n"); @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, qp->npages_recv = 0; qp->npages = qp->npages_send + qp->npages_recv; } else { - qp->is_kernel = true; - ret = pvrdma_set_sq_size(to_vdev(pd->device), &init_attr->cap, qp); if (ret) @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, err_pdir: pvrdma_page_dir_cleanup(dev, &qp->pdir); err_umem: - if (pd->uobject && udata) { + if (!qp->is_kernel) { if (qp->rumem) ib_umem_release(qp->rumem); if (qp->sumem) -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171208190010.GA31023-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic [not found] ` <20171208190010.GA31023-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 9:02 ` Yuval Shaia 2017-12-13 19:08 ` Bryan Tan 0 siblings, 1 reply; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 9:02 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote: > Be more consistent in checking is_kernel flag for QPs. Consist with what? (asking because expecting also pvrdma_create_cq to be fixed). > > Testing Done: ibv_rc_pingpong, rping, perftests. These are all userspace tests, right? > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > --- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > index 10420a1..b932b7e 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > init_waitqueue_head(&qp->wait); > > qp->state = IB_QPS_RESET; > + qp->is_kernel = !(pd->uobject && udata); > > - if (pd->uobject && udata) { > + if (!qp->is_kernel) { > dev_dbg(&dev->pdev->dev, > "create queuepair from user space\n"); > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > qp->npages_recv = 0; > qp->npages = qp->npages_send + qp->npages_recv; > } else { > - qp->is_kernel = true; > - > ret = pvrdma_set_sq_size(to_vdev(pd->device), > &init_attr->cap, qp); > if (ret) > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > err_pdir: > pvrdma_page_dir_cleanup(dev, &qp->pdir); > err_umem: > - if (pd->uobject && udata) { > + if (!qp->is_kernel) { > if (qp->rumem) > ib_umem_release(qp->rumem); > if (qp->sumem) Regardless of the comments: Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> > -- > 1.8.5.6 > > -- > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic 2017-12-13 9:02 ` Yuval Shaia @ 2017-12-13 19:08 ` Bryan Tan [not found] ` <20171213190825.GH20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-13 19:08 UTC (permalink / raw) To: Yuval Shaia; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote: > On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote: > > Be more consistent in checking is_kernel flag for QPs. > > Consist with what? > (asking because expecting also pvrdma_create_cq to be fixed). I had updated create SRQ's is_kernel in the same way. Thanks for pointing out that I had missed it in create CQ. > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > These are all userspace tests, right? Yes. That was accidentally carried over from our how we format our commits internally. I'll remove the "Testing Done" line here and in the next commit message. > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > --- > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > index 10420a1..b932b7e 100644 > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > init_waitqueue_head(&qp->wait); > > > > qp->state = IB_QPS_RESET; > > + qp->is_kernel = !(pd->uobject && udata); > > > > - if (pd->uobject && udata) { > > + if (!qp->is_kernel) { > > dev_dbg(&dev->pdev->dev, > > "create queuepair from user space\n"); > > > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > qp->npages_recv = 0; > > qp->npages = qp->npages_send + qp->npages_recv; > > } else { > > - qp->is_kernel = true; > > - > > ret = pvrdma_set_sq_size(to_vdev(pd->device), > > &init_attr->cap, qp); > > if (ret) > > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > err_pdir: > > pvrdma_page_dir_cleanup(dev, &qp->pdir); > > err_umem: > > - if (pd->uobject && udata) { > > + if (!qp->is_kernel) { > > if (qp->rumem) > > ib_umem_release(qp->rumem); > > if (qp->sumem) > > Regardless of the comments: > > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> > > > -- > > 1.8.5.6 > > > > -- > > 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 https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e= -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213190825.GH20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic [not found] ` <20171213190825.GH20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 19:20 ` Yuval Shaia 2017-12-13 21:22 ` Bryan Tan 0 siblings, 1 reply; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 19:20 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 11:08:26AM -0800, Bryan Tan wrote: > On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote: > > On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote: > > > Be more consistent in checking is_kernel flag for QPs. > > > > Consist with what? > > (asking because expecting also pvrdma_create_cq to be fixed). > > I had updated create SRQ's is_kernel in the same way. Thanks for > pointing out that I had missed it in create CQ. > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > These are all userspace tests, right? > > Yes. That was accidentally carried over from our how we > format our commits internally. I'll remove the "Testing Done" > line here and in the next commit message. But...since you exposed this info my question remains, you changed a flow of a branch that checks if verb was requested by kernel or userspace but you tested it only with userspace tools. > > > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > --- > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > index 10420a1..b932b7e 100644 > > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > init_waitqueue_head(&qp->wait); > > > > > > qp->state = IB_QPS_RESET; > > > + qp->is_kernel = !(pd->uobject && udata); > > > > > > - if (pd->uobject && udata) { > > > + if (!qp->is_kernel) { > > > dev_dbg(&dev->pdev->dev, > > > "create queuepair from user space\n"); > > > > > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > qp->npages_recv = 0; > > > qp->npages = qp->npages_send + qp->npages_recv; > > > } else { > > > - qp->is_kernel = true; > > > - > > > ret = pvrdma_set_sq_size(to_vdev(pd->device), > > > &init_attr->cap, qp); > > > if (ret) > > > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > err_pdir: > > > pvrdma_page_dir_cleanup(dev, &qp->pdir); > > > err_umem: > > > - if (pd->uobject && udata) { > > > + if (!qp->is_kernel) { > > > if (qp->rumem) > > > ib_umem_release(qp->rumem); > > > if (qp->sumem) > > > > Regardless of the comments: > > > > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> > > > > > -- > > > 1.8.5.6 > > > > > > -- > > > 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 https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e= -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic 2017-12-13 19:20 ` Yuval Shaia @ 2017-12-13 21:22 ` Bryan Tan 0 siblings, 0 replies; 32+ messages in thread From: Bryan Tan @ 2017-12-13 21:22 UTC (permalink / raw) To: Yuval Shaia; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 09:20:09PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 11:08:26AM -0800, Bryan Tan wrote: > > On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote: > > > On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote: > > > > Be more consistent in checking is_kernel flag for QPs. > > > > > > Consist with what? > > > (asking because expecting also pvrdma_create_cq to be fixed). > > > > I had updated create SRQ's is_kernel in the same way. Thanks for > > pointing out that I had missed it in create CQ. > > > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > These are all userspace tests, right? > > > > Yes. That was accidentally carried over from our how we > > format our commits internally. I'll remove the "Testing Done" > > line here and in the next commit message. > > But...since you exposed this info my question remains, you changed a flow > of a branch that checks if verb was requested by kernel or userspace but > you tested it only with userspace tools. Given that it was an if-else and the nature of the change I felt it sufficient to test with userspace applications. A kernel QP is also created for CM, which is used in rping. I just tested the patch series with krping though, and no issues there. > > > > > > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > --- > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > index 10420a1..b932b7e 100644 > > > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > > init_waitqueue_head(&qp->wait); > > > > > > > > qp->state = IB_QPS_RESET; > > > > + qp->is_kernel = !(pd->uobject && udata); > > > > > > > > - if (pd->uobject && udata) { > > > > + if (!qp->is_kernel) { > > > > dev_dbg(&dev->pdev->dev, > > > > "create queuepair from user space\n"); > > > > > > > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > > qp->npages_recv = 0; > > > > qp->npages = qp->npages_send + qp->npages_recv; > > > > } else { > > > > - qp->is_kernel = true; > > > > - > > > > ret = pvrdma_set_sq_size(to_vdev(pd->device), > > > > &init_attr->cap, qp); > > > > if (ret) > > > > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, > > > > err_pdir: > > > > pvrdma_page_dir_cleanup(dev, &qp->pdir); > > > > err_umem: > > > > - if (pd->uobject && udata) { > > > > + if (!qp->is_kernel) { > > > > if (qp->rumem) > > > > ib_umem_release(qp->rumem); > > > > if (qp->sumem) > > > > > > Regardless of the comments: > > > > > > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> > > > > > > > -- > > > > 1.8.5.6 > > > > > > > > -- > > > > 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 https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e= -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:00 ` [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic Bryan Tan @ 2017-12-08 19:01 ` Bryan Tan [not found] ` <20171208190106.GA32066-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:01 ` [PATCH for-rc 3/6] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc Bryan Tan ` (3 subsequent siblings) 5 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:01 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA The QP cleanup did not previously call ib_umem_release. Fix this. Testing Done: ibv_rc_pingpong, rping, perftests. Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c index b932b7e..77e7e57 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) atomic_dec(&qp->refcnt); wait_event(qp->wait, !atomic_read(&qp->refcnt)); + if (!qp->is_kernel) { + if (qp->rumem) + ib_umem_release(qp->rumem); + if (qp->sumem) + ib_umem_release(qp->sumem); + } + pvrdma_page_dir_cleanup(dev, &qp->pdir); kfree(qp); -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171208190106.GA32066-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path [not found] ` <20171208190106.GA32066-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 8:53 ` Yuval Shaia 2017-12-13 16:17 ` Jason Gunthorpe 0 siblings, 1 reply; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 8:53 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > The QP cleanup did not previously call ib_umem_release. Fix this. > > Testing Done: ibv_rc_pingpong, rping, perftests. > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > --- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > index b932b7e..77e7e57 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > atomic_dec(&qp->refcnt); > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > + if (!qp->is_kernel) { > + if (qp->rumem) IS_ERR_OR_NULL? > + ib_umem_release(qp->rumem); > + if (qp->sumem) > + ib_umem_release(qp->sumem); > + } > + > pvrdma_page_dir_cleanup(dev, &qp->pdir); > > kfree(qp); > -- > 1.8.5.6 > > -- > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path 2017-12-13 8:53 ` Yuval Shaia @ 2017-12-13 16:17 ` Jason Gunthorpe [not found] ` <20171213161755.GB16920-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-13 16:17 UTC (permalink / raw) To: Yuval Shaia; +Cc: Bryan Tan, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > index b932b7e..77e7e57 100644 > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > atomic_dec(&qp->refcnt); > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > + if (!qp->is_kernel) { > > + if (qp->rumem) > > IS_ERR_OR_NULL? How can we get an ERR_PTR in rumem? Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213161755.GB16920-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path [not found] ` <20171213161755.GB16920-uk2M96/98Pc@public.gmane.org> @ 2017-12-13 18:40 ` Yuval Shaia 2017-12-13 18:55 ` Bryan Tan 2017-12-13 18:56 ` Jason Gunthorpe 0 siblings, 2 replies; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 18:40 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Bryan Tan, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > index b932b7e..77e7e57 100644 > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > atomic_dec(&qp->refcnt); > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > + if (!qp->is_kernel) { > > > + if (qp->rumem) > > > > IS_ERR_OR_NULL? > > How can we get an ERR_PTR in rumem? qp->rumem is set with ib_umem_get which can either return ib_umem pointer or ERR_PTR. So i looked at other places which calls ib_umem_release. The first one that pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. So assuming it can be used here. > > Jason > -- > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path 2017-12-13 18:40 ` Yuval Shaia @ 2017-12-13 18:55 ` Bryan Tan [not found] ` <20171213185517.GG20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 18:56 ` Jason Gunthorpe 1 sibling, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-13 18:55 UTC (permalink / raw) To: Yuval Shaia; +Cc: Jason Gunthorpe, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > index b932b7e..77e7e57 100644 > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > atomic_dec(&qp->refcnt); > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > + if (!qp->is_kernel) { > > > > + if (qp->rumem) > > > > > > IS_ERR_OR_NULL? > > > > How can we get an ERR_PTR in rumem? > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > or ERR_PTR. > So i looked at other places which calls ib_umem_release. The first one that > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > So assuming it can be used here. We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. If it's ERR_PTR we fail the QP creation: > qp->rumem = ib_umem_get(pd->uobject->context, > ucmd.rbuf_addr, > ucmd.rbuf_size, 0, 0); > if (IS_ERR(qp->rumem)) { > ret = PTR_ERR(qp->rumem); > goto err_qp; > } so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). Thanks, Bryan -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213185517.GG20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path [not found] ` <20171213185517.GG20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 19:04 ` Yuval Shaia 2017-12-13 19:16 ` Yuval Shaia 0 siblings, 1 reply; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 19:04 UTC (permalink / raw) To: Bryan Tan; +Cc: Jason Gunthorpe, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote: > On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > index b932b7e..77e7e57 100644 > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > > atomic_dec(&qp->refcnt); > > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > > > + if (!qp->is_kernel) { > > > > > + if (qp->rumem) > > > > > > > > IS_ERR_OR_NULL? > > > > > > How can we get an ERR_PTR in rumem? > > > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > > or ERR_PTR. > > So i looked at other places which calls ib_umem_release. The first one that > > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > > > So assuming it can be used here. > > We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. > If it's ERR_PTR we fail the QP creation: > > > qp->rumem = ib_umem_get(pd->uobject->context, > > ucmd.rbuf_addr, > > ucmd.rbuf_size, 0, 0); > > if (IS_ERR(qp->rumem)) { > > ret = PTR_ERR(qp->rumem); > > goto err_qp; > > } > > so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). I see. Missed that. > > Thanks, > Bryan -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path 2017-12-13 19:04 ` Yuval Shaia @ 2017-12-13 19:16 ` Yuval Shaia 0 siblings, 0 replies; 32+ messages in thread From: Yuval Shaia @ 2017-12-13 19:16 UTC (permalink / raw) To: Bryan Tan; +Cc: Jason Gunthorpe, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 09:04:22PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote: > > On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > > > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > > index b932b7e..77e7e57 100644 > > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > > > atomic_dec(&qp->refcnt); > > > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > > > > > + if (!qp->is_kernel) { > > > > > > + if (qp->rumem) > > > > > > > > > > IS_ERR_OR_NULL? > > > > > > > > How can we get an ERR_PTR in rumem? > > > > > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > > > or ERR_PTR. > > > So i looked at other places which calls ib_umem_release. The first one that > > > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > > > > > So assuming it can be used here. > > > > We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. > > If it's ERR_PTR we fail the QP creation: > > > > > qp->rumem = ib_umem_get(pd->uobject->context, > > > ucmd.rbuf_addr, > > > ucmd.rbuf_size, 0, 0); > > > if (IS_ERR(qp->rumem)) { > > > ret = PTR_ERR(qp->rumem); > > > goto err_qp; > > > } > > > > so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). > > I see. > Missed that. That being said i have no other questions. Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> > > > > > Thanks, > > Bryan > -- > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path 2017-12-13 18:40 ` Yuval Shaia 2017-12-13 18:55 ` Bryan Tan @ 2017-12-13 18:56 ` Jason Gunthorpe 1 sibling, 0 replies; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-13 18:56 UTC (permalink / raw) To: Yuval Shaia; +Cc: Bryan Tan, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > index b932b7e..77e7e57 100644 > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > atomic_dec(&qp->refcnt); > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > + if (!qp->is_kernel) { > > > > + if (qp->rumem) > > > > > > IS_ERR_OR_NULL? > > > > How can we get an ERR_PTR in rumem? > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > or ERR_PTR. In generall ERR_PTR's should not be permanently stored in structs and I do not expect to see IS_ERR_OR_NULL used. Several kernel devs consider is an abomination, and it certainly should *NEVER* be used on a ptr which cannot, by design, have an ERR_PTR in it. At least vmw_pvrdma appears to be OK: qp->rumem = ib_umem_get(pd->uobject->context, ucmd.rbuf_addr, ucmd.rbuf_size, 0, 0); if (IS_ERR(qp->rumem)) { ret = PTR_ERR(qp->rumem); goto err_qp; [..] err_qp: kfree(qp); So we cannot have a 'qp' struct with an ERR_PTR in rumem. > So i looked at other places which calls ib_umem_release. The first one that > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. The test in bnxt looks wrong to me, and should be removed: umem = ib_umem_get(context, ureq.qprva, bytes, IB_ACCESS_LOCAL_WRITE, 1); if (IS_ERR(umem)) goto rqfail; qp->rumem = umem; So rumem can never be assigned to PTR_ERR, and this is garbage: if (!IS_ERR_OR_NULL(qp->rumem)) ib_umem_release(qp->rumem); Similar remark for other tests in that driver. Care to send a patch? Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH for-rc 3/6] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:00 ` [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic Bryan Tan 2017-12-08 19:01 ` [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path Bryan Tan @ 2017-12-08 19:01 ` Bryan Tan 2017-12-08 19:02 ` [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning Bryan Tan ` (2 subsequent siblings) 5 siblings, 0 replies; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:01 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Convert the sizeof(void *) in two kcalloc calls to be more specific for the arrays that are being allocated. Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c index 1f4e187..941e324 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c @@ -243,13 +243,13 @@ static int pvrdma_register_device(struct pvrdma_dev *dev) mutex_init(&dev->port_mutex); spin_lock_init(&dev->desc_lock); - dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(void *), + dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(struct pvrdma_cq *), GFP_KERNEL); if (!dev->cq_tbl) return ret; spin_lock_init(&dev->cq_tbl_lock); - dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(void *), + dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(struct pvrdma_qp *), GFP_KERNEL); if (!dev->qp_tbl) goto err_cq_free; -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> ` (2 preceding siblings ...) 2017-12-08 19:01 ` [PATCH for-rc 3/6] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc Bryan Tan @ 2017-12-08 19:02 ` Bryan Tan [not found] ` <20171208190218.GA744-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:02 ` [PATCH for-rc 5/6] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t Bryan Tan 2017-12-08 19:03 ` [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file Bryan Tan 5 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:02 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA refcount_dec generates a warning when the operation causes the refcount to hit zero. Avoid this by using refcount_dec_and_test. Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c index 826ccb8..a2b1a3c 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) dev->srq_tbl[srq->srq_handle] = NULL; spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); - refcount_dec(&srq->refcnt); - wait_event(srq->wait, !refcount_read(&srq->refcnt)); + if (!refcount_dec_and_test(&srq->refcnt)) + wait_event(srq->wait, !refcount_read(&srq->refcnt)); /* There is no support for kernel clients, so this is safe. */ ib_umem_release(srq->umem); -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171208190218.GA744-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171208190218.GA744-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-08 23:30 ` Jason Gunthorpe [not found] ` <20171208233049.GB23239-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-08 23:30 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > refcount_dec generates a warning when the operation > causes the refcount to hit zero. Avoid this by using > refcount_dec_and_test. > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > index 826ccb8..a2b1a3c 100644 > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > dev->srq_tbl[srq->srq_handle] = NULL; > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > - refcount_dec(&srq->refcnt); > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > + if (!refcount_dec_and_test(&srq->refcnt)) > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); I really don't like this idiom for using refcount, refcount should not be dec'd below 0 even under a dec_and_test.. Why not just simplify: wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); ??? Same comment on the other patch switching from atomic to refcount Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171208233049.GB23239-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171208233049.GB23239-uk2M96/98Pc@public.gmane.org> @ 2017-12-11 18:33 ` Bryan Tan [not found] ` <20171211183314.GA20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-11 18:33 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote: > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > > refcount_dec generates a warning when the operation > > causes the refcount to hit zero. Avoid this by using > > refcount_dec_and_test. > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > index 826ccb8..a2b1a3c 100644 > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > > dev->srq_tbl[srq->srq_handle] = NULL; > > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > > > - refcount_dec(&srq->refcnt); > > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > + if (!refcount_dec_and_test(&srq->refcnt)) > > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > I really don't like this idiom for using refcount, refcount should > not be dec'd below 0 even under a dec_and_test.. > Why not just simplify: > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > ??? > > Same comment on the other patch switching from atomic to refcount > > Jason The refcount doesn't go below 0 in either case--the problem is that I didn't realise refcount_dec complains about decrements resulting in a value of 0 [1]. There are no problems with refcount_dec_and_test resulting in a value of 0. About checking for refcnt == 1, I do not know of a safe way to only wake up when the refcount hits 1. Right now we do that by checking for 0 and doing a wake_up if the result of refcount_dec_and_test is 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to accomplish this without another dec_and_test in the destroy path, I can do so. Bryan [1] https://patchwork.kernel.org/patch/9863573/ 3rd paragraph -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171211183314.GA20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171211183314.GA20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-11 20:14 ` Jason Gunthorpe [not found] ` <20171211201421.GD27709-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-11 20:14 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote: > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote: > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > > > refcount_dec generates a warning when the operation > > > causes the refcount to hit zero. Avoid this by using > > > refcount_dec_and_test. > > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > index 826ccb8..a2b1a3c 100644 > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > > > dev->srq_tbl[srq->srq_handle] = NULL; > > > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > > > > > - refcount_dec(&srq->refcnt); > > > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > + if (!refcount_dec_and_test(&srq->refcnt)) > > > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > I really don't like this idiom for using refcount, refcount should > > not be dec'd below 0 even under a dec_and_test.. > > Why not just simplify: > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > > > ??? > > > > Same comment on the other patch switching from atomic to refcount > > > > Jason > > The refcount doesn't go below 0 in either case--the problem is that > I didn't realise refcount_dec complains about decrements resulting > in a value of 0 [1]. There are no problems with refcount_dec_and_test > resulting in a value of 0. Thats what I ment with my remark.. Sorry for the confusion > About checking for refcnt == 1, I do not know of a safe way to only > wake up when the refcount hits 1. Right now we do that by checking > for 0 and doing a wake_up if the result of refcount_dec_and_test is > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to > accomplish this without another dec_and_test in the destroy path, I > can do so. What is wrong with this: wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); ? refcount == 1 means the caller is the last owner of the refcount, presumably you have already taken care to ensure it cannot be inc'd again (or the code is already not locked right) Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171211201421.GD27709-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171211201421.GD27709-uk2M96/98Pc@public.gmane.org> @ 2017-12-11 21:59 ` Bryan Tan [not found] ` <20171211215936.GC20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-11 21:59 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote: > On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote: > > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote: > > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > > > > refcount_dec generates a warning when the operation > > > > causes the refcount to hit zero. Avoid this by using > > > > refcount_dec_and_test. > > > > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > index 826ccb8..a2b1a3c 100644 > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > > > > dev->srq_tbl[srq->srq_handle] = NULL; > > > > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > > > > > > > - refcount_dec(&srq->refcnt); > > > > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > + if (!refcount_dec_and_test(&srq->refcnt)) > > > > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > I really don't like this idiom for using refcount, refcount should > > > not be dec'd below 0 even under a dec_and_test.. > > > Why not just simplify: > > > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > > > > > ??? > > > > > > Same comment on the other patch switching from atomic to refcount > > > > > > Jason > > > > The refcount doesn't go below 0 in either case--the problem is that > > I didn't realise refcount_dec complains about decrements resulting > > in a value of 0 [1]. There are no problems with refcount_dec_and_test > > resulting in a value of 0. > > Thats what I ment with my remark.. Sorry for the confusion > > > About checking for refcnt == 1, I do not know of a safe way to only > > wake up when the refcount hits 1. Right now we do that by checking > > for 0 and doing a wake_up if the result of refcount_dec_and_test is > > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to > > accomplish this without another dec_and_test in the destroy path, I > > can do so. > > What is wrong with this: > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > ? > > refcount == 1 means the caller is the last owner of the refcount, > presumably you have already taken care to ensure it cannot be inc'd > again (or the code is already not locked right) In the SRQ event handler, we do this at the end of handling the event: > if (refcount_dec_and_test(&srq->refcnt)) > wake_up(&srq->wait); If we wait on refcnt == 1, there isn't an atomic way to decrement and check if the result is 1. If it's fine to wake up the process while the condition hasn't been met, this would work, but that doesn't seem to be the right solution here, unless I'm missing something. Bryan -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171211215936.GC20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171211215936.GC20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-11 22:14 ` Jason Gunthorpe [not found] ` <20171211221425.GA7551-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-11 22:14 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Mon, Dec 11, 2017 at 01:59:37PM -0800, Bryan Tan wrote: > On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote: > > On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote: > > > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote: > > > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > > > > > refcount_dec generates a warning when the operation > > > > > causes the refcount to hit zero. Avoid this by using > > > > > refcount_dec_and_test. > > > > > > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > > index 826ccb8..a2b1a3c 100644 > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > > > > > dev->srq_tbl[srq->srq_handle] = NULL; > > > > > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > > > > > > > > > - refcount_dec(&srq->refcnt); > > > > > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > + if (!refcount_dec_and_test(&srq->refcnt)) > > > > > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > > > I really don't like this idiom for using refcount, refcount should > > > > not be dec'd below 0 even under a dec_and_test.. > > > > Why not just simplify: > > > > > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > > > > > > > ??? > > > > > > > > Same comment on the other patch switching from atomic to refcount > > > > > > > > Jason > > > > > > The refcount doesn't go below 0 in either case--the problem is that > > > I didn't realise refcount_dec complains about decrements resulting > > > in a value of 0 [1]. There are no problems with refcount_dec_and_test > > > resulting in a value of 0. > > > > Thats what I ment with my remark.. Sorry for the confusion > > > > > About checking for refcnt == 1, I do not know of a safe way to only > > > wake up when the refcount hits 1. Right now we do that by checking > > > for 0 and doing a wake_up if the result of refcount_dec_and_test is > > > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to > > > accomplish this without another dec_and_test in the destroy path, I > > > can do so. > > > > What is wrong with this: > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > ? > > > > refcount == 1 means the caller is the last owner of the refcount, > > presumably you have already taken care to ensure it cannot be inc'd > > again (or the code is already not locked right) > > In the SRQ event handler, we do this at the end of handling the event: > > > if (refcount_dec_and_test(&srq->refcnt)) > > wake_up(&srq->wait); What? You can't combine one thread doing if (refcount_dec_and_test(&srq->refcnt)) wake_up(&srq->wait); With if (!refcount_dec_and_test(&srq->refcnt)) wait_event(srq->wait, !refcount_read(&srq->refcnt)); It makes no sense, how is that a refcount??? They can't *BOTH* refcount_dec_and_test! I can understand doing if (refcount_dec_and_test(&srq->refcnt)) wake_up(&srq->wait); and then wait_event(srq->wait, !refcount_read(&srq->refcnt)); That makes perfect sense. Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171211221425.GA7551-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171211221425.GA7551-uk2M96/98Pc@public.gmane.org> @ 2017-12-12 17:13 ` Bryan Tan [not found] ` <20171212171300.GD20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-12 17:13 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Mon, Dec 11, 2017 at 03:14:25PM -0700, Jason Gunthorpe wrote: > On Mon, Dec 11, 2017 at 01:59:37PM -0800, Bryan Tan wrote: > > On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote: > > > On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote: > > > > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote: > > > > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote: > > > > > > refcount_dec generates a warning when the operation > > > > > > causes the refcount to hit zero. Avoid this by using > > > > > > refcount_dec_and_test. > > > > > > > > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > > > index 826ccb8..a2b1a3c 100644 > > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c > > > > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq) > > > > > > dev->srq_tbl[srq->srq_handle] = NULL; > > > > > > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags); > > > > > > > > > > > > - refcount_dec(&srq->refcnt); > > > > > > - wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > + if (!refcount_dec_and_test(&srq->refcnt)) > > > > > > + wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > > > > > I really don't like this idiom for using refcount, refcount should > > > > > not be dec'd below 0 even under a dec_and_test.. > > > > > Why not just simplify: > > > > > > > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > > > > > > > > > ??? > > > > > > > > > > Same comment on the other patch switching from atomic to refcount > > > > > > > > > > Jason > > > > > > > > The refcount doesn't go below 0 in either case--the problem is that > > > > I didn't realise refcount_dec complains about decrements resulting > > > > in a value of 0 [1]. There are no problems with refcount_dec_and_test > > > > resulting in a value of 0. > > > > > > Thats what I ment with my remark.. Sorry for the confusion > > > > > > > About checking for refcnt == 1, I do not know of a safe way to only > > > > wake up when the refcount hits 1. Right now we do that by checking > > > > for 0 and doing a wake_up if the result of refcount_dec_and_test is > > > > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to > > > > accomplish this without another dec_and_test in the destroy path, I > > > > can do so. > > > > > > What is wrong with this: > > > > > > wait_event(srq->wait, refcount_read(&srq->refcnt) == 1); > > > ? > > > > > > refcount == 1 means the caller is the last owner of the refcount, > > > presumably you have already taken care to ensure it cannot be inc'd > > > again (or the code is already not locked right) > > > > In the SRQ event handler, we do this at the end of handling the event: > > > > > if (refcount_dec_and_test(&srq->refcnt)) > > > wake_up(&srq->wait); > > What? You can't combine one thread doing > > if (refcount_dec_and_test(&srq->refcnt)) > wake_up(&srq->wait); > > With > if (!refcount_dec_and_test(&srq->refcnt)) > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > It makes no sense, how is that a refcount??? > > They can't *BOTH* refcount_dec_and_test! > > I can understand doing > > if (refcount_dec_and_test(&srq->refcnt)) > wake_up(&srq->wait); > > and then > > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > That makes perfect sense. Perhaps I am mistaken in my understanding of refcounts here, but what is wrong with this? If it's not that the refcount is set to 1 on resource creation (which, by your earlier suggestion of checking refcnt == 1, seems to be fine), and both callers (the resource event handler and resource destroy call) need to make sure the destroy sequence only happens once (when the refcount reaches 0), then using refcount_dec_and_test seems right to me. Your last suggestion would mean setting the refcount to 0 on resource creation, as we wait on refcount hitting 0, but that would mean the resource event handler would often be calling wake_up unnecessarily. What we're doing is effectively the same as how mlx5 handles this (see drivers/net/ethernet/mellanox/mlx5/core/srq.c) I suppose using refcount_t requires one to follow a particular model of usage but I'm not sure how this isn't right. Thanks, Bryan -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171212171300.GD20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171212171300.GD20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-12 18:55 ` Jason Gunthorpe [not found] ` <20171212185530.GB16603-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-12 18:55 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA > > What? You can't combine one thread doing > > > > if (refcount_dec_and_test(&srq->refcnt)) > > wake_up(&srq->wait); > > > > With > > if (!refcount_dec_and_test(&srq->refcnt)) > > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > It makes no sense, how is that a refcount??? > > > > They can't *BOTH* refcount_dec_and_test! > > > > I can understand doing > > > > if (refcount_dec_and_test(&srq->refcnt)) > > wake_up(&srq->wait); > > > > and then > > > > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > That makes perfect sense. > > Perhaps I am mistaken in my understanding of refcounts here, but > what is wrong with this? If it's not that the refcount is set to > 1 on resource creation (which, by your earlier suggestion of > checking refcnt == 1, seems to be fine), and both callers (the > resource event handler and resource destroy call) need to make > sure the destroy sequence only happens once (when the refcount > reaches 0), then using refcount_dec_and_test seems right to me. I think the issue here is trying to fit an optimized approach that was using atomics into a refcount. I have certain expectations when I see something called 'refcount' that this scheme really doesn't follow anymore, and those expectations are pretty much matched by the protections inside refcount against going to 0 and so on. The algorithm works OK, but I'm not sure it is a 'refcount', more of a 'usecnt'.. On the other hand, having the refcount overflow protections in this path does seem useful. I did a random audit of refcount_dec_and_test existing users and didn't find any that use this pattern. What you've actually built here is a rwsem out of an atomic and a wait queue.. So, what is wrong with a rwsem? > What we're doing is effectively the same as how mlx5 handles > this (see drivers/net/ethernet/mellanox/mlx5/core/srq.c) I > suppose using refcount_t requires one to follow a particular > model of usage but I'm not sure how this isn't right. I didn't say it was wrong, I asked 'how is that a refcount??' Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171212185530.GB16603-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171212185530.GB16603-uk2M96/98Pc@public.gmane.org> @ 2017-12-13 0:08 ` Bryan Tan [not found] ` <20171213000815.GE20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-13 0:08 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote: > > > What? You can't combine one thread doing > > > > > > if (refcount_dec_and_test(&srq->refcnt)) > > > wake_up(&srq->wait); > > > > > > With > > > if (!refcount_dec_and_test(&srq->refcnt)) > > > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > It makes no sense, how is that a refcount??? > > > > > > They can't *BOTH* refcount_dec_and_test! > > > > > > I can understand doing > > > > > > if (refcount_dec_and_test(&srq->refcnt)) > > > wake_up(&srq->wait); > > > > > > and then > > > > > > wait_event(srq->wait, !refcount_read(&srq->refcnt)); > > > > > > That makes perfect sense. > > > > Perhaps I am mistaken in my understanding of refcounts here, but > > what is wrong with this? If it's not that the refcount is set to > > 1 on resource creation (which, by your earlier suggestion of > > checking refcnt == 1, seems to be fine), and both callers (the > > resource event handler and resource destroy call) need to make > > sure the destroy sequence only happens once (when the refcount > > reaches 0), then using refcount_dec_and_test seems right to me. > > I think the issue here is trying to fit an optimized approach that was > using atomics into a refcount. > > I have certain expectations when I see something called 'refcount' > that this scheme really doesn't follow anymore, and those expectations > are pretty much matched by the protections inside refcount against > going to 0 and so on. > > The algorithm works OK, but I'm not sure it is a 'refcount', more of a > 'usecnt'.. Hm, it's still not clear to me what expectations are not followed here, and how you're differentiating 'refcount' from 'usecnt'. And not that it makes it right, but I did notice that mlx4 uses the same pattern as what I've done here [1]. If it makes the most sense to revert to atomic operations or use a rwsem, I can do that. Just trying to understand what the expectations are for using refcount_t. Bryan [1] Commit ff61b5e3f041c2f1aa8d7c700af3007889973889 -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213000815.GE20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171213000815.GE20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 2:20 ` Jason Gunthorpe [not found] ` <20171213022031.GE16603-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-13 2:20 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Tue, Dec 12, 2017 at 04:08:16PM -0800, Bryan Tan wrote: > On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote: > Hm, it's still not clear to me what expectations are not followed here, > and how you're differentiating 'refcount' from 'usecnt'. A refcount is something that has exactly one 'refcount_dec_and_test' that then goes on to 'free' the thing being refcountedt. It never has a wait queue. A usecnt is something that has an async path that waits for all users to stop using it then 'frees' it. It always includes a wait queue. > And not that it makes it right, but I did notice that mlx4 uses the > same pattern as what I've done here [1]. Ah, this is good, I was trying to find something like it. Kees approves so I have no problem taking your patch. Though you might want to reflect on if using a completion is better than a wait_event.. Not immediately clear to me. Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213022031.GE16603-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171213022031.GE16603-uk2M96/98Pc@public.gmane.org> @ 2017-12-13 9:07 ` Bryan Tan [not found] ` <20171213090702.GF20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-13 9:07 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Tue, Dec 12, 2017 at 07:20:31PM -0700, Jason Gunthorpe wrote: > On Tue, Dec 12, 2017 at 04:08:16PM -0800, Bryan Tan wrote: > > On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote: > > > Hm, it's still not clear to me what expectations are not followed here, > > and how you're differentiating 'refcount' from 'usecnt'. > > A refcount is something that has exactly one 'refcount_dec_and_test' > that then goes on to 'free' the thing being refcountedt. It never has > a wait queue. > > A usecnt is something that has an async path that waits for all users > to stop using it then 'frees' it. It always includes a wait > queue. I see, thanks for the explanation. > > And not that it makes it right, but I did notice that mlx4 uses the > > same pattern as what I've done here [1]. > > Ah, this is good, I was trying to find something like it. Kees > approves so I have no problem taking your patch. > > Though you might want to reflect on if using a completion is better > than a wait_event.. Not immediately clear to me. Oof, thanks for pointing that out. There's a problem here it seems: 0. refcount for a resource is 2 after entering an event handler 1. refcount_dec_and_test gets called by the resource destroy 2. refcount_dec_and_test gets called by the resource event handler 3. resource destroy call checks the condition in wait_event and proceeds to free the resource 4. resource event handler calls wake_up() on free'd resource I'll use a completion here instead. -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171213090702.GF20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning [not found] ` <20171213090702.GF20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-13 16:12 ` Jason Gunthorpe 0 siblings, 0 replies; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-13 16:12 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Wed, Dec 13, 2017 at 01:07:02AM -0800, Bryan Tan wrote: > > > And not that it makes it right, but I did notice that mlx4 uses the > > > same pattern as what I've done here [1]. > > > > Ah, this is good, I was trying to find something like it. Kees > > approves so I have no problem taking your patch. > > > > Though you might want to reflect on if using a completion is better > > than a wait_event.. Not immediately clear to me. > > Oof, thanks for pointing that out. There's a problem here it seems: > 0. refcount for a resource is 2 after entering an event handler > 1. refcount_dec_and_test gets called by the resource destroy > 2. refcount_dec_and_test gets called by the resource event handler > 3. resource destroy call checks the condition in wait_event and > proceeds to free the resource > 4. resource event handler calls wake_up() on free'd resource Yes I agree. I knew it looked funny :) Please resend Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH for-rc 5/6] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> ` (3 preceding siblings ...) 2017-12-08 19:02 ` [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning Bryan Tan @ 2017-12-08 19:02 ` Bryan Tan 2017-12-08 19:03 ` [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file Bryan Tan 5 siblings, 0 replies; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:02 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA refcount_t is the preferred type for refcounts. Change the QP and CQ refcnt fields to use refcount_t. Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma.h | 4 ++-- drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 6 +++--- drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 15 ++++++--------- drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 6 +++--- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h index 63bc2ef..07d287e 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h @@ -93,7 +93,7 @@ struct pvrdma_cq { struct pvrdma_page_dir pdir; u32 cq_handle; bool is_kernel; - atomic_t refcnt; + refcount_t refcnt; wait_queue_head_t wait; }; @@ -196,7 +196,7 @@ struct pvrdma_qp { u8 state; bool is_kernel; struct mutex mutex; /* QP state mutex. */ - atomic_t refcnt; + refcount_t refcnt; wait_queue_head_t wait; }; diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c index 3562c0c..11cda60 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c @@ -178,7 +178,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev, else pvrdma_page_dir_insert_umem(&cq->pdir, cq->umem, 0); - atomic_set(&cq->refcnt, 1); + refcount_set(&cq->refcnt, 1); init_waitqueue_head(&cq->wait); spin_lock_init(&cq->cq_lock); @@ -230,8 +230,8 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev, static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq) { - atomic_dec(&cq->refcnt); - wait_event(cq->wait, !atomic_read(&cq->refcnt)); + if (!refcount_dec_and_test(&cq->refcnt)) + wait_event(cq->wait, !refcount_read(&cq->refcnt)); if (!cq->is_kernel) ib_umem_release(cq->umem); diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c index 941e324..5cff9fa 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c @@ -333,7 +333,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type) spin_lock_irqsave(&dev->qp_tbl_lock, flags); qp = dev->qp_tbl[qpn % dev->dsr->caps.max_qp]; if (qp) - atomic_inc(&qp->refcnt); + refcount_inc(&qp->refcnt); spin_unlock_irqrestore(&dev->qp_tbl_lock, flags); if (qp && qp->ibqp.event_handler) { @@ -346,8 +346,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type) ibqp->event_handler(&e, ibqp->qp_context); } if (qp) { - atomic_dec(&qp->refcnt); - if (atomic_read(&qp->refcnt) == 0) + if (refcount_dec_and_test(&qp->refcnt)) wake_up(&qp->wait); } } @@ -360,7 +359,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type) spin_lock_irqsave(&dev->cq_tbl_lock, flags); cq = dev->cq_tbl[cqn % dev->dsr->caps.max_cq]; if (cq) - atomic_inc(&cq->refcnt); + refcount_inc(&cq->refcnt); spin_unlock_irqrestore(&dev->cq_tbl_lock, flags); if (cq && cq->ibcq.event_handler) { @@ -373,8 +372,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type) ibcq->event_handler(&e, ibcq->cq_context); } if (cq) { - atomic_dec(&cq->refcnt); - if (atomic_read(&cq->refcnt) == 0) + if (refcount_dec_and_test(&cq->refcnt)) wake_up(&cq->wait); } } @@ -533,14 +531,13 @@ static irqreturn_t pvrdma_intrx_handler(int irq, void *dev_id) spin_lock_irqsave(&dev->cq_tbl_lock, flags); cq = dev->cq_tbl[cqne->info % dev->dsr->caps.max_cq]; if (cq) - atomic_inc(&cq->refcnt); + refcount_inc(&cq->refcnt); spin_unlock_irqrestore(&dev->cq_tbl_lock, flags); if (cq && cq->ibcq.comp_handler) cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context); if (cq) { - atomic_dec(&cq->refcnt); - if (atomic_read(&cq->refcnt)) + if (refcount_dec_and_test(&cq->refcnt)) wake_up(&cq->wait); } pvrdma_idx_ring_inc(&ring->cons_head, ring_slots); diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c index 77e7e57..9745cb1 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c @@ -245,7 +245,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, spin_lock_init(&qp->sq.lock); spin_lock_init(&qp->rq.lock); mutex_init(&qp->mutex); - atomic_set(&qp->refcnt, 1); + refcount_set(&qp->refcnt, 1); init_waitqueue_head(&qp->wait); qp->state = IB_QPS_RESET; @@ -427,8 +427,8 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) pvrdma_unlock_cqs(scq, rcq, &scq_flags, &rcq_flags); - atomic_dec(&qp->refcnt); - wait_event(qp->wait, !atomic_read(&qp->refcnt)); + if (!refcount_dec_and_test(&qp->refcnt)) + wait_event(qp->wait, !refcount_read(&qp->refcnt)); if (!qp->is_kernel) { if (qp->rumem) -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> ` (4 preceding siblings ...) 2017-12-08 19:02 ` [PATCH for-rc 5/6] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t Bryan Tan @ 2017-12-08 19:03 ` Bryan Tan [not found] ` <20171208190317.GA3636-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 5 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-08 19:03 UTC (permalink / raw) To: linux-rdma-u79uwXL29TY76Z2rM5mHXA Add the two UAR SRQ macros that are required by the userlevel library. Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> --- include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h index aaa352f..4007cac 100644 --- a/include/uapi/rdma/vmw_pvrdma-abi.h +++ b/include/uapi/rdma/vmw_pvrdma-abi.h @@ -58,6 +58,8 @@ #define PVRDMA_UAR_CQ_ARM_SOL BIT(29) /* Arm solicited bit. */ #define PVRDMA_UAR_CQ_ARM BIT(30) /* Arm bit. */ #define PVRDMA_UAR_CQ_POLL BIT(31) /* Poll bit. */ +#define PVRDMA_UAR_SRQ_OFFSET 8 /* SRQ doorbell. */ +#define PVRDMA_UAR_SRQ_RECV BIT(30) /* Recv bit. */ enum pvrdma_wr_opcode { PVRDMA_WR_RDMA_WRITE, -- 1.8.5.6 -- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171208190317.GA3636-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file [not found] ` <20171208190317.GA3636-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-08 23:28 ` Jason Gunthorpe [not found] ` <20171208232809.GA23239-uk2M96/98Pc@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-08 23:28 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote: > Add the two UAR SRQ macros that are required by the userlevel > library. > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h > index aaa352f..4007cac 100644 > +++ b/include/uapi/rdma/vmw_pvrdma-abi.h > @@ -58,6 +58,8 @@ > #define PVRDMA_UAR_CQ_ARM_SOL BIT(29) /* Arm solicited bit. */ > #define PVRDMA_UAR_CQ_ARM BIT(30) /* Arm bit. */ > #define PVRDMA_UAR_CQ_POLL BIT(31) /* Poll bit. */ > +#define PVRDMA_UAR_SRQ_OFFSET 8 /* SRQ doorbell. */ > +#define PVRDMA_UAR_SRQ_RECV BIT(30) /* Recv bit. */ I didn't think BIT() was allowed in uapi headers? Where does it get defined in user space? Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171208232809.GA23239-uk2M96/98Pc@public.gmane.org>]
* Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file [not found] ` <20171208232809.GA23239-uk2M96/98Pc@public.gmane.org> @ 2017-12-11 18:35 ` Bryan Tan [not found] ` <20171211183554.GB20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bryan Tan @ 2017-12-11 18:35 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Dec 08, 2017 at 04:28:09PM -0700, Jason Gunthorpe wrote: > On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote: > > Add the two UAR SRQ macros that are required by the userlevel > > library. > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h > > index aaa352f..4007cac 100644 > > +++ b/include/uapi/rdma/vmw_pvrdma-abi.h > > @@ -58,6 +58,8 @@ > > #define PVRDMA_UAR_CQ_ARM_SOL BIT(29) /* Arm solicited bit. */ > > #define PVRDMA_UAR_CQ_ARM BIT(30) /* Arm bit. */ > > #define PVRDMA_UAR_CQ_POLL BIT(31) /* Poll bit. */ > > +#define PVRDMA_UAR_SRQ_OFFSET 8 /* SRQ doorbell. */ > > +#define PVRDMA_UAR_SRQ_RECV BIT(30) /* Recv bit. */ > > I didn't think BIT() was allowed in uapi headers? Where does it get > defined in user space? > > Jason Looks like we had BIT() defined in userspace. Seems like the rest of the headers just use a bit shift? If that's fine I can send out a V2 with the BIT()s removed. Bryan -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171211183554.GB20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org>]
* Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file [not found] ` <20171211183554.GB20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> @ 2017-12-11 20:12 ` Jason Gunthorpe 0 siblings, 0 replies; 32+ messages in thread From: Jason Gunthorpe @ 2017-12-11 20:12 UTC (permalink / raw) To: Bryan Tan; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA On Mon, Dec 11, 2017 at 10:35:55AM -0800, Bryan Tan wrote: > On Fri, Dec 08, 2017 at 04:28:09PM -0700, Jason Gunthorpe wrote: > > On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote: > > > Add the two UAR SRQ macros that are required by the userlevel > > > library. > > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support") > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org> > > > include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h > > > index aaa352f..4007cac 100644 > > > +++ b/include/uapi/rdma/vmw_pvrdma-abi.h > > > @@ -58,6 +58,8 @@ > > > #define PVRDMA_UAR_CQ_ARM_SOL BIT(29) /* Arm solicited bit. */ > > > #define PVRDMA_UAR_CQ_ARM BIT(30) /* Arm bit. */ > > > #define PVRDMA_UAR_CQ_POLL BIT(31) /* Poll bit. */ > > > +#define PVRDMA_UAR_SRQ_OFFSET 8 /* SRQ doorbell. */ > > > +#define PVRDMA_UAR_SRQ_RECV BIT(30) /* Recv bit. */ > > > > I didn't think BIT() was allowed in uapi headers? Where does it get > > defined in user space? > > > > Jason > > Looks like we had BIT() defined in userspace. In rdma-core? Please remove that too :) > Seems like the rest of the headers just use a bit shift? If that's > fine I can send out a V2 with the BIT()s removed. Yes please remove them all Jason -- 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2017-12-13 21:22 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-08 18:58 [PATCH for-rc 0/6] vmw_pvrdma fixes for 4.15 Bryan Tan [not found] ` <20171208185818.GA28514-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 19:00 ` [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic Bryan Tan [not found] ` <20171208190010.GA31023-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 9:02 ` Yuval Shaia 2017-12-13 19:08 ` Bryan Tan [not found] ` <20171213190825.GH20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 19:20 ` Yuval Shaia 2017-12-13 21:22 ` Bryan Tan 2017-12-08 19:01 ` [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path Bryan Tan [not found] ` <20171208190106.GA32066-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 8:53 ` Yuval Shaia 2017-12-13 16:17 ` Jason Gunthorpe [not found] ` <20171213161755.GB16920-uk2M96/98Pc@public.gmane.org> 2017-12-13 18:40 ` Yuval Shaia 2017-12-13 18:55 ` Bryan Tan [not found] ` <20171213185517.GG20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 19:04 ` Yuval Shaia 2017-12-13 19:16 ` Yuval Shaia 2017-12-13 18:56 ` Jason Gunthorpe 2017-12-08 19:01 ` [PATCH for-rc 3/6] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc Bryan Tan 2017-12-08 19:02 ` [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning Bryan Tan [not found] ` <20171208190218.GA744-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 23:30 ` Jason Gunthorpe [not found] ` <20171208233049.GB23239-uk2M96/98Pc@public.gmane.org> 2017-12-11 18:33 ` Bryan Tan [not found] ` <20171211183314.GA20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-11 20:14 ` Jason Gunthorpe [not found] ` <20171211201421.GD27709-uk2M96/98Pc@public.gmane.org> 2017-12-11 21:59 ` Bryan Tan [not found] ` <20171211215936.GC20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-11 22:14 ` Jason Gunthorpe [not found] ` <20171211221425.GA7551-uk2M96/98Pc@public.gmane.org> 2017-12-12 17:13 ` Bryan Tan [not found] ` <20171212171300.GD20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-12 18:55 ` Jason Gunthorpe [not found] ` <20171212185530.GB16603-uk2M96/98Pc@public.gmane.org> 2017-12-13 0:08 ` Bryan Tan [not found] ` <20171213000815.GE20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 2:20 ` Jason Gunthorpe [not found] ` <20171213022031.GE16603-uk2M96/98Pc@public.gmane.org> 2017-12-13 9:07 ` Bryan Tan [not found] ` <20171213090702.GF20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-13 16:12 ` Jason Gunthorpe 2017-12-08 19:02 ` [PATCH for-rc 5/6] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t Bryan Tan 2017-12-08 19:03 ` [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file Bryan Tan [not found] ` <20171208190317.GA3636-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-08 23:28 ` Jason Gunthorpe [not found] ` <20171208232809.GA23239-uk2M96/98Pc@public.gmane.org> 2017-12-11 18:35 ` Bryan Tan [not found] ` <20171211183554.GB20684-qXbCdz4EeRo1jLI2hToXVI42T8aCTgcwy4vvyvUx+exJXi8ZT2ovy+oDBWuYMCC/JZORHMmSJCU@public.gmane.org> 2017-12-11 20:12 ` Jason Gunthorpe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.