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:
signature.asc
Description: PGP signature
