On Wed, Feb 06, 2019 at 07:53:16PM +0200, Leon Romanovsky wrote:
> > void rdma_restrack_del(struct rdma_restrack_entry *res)
> > {
> > [..]
> >
> > down_write(&rt->rwsem);
> > xa_erase(xa, res->id);
> > up_write(&rt->rwsem);
> >
> > rdma_restrack_put(res);
> >
> > wait_for_completion(&res->comp);
> >
> > I'd also get rid of valid during the xarray conversion.
> >
> > The only place that sets it to true is after rt_xa_alloc_cyclic, so
> > write it as:
> >
> > down_write(&rt->rwsem);
> > add_has_been_done = xa_cmpxchg(xa, res->id, res, NULL, GFP_KERNEL) == res;
> > up_write(&rt->rwsem);
> >
> > Ie if the entry was stored in it's own ID in the xarray then it is
> > 'valid'.
>
> what is the advantage over simple if (!ret) check?
You mean if valid?
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.
I also continue to think if you are adding the zalloc_drv thing then
that should also initialize the restrack (ie the kref, completion,
etc), so the new zalloc flow would look like:
ib_alloc_pd()
{
pd = zalloc_drv(ib_pd)
.. which magically calls restrack_init(pd->res)
.. sets res.type
.. etc
restrack_set_user(udata); or restrack_set_kernel(common);
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.
Maybe if we have zalloc_drv we also have a free_drv() that does a
xa_erase, kfree, WARN_ON, etc. That could work OK..
Jason
