Re: [PATCH rdma-next v3 6/9] RDMA/restrack: Add error handling while adding restrack object

[Leon Romanovsky <leon@kernel.org>]

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