On Sun, Jan 27, 2019 at 08:56:08PM +0200, Leon Romanovsky wrote:
> 41 files changed, 270 insertions(+), 435 deletions(-)
Wow!
> - pd = ib_dev->ops.alloc_pd(ib_dev, uobj->context, &attrs->driver_udata);
> - if (IS_ERR(pd)) {
> - ret = PTR_ERR(pd);
> + pd = kzalloc(RDMA_DRIVER_SIZE(ib_dev, ib_pd), GFP_KERNEL);
> + if (!pd) {
> + ret = -ENOMEM;
If we are going to add macros, I think this is better written as
#define rdma_zalloc_drv_obj(ib_dev, ib_type) \
((struct obj *)kzalloc(ib_dev->ops->size##ib_type, GFP_KERNEL))
pd = rdma_zalloc_drv_obj(ib_dev, ib_pd);
All drivers should be able to rely on zero initialized objects, and
this continues to propogate the type safey since we have it..
> goto err;
> }
>
> @@ -413,11 +413,15 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> pd->uobject = uobj;
> pd->__internal_mr = NULL;
> atomic_set(&pd->usecnt, 0);
> + rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> +
> + ret = ib_dev->ops.alloc_pd(pd, uobj->context, &attrs->driver_udata);
> + if (ret)
> + goto err_alloc;
>
> uobj->object = pd;
> memset(&resp, 0, sizeof resp);
> resp.pd_handle = uobj->id;
> - rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> rdma_restrack_uadd(&pd->res);
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> @@ -428,7 +432,8 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
>
> err_copy:
> ib_dealloc_pd(pd);
> -
> +err_alloc:
> + kfree(pd);
> err:
> uobj_alloc_abort(uobj);
> return ret;
> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index cbc72312eb41..f224cb727224 100644
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -188,7 +188,7 @@ static int uverbs_free_pd(struct ib_uobject *uobject,
> if (ret)
> return ret;
>
> - ib_dealloc_pd((struct ib_pd *)uobject->object);
> + ib_dealloc_pd(pd);
> return 0;
> }
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 814cbc814cf4..6acede55c9e8 100644
> +++ b/drivers/infiniband/core/verbs.c
> @@ -254,10 +254,11 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
> {
> struct ib_pd *pd;
> int mr_access_flags = 0;
> + int ret;
>
> - pd = device->ops.alloc_pd(device, NULL, NULL);
> - if (IS_ERR(pd))
> - return pd;
> + pd = kzalloc(RDMA_DRIVER_SIZE(device, ib_pd), GFP_KERNEL);
> + if (!pd)
> + return ERR_PTR(-ENOMEM);
Doesn't this miss
mr = pd->device->ops.get_dma_mr(pd, mr_access_flags);
if (IS_ERR(mr)) {
ib_dealloc_pd(pd);
return ERR_CAST(mr);
}
?
> pd->device = device;
> pd->uobject = NULL;
> @@ -265,6 +266,16 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
> atomic_set(&pd->usecnt, 0);
> pd->flags = flags;
>
> + rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> + rdma_restrack_set_task(&pd->res, caller);
> +
> + ret = device->ops.alloc_pd(pd, NULL, NULL);
> + if (ret) {
> + kfree(pd);
> + return ERR_PTR(ret);
> + }
> + rdma_restrack_kadd(&pd->res);
> +
> if (device->attrs.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
> pd->local_dma_lkey = device->local_dma_lkey;
> else
> @@ -275,10 +286,6 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
> mr_access_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
> }
>
> - rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> - rdma_restrack_set_task(&pd->res, caller);
> - rdma_restrack_kadd(&pd->res);
> -
> if (mr_access_flags) {
> struct ib_mr *mr;
>
> @@ -329,10 +336,8 @@ void ib_dealloc_pd(struct ib_pd *pd)
> WARN_ON(atomic_read(&pd->usecnt));
>
> rdma_restrack_del(&pd->res);
> - /* Making delalloc_pd a void return is a WIP, no driver should return
> - an error here. */
> - ret = pd->device->ops.dealloc_pd(pd);
> - WARN_ONCE(ret, "Infiniband HW driver failed dealloc_pd");
> + pd->device->ops.dealloc_pd(pd);
> + kfree(pd);
> }
> EXPORT_SYMBOL(ib_dealloc_pd);
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 9bc637e49faa..e71fee5f42f4 100644
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -563,41 +563,29 @@ static int bnxt_re_create_fence_mr(struct bnxt_re_pd *pd)
> }
>
> /* Protection Domains */
> -int bnxt_re_dealloc_pd(struct ib_pd *ib_pd)
> +void bnxt_re_dealloc_pd(struct ib_pd *ib_pd)
> {
> struct bnxt_re_pd *pd = container_of(ib_pd, struct bnxt_re_pd, ib_pd);
> struct bnxt_re_dev *rdev = pd->rdev;
> - int rc;
>
> bnxt_re_destroy_fence_mr(pd);
>
> - if (pd->qplib_pd.id) {
> - rc = bnxt_qplib_dealloc_pd(&rdev->qplib_res,
> - &rdev->qplib_res.pd_tbl,
> - &pd->qplib_pd);
> - if (rc)
> - dev_err(rdev_to_dev(rdev), "Failed to deallocate HW PD");
Should we be deleting these warnings? Does indicate a driver problem.
Maybe making destory_pd void should be a another patch?
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index 16eecfa5882c..20e41521455f 100644
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -575,6 +575,7 @@ static const struct ib_device_ops bnxt_re_dev_ops = {
> .alloc_hw_stats = bnxt_re_ib_alloc_hw_stats,
> .alloc_mr = bnxt_re_alloc_mr,
> .alloc_pd = bnxt_re_alloc_pd,
> + INIT_RDMA_DRIVER_SIZE(ib_pd, bnxt_re_pd, ib_pd),
> .alloc_ucontext = bnxt_re_alloc_ucontext,
> .create_ah = bnxt_re_create_ah,
> .create_cq = bnxt_re_create_cq,
I think we should keep these sorted at the end of the struct. Putting
them here makes keep-sorted harder.
> @@ -742,12 +743,16 @@ static int hns_roce_v1_rsv_lp_qp(struct hns_roce_dev *hr_dev)
> free_mr->mr_free_cq->ib_cq.cq_context = NULL;
> atomic_set(&free_mr->mr_free_cq->ib_cq.usecnt, 0);
>
> - pd = hns_roce_alloc_pd(&hr_dev->ib_dev, NULL, NULL);
> - if (IS_ERR(pd)) {
> - dev_err(dev, "Create pd for reserved loop qp failed!");
> - ret = -ENOMEM;
> + ibdev = &hr_dev->ib_dev;
> + pd = kzalloc(RDMA_DRIVER_SIZE(ibdev, ib_pd), GFP_KERNEL);
> + if (pd)
> + goto alloc_mem_failed;
> +
> + pd->device = ibdev;
> + ret = hns_roce_alloc_pd(pd, NULL, NULL);
> + if (ret)
> goto alloc_pd_failed;
> - }
So this thing doesn't exist in restrack then? Weird.. It would be
nicer if this driver would call the verbs instead..
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index a1f097fa33f2..cf59c61cb953 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -2380,10 +2380,10 @@ struct ib_device_ops {
> int (*dealloc_ucontext)(struct ib_ucontext *context);
> int (*mmap)(struct ib_ucontext *context, struct vm_area_struct *vma);
> void (*disassociate_ucontext)(struct ib_ucontext *ibcontext);
> - struct ib_pd *(*alloc_pd)(struct ib_device *device,
> - struct ib_ucontext *context,
> - struct ib_udata *udata);
> - int (*dealloc_pd)(struct ib_pd *pd);
> + int (*alloc_pd)(struct ib_pd *pd, struct ib_ucontext *context,
> + struct ib_udata *udata);
> + DECLARE_RDMA_DRIVER_SIZE(ib_pd);
These should be a sorted block at the end, putting them here will
waste memory due to alignment.
Jason
