On Fri, Oct 02, 2020 at 10:13:18AM -0300, Jason Gunthorpe wrote:
> On Sat, Sep 26, 2020 at 01:19:35PM +0300, Leon Romanovsky wrote:
> > @@ -449,7 +453,10 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> > ret = ib_dev->ops.alloc_pd(pd, &attrs->driver_udata);
> > if (ret)
> > goto err_alloc;
> > - rdma_restrack_add(&pd->res);
> > +
> > + ret = rdma_restrack_add(&pd->res);
> > + if (ret)
> > + goto err_restrack;
> >
> > uobj->object = pd;
> > uobj_finalize_uobj_create(uobj, attrs);
> > @@ -457,6 +464,9 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> > resp.pd_handle = uobj->id;
> > return uverbs_response(attrs, &resp, sizeof(resp));
> >
> > +err_restrack:
> > + ib_dev->ops.dealloc_pd(pd, &attrs->driver_udata);
>
> There can be no failure between allocating the HW object and calling
> uobj_finalize_uobj_create(). That was the whole point of that scheme.
> It is really important that be kept.
>
> Now that destroys are allowed to fail we aboslutely cannot have any
> open coded destroys like this *anywhere* in the uobject system.
>
> I think you need to go back to a model where the xarray allocation can
> fail but we can still call del, otherwise the error unwind is a
> complete nightmare.
>
> This also has problems like this:
>
> int ib_dereg_mr_user(struct ib_mr *mr, struct ib_udata *udata)
> {
> rdma_restrack_del(&mr->res);
> ret = mr->device->ops.dereg_mr(mr, udata);
> if (!ret) {
>
> With the uobject destroy retry scheme restrack_del will be called
> multiple times.
>
> I think this is pretty simple to solve, write del as this:
>
> void rdma_restrack_del(struct rdma_restrack_entry *res)
> {
> struct ib_device *dev = res_to_dev(res);
> struct rdma_restrack_entry *old;
> struct rdma_restrack_root *rt;
>
> if (WARN_ON(!dev))
> return
>
> rt = &dev->res[res->type];
> if (xa_cmpxchg(&rt->xa, res->id, res, NULL, GFP_KERNEL) != res)
> return;
> rdma_restrack_put(res);
> wait_for_completion(&res->comp);
> }
>
> There is no need to do the wait_for_completion if we didn't change the
> xarray.
We still need to do it, because restrack does two things at the same
time: manages object lifetime (rdma_restrack_new and rdma_restrack_put)
and stores HW IDs (rdma_restrack_add).
For the objects that marked as no_track we still want to ensure that
their allocations are correct.
Thanks
>
> Jason
