Re: [PATCH rdma-next v7 3/8] RDMA/restrack: Add general infrastructure to track RDMA resources

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Sun, Jan 28, 2018 at 02:03:50PM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 28, 2018 at 11:17:20AM +0200, Leon Romanovsky wrote:
> > +int __must_check rdma_restrack_get(struct rdma_restrack_entry *res)
> > +{
> > +	return kref_get_unless_zero(&res->kref);
> > +}
> > +EXPORT_SYMBOL(rdma_restrack_get);
> > +
> > +static void restrack_release(struct kref *kref)
> > +{
> > +	struct rdma_restrack_entry *res;
> > +
> > +	res = container_of(kref, struct rdma_restrack_entry, kref);
> > +	complete(&res->comp);
> > +}
> > +
> > +int rdma_restrack_put(struct rdma_restrack_entry *res)
> > +{
> > +	return kref_put(&res->kref, restrack_release);
> > +}
> > +EXPORT_SYMBOL(rdma_restrack_put);
> > +
> > +void rdma_restrack_del(struct rdma_restrack_entry *res)
> > +{
> > +	struct ib_device *dev;
> > +
> > +	if (!res->valid)
> > +		return;
> > +
> > +	dev = res_to_dev(res);
> > +	if (!dev)
> > +		return;
> > +
> > +	down_read(&dev->res.rwsem);
> > +	rdma_restrack_put(res);
> > +	up_read(&dev->res.rwsem);
>
> I can't see why this lock is necessary, the underlying kref is already
> atomic.

Just to be similar to read implementation.

>
> This locking seems fine, can't see any problem with it.
>
> But I still hate the readability of the kref-as-not-a-kref approach.
>

And I like :)

> Now that you've written it out, it is clear this is actually open
> coding a rw_semaphore??
>
> I think lockdep will be okay with this due to the trylock?
>
> It saves a bit of memory compared to a kref + completion, and has
> better clarity:

Let's put debug option aside,
It saves one "unsigned int done" and only if you didn't
enable CONFIG_RWSEM_SPIN_ON_OWNER, otherwise they are the same.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]