Re: [PATCH V2 for-next 2/7] IB/core: Add support for idr types

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Mar 29, 2017 at 10:23:19PM +0300, Matan Barak wrote:
> > A function called lookup_get should also do the get. I think this
> > would look iodmatically better like:
> >
> >         rcu_read_lock();
> >         uobj = idr_find(&ucontext->ufile->idr, id);
> >         if (uobj)
> >                 uverbs_uobject_get(uobj);
> >         rcu_read_unlock();
> >
> > And drop the overlap from rdma_lookup_get_uobject
> >
> 
> Yeah, that could be a nice small refactor. However, it makes
> the needs_rcu asymmetrical. We test this flag at release, but
> we don't force that in the lookup_get. So, that's a trade-off between
> these methods.

I think that is OK. You could change it to needs_kfree_rcu for
clarity.


> >> +     if (uobj->type != type)
> >> +             return ERR_PTR(-EINVAL);
> >> +
> >
> > This should probably be done in rdma_lookup_get_uobject?
> >
> 
> But what happens if you fail? In the fd case, you have to fput the file.
> That's why I preferred this model.

Since it is lookup_get the caller always has to call uobj_put on any
failure, and that does fput for fds. No problem?

> I agree. The only thing that we have to ensure is that the release and
> free functions have
> to be available when they're called. How the module actually does that
> is something we could
> decide when we cross that bridge.

Didn't we get rid of the release function? Is there any callback that
could happen after driver detatch?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html