On Thu, Feb 07, 2019 at 10:31:39AM -0700, Jason Gunthorpe wrote:
> 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 #.
I'll figure it out when I'll submit my SW/HW ID allocation patches.
For now, I'll use your xa_cmxchng variant.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
