On Thu, Feb 07, 2019 at 09:53:19AM +0200, Leon Romanovsky wrote:
> > I mostly dislike random bits like 'valid' with no clear definition or
> > locking.
> >
> > In this case the intent seems to be to determine if the object was
> > published through the xarray, meaning there could be some thread out
> > there holding the refcount and the xarray needs an erase. So why not
> > check that directly? It costs nothing.
> >
> > But I'm looking at this again, and I think the ultimate sequencing is
> > a bit off if the goal is to get the drivers to use this instead of
> > their IDR.
> >
> > I'd expect like this:
> >
> > ib_dealloc_pd()
> > {
> > // Hide from users, but keep the ID # reserved
> > down_write(&rt->rwsem);
> > add_has_been_done = xa_cmpxchg(xa, res->id, res, NULL, GFP_KERNEL) == res;
> > up_write(&rt->rwsem);
> > if (add_has_been_done)
> > rdma_restrack_put(res);
> > wait_for_completion(&res->comp);
> >
> > // Users are now gone
> >
> > // Driver finishes with ID
> > ib_dev->ops.dealloc_pd(pd);
> >
> > // Allow the ID to be reallocated now that the everyone is finished with it
> > if (add_has_been_done)
> > xa_erase(res->id);
> >
> > The challenge will be to make this pattern into something less painful
> > for all the call sites.
>
> We almost there, rdma_restrack_del() does that after ib_dev->ops.dealloc_pd(pd).
The first half should still be done before calling the driver dealloc.
> > ib_dev->ops.alloc_pd(pd)
> > restrack_assign_default_id(&pd->res);
> >
> > When drivers are converted they call
> > 'restrack_set_id'/'restrack_alloc_id' internally and
> > 'assign_default_id' does nothing.
>
> I had in mind slightly simpler solution than you are proposing.
>
> All drivers will declare in advance which objects are managed in HW.
> For SW-managed objects, zalloc_drv will assign res->id, reserve it
> with xa_reserve() and rdma_restrack_add() will xa_insert to this ->id.
>
> For HW-managed objects, zalloc_drv won't reserve res->id and drivers
> will set such id internally, rdma_restrack_add() will xa_insert()
> to this ->id.
>
> It ensures that rdma_restrack_add() is the same for SW/HW flows and
> many checks will be performed, like max_pd/msx_qp/e.t.c, will be
> performed ever before calling to drivers.
This is fine too, probably better as it makes it harder to have a half
initialized pointer in the xarray.
But if you use reserve (aka alloc and store NULL) then you have to
xa_erase in all the error cases to undo the ID allocation.
This can't be done with cmpxchg anymore, and you are back to 'valid'
(which is badly named, now, it is really 'id_is_valid') or maybe an
invalid ID #.
Jason
