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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
