On Wed, Feb 06, 2019 at 11:49:33AM -0700, Jason Gunthorpe wrote:
> 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?
We talked about different location of "valid" checks.
Anyway, I see more clearly now, there you wanted to stick
this xa_cmpxchg() check.
>
> 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).
>
> 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);
I thought that we already agreed on this direction and I'm "guiding my
patches" to this direction. Unfortunately, It will be possible only after
we will convert all sites to use zalloc_drv and cxbg4 posses some
challenges down the road.
>
> 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.
>
> 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..
Yes
>
> Jason
Attachment:
signature.asc
Description: PGP signature
