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. :)
>
> > 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
>
Attachment:
signature.asc
Description: PGP signature
