On 2/6/2019 12:02 PM, Leon Romanovsky wrote:
> On Tue, Feb 05, 2019 at 08:10:32PM -0600, Steve Wise wrote:
>>
>>> -----Original Message-----
>>> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
>>> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
>>> Sent: Tuesday, February 5, 2019 5:54 PM
>>> To: Leon Romanovsky <leon@xxxxxxxxxx>; Steve Wise
>>> <swise@xxxxxxxxxxxxxxxxxxxxx>
>>> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
>>> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>
>>> Subject: Re: [PATCH rdma-next] RDMA: Handle ucontext allocations by
>>> IB/core
>>>
>>> On Mon, Feb 04, 2019 at 03:56:21PM +0200, Leon Romanovsky wrote:
>>>> diff --git a/drivers/infiniband/core/uverbs_cmd.c
>>> b/drivers/infiniband/core/uverbs_cmd.c
>>>> index 0aeb53ef7869..a48bb388160d 100644
>>>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>>>> @@ -224,13 +224,19 @@ static int ib_uverbs_get_context(struct
>>> uverbs_attr_bundle *attrs)
>>>> if (ret)
>>>> goto err;
>>>>
>>>> - ucontext = ib_dev->ops.alloc_ucontext(ib_dev, &attrs-
>>>> driver_udata);
>>>> - if (IS_ERR(ucontext)) {
>>>> - ret = PTR_ERR(ucontext);
>>>> + ucontext = rdma_zalloc_drv_obj(ib_dev, ib_ucontext);
>>>> + if (!ucontext) {
>>>> + ret = -ENOMEM;
>>>> goto err_alloc;
>>>> }
>>>>
>>>> + rdma_rt_set_type(&ucontext->res, RDMA_RESTRACK_CTX);
>>>> ucontext->device = ib_dev;
>>>> +
>>>> + ret = ib_dev->ops.alloc_ucontext(ucontext, &attrs->driver_udata);
>>>> + if (ret)
>>>> + goto err_free;
>>>> +
>>>> ucontext->cg_obj = cg_obj;
>>>> /* ufile is required when some objects are released */
>>>> ucontext->ufile = file;
>>>> @@ -262,7 +268,6 @@ static int ib_uverbs_get_context(struct
>>> uverbs_attr_bundle *attrs)
>>>> fd_install(resp.async_fd, filp);
>>>>
>>>> - rdma_rt_set_type(&ucontext->res, RDMA_RESTRACK_CTX);
>>>> rdma_restrack_uadd(&ucontext->res);
>>>>
>>>> /*
>>>> @@ -283,7 +288,7 @@ static int ib_uverbs_get_context(struct
>>> uverbs_attr_bundle *attrs)
>>>> put_unused_fd(resp.async_fd);
>>>>
>>>> err_free:
>>>> - ib_dev->ops.dealloc_ucontext(ucontext);
>>>> + kfree(ucontext);
>>> This error unwind isn't right any more.
>>>
>>> Re-organize it to have the driver call at the end when failure still
>>> works on the existing goto unwind
>>>
>>>> @@ -565,6 +556,7 @@ static const struct ib_device_ops c4iw_dev_ops = {
>>>> .reg_user_mr = c4iw_reg_user_mr,
>>>> .req_notify_cq = c4iw_arm_cq,
>>>> INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
>>>> + INIT_RDMA_OBJ_SIZE(ib_ucontext, c4iw_ucontext, ibucontext),
>>>> };
>>>>
>>>> void c4iw_register_device(struct work_struct *work)
>>>> diff --git a/drivers/infiniband/hw/cxgb4/qp.c
>>> b/drivers/infiniband/hw/cxgb4/qp.c
>>>> index 0fe87b9c1e10..5da85f7679d8 100644
>>>> +++ b/drivers/infiniband/hw/cxgb4/qp.c
>>>> @@ -2338,7 +2338,6 @@ struct ib_qp *c4iw_create_qp(struct ib_pd *pd,
>>> struct ib_qp_init_attr *attrs,
>>>> insert_mmap(ucontext, ma_sync_key_mm);
>>>> }
>>>>
>>>> - c4iw_get_ucontext(ucontext);
>>>> qhp->ucontext = ucontext;
>>> Why? Looks wrong...
>>>
>>> Actually this whole cxgb4 stuff looks very peculiar - I don't see how
>>> we can safely move the kfree out of its kref release and into the core
>>> code and be sure it will still work...
>>>
>>> Arguably drivers should not be refcounting these structs though - I
>>> wonder if the ref counting could be deleted?
>>>
>>> Steve? Is there some reason cxgb4 is the only driver trying to stick a
>>> refcount on the ucontext?
>>>
>> I don't think it is needed.
>>
>>> ucontext deletion means that every object contained in the ucontext
>>> has been deleted, the core code guarentees this.
>>>
>>> If there is some async work someplace then generally it should be
>>> flushed/canceled during ucontext delete, not managed via kref...
>>>
>> no async work. If the qps and cqs for that ucontext have all been
>> destroyed, then cxgb4 just needs to clean up the qids tracked by the
>> c4iw_dev_ucontext contained in the c4iw_ucontext.
> Steve, can you please send patch which does that?
> Otherwise, I'll send something completely untested. :)
Sure.
