From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matan Barak Subject: Re: [PATCH V2 for-next 2/7] IB/core: Add support for idr types Date: Thu, 30 Mar 2017 18:42:12 +0300 Message-ID: References: <1489939145-125246-1-git-send-email-matanb@mellanox.com> <1489939145-125246-3-git-send-email-matanb@mellanox.com> <20170329151028.GD2586@obsidianresearch.com> <20170329222744.GA30605@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20170329222744.GA30605-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jason Gunthorpe Cc: Matan Barak , Doug Ledford , linux-rdma , Liran Liss , Sean Hefty , Leon Romanovsky , Majd Dibbiny , Tal Alon , Yishai Hadas , Ira Weiny , Haggai Eran , Christoph Lameter List-Id: linux-rdma@vger.kernel.org On Thu, Mar 30, 2017 at 1:27 AM, Jason Gunthorpe wrote: > 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. > > Yeah, I guess both are ok. I tend to like symmetrical approaches, but I don't have a strong objection to go with needs_kfree_rcu if you really prefer that. >> >> + 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? > Not necessarily. For example, lookup_get could fail because it can't find a valid object. In that case, you have no object to put. However, I'll just put a type->lookup_put in the release path, so we could move that into the shared code. >> 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? > We don't have a release memory functions - we got rid of them awhile ago. However, when you detach a driver based fd object, and the release file_operations of that fd points to driver code. You could get a nasty exception if you try to close that file after unbinding the context because the module was unloaded. > Jason Matan -- 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