> -----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.
> ie there is no reason for that mmap stuff to be under the kref
> release, c4iw_mmap will not be called once the core destroyes the
> ucontext.
>
So I think what is currently done in _c4iw_free_ucontext() can be done in
c4iw_dealloc_ucontext(), if what you say is true (all qps, mms, cqs, etc are
cleaned up prior).
> Fixing this needs another patch..
>
> > /**
> > * i40iw_dealloc_ucontext - deallocate the user context data structure
> > * @context: user context created during alloc
> > */
> > -static int i40iw_dealloc_ucontext(struct ib_ucontext *context)
> > +static void i40iw_dealloc_ucontext(struct ib_ucontext *context)
> > {
> > struct i40iw_ucontext *ucontext = to_ucontext(context);
> > - unsigned long flags;
> > -
> > - spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> > - if (!list_empty(&ucontext->cq_reg_mem_list)) {
> > - spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock,
> flags);
> > - return -EBUSY;
> > - }
> > - spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> > - spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
> > - if (!list_empty(&ucontext->qp_reg_mem_list)) {
> > - spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock,
> flags);
> > - return -EBUSY;
> > - }
> > - spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
> >
> > - kfree(ucontext);
> > - return 0;
> > + /*
> > + * dealloc called before all MRs QPs were freed,
> > + * no need to grab XXX_reg_mem_list_lock locks,
> > + * it is too lat and the warning below symbol of bug
> > + * in th driver.
> > + */
>
> various spelling errors
>
> > diff --git a/drivers/infiniband/hw/nes/nes_verbs.h
> b/drivers/infiniband/hw/nes/nes_verbs.h
> > index e02a5662dc20..114a9b59fefd 100644
> > +++ b/drivers/infiniband/hw/nes/nes_verbs.h
> > @@ -59,7 +59,6 @@ struct nes_ucontext {
> > struct list_head cq_reg_mem_list;
> > struct list_head qp_reg_mem_list;
> > u32 mcrqf;
> > - atomic_t usecnt;
>
> Lets have this in its own patch please
>
> > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> > index f44220f72e05..196d7a6e4c8c 100644
> > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> > @@ -306,41 +306,33 @@ int pvrdma_modify_port(struct ib_device *ibdev,
> u8 port, int mask,
> >
> > /**
> > * pvrdma_alloc_ucontext - allocate ucontext
> > - * @ibdev: the IB device
> > + * @uctx: the uverbs countext
> > * @udata: user data
> > *
> > - * @return: the ib_ucontext pointer on success, otherwise errno.
> > + * @return: zero on success, otherwise errno.
> > */
> > -struct ib_ucontext *pvrdma_alloc_ucontext(struct ib_device *ibdev,
> > - struct ib_udata *udata)
> > +int vrdma_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata
> *udata)
>
> Wrong name
>
> Jason
