On Thu, Mar 30, 2017 at 7:12 PM, Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote: > On Thu, Mar 30, 2017 at 06:47:02PM +0300, Matan Barak wrote: > >> The problem with that is that get_read also locks the object for >> reading (so if you try to lock it for write or destroy it, you'll >> get a locking error). So, you want to increase the reference count >> to ensure it exists in the memory, but unlock it. > > Okay, I see. I've looked at the whole thing more closely and there are > more places that should be holding krefs, so I think you should make a > function specifically for this pattern. > > /* Convert a locked reference obtained from rdma_lookup_get_uobject to > a simple kref by dropping the read/write lock. The caller must pair > this with uverbs_uobject_put */ > void rdma_lookup_to_kref(struct ib_uobject *uobj, bool write); > > Similarly I think you should add a uobj_remove_commit_kref() wrapper > which does remove_commit but leaves the caller with a kref, as that > pattern seems to come up alot in this code as well. > Actually, this isn't that frequent pattern. It mainly happens in ib_uverbs_lookup_comp_file. There's a different pattern of inc_kref and then remove_commit which is used in order to copy the latest events_reported info before releasing the uobject's memory. > Looking at things after this series is applied, there are two more > areas that need some more work in future patches before adding the new > uapi stuff.. > > usecnt is confused, the uapi part makes sense, we must hold uobj read > when doing atomic_inc and uobj write when doing atomic_dec. However > the kapi does something entirely different. Maybe we should delete > usecnt from the kapi side? > > Lots of places that incr usecnt fail to hold the kref: > > pd = uobj_get_obj_read(pd, cmd.pd_handle, file->ucontext); > > mr->device = pd->device; > mr->pd = pd; > mr->uobject = uobj; > atomic_inc(&pd->usecnt); > > uobj_put_obj_read(pd); > > It really should be doing rdma_lookup_to_kref(pd) and have the proper > put in the destroy of the pd uobj. Many other places like this as > well. > There is a usecnt in the uobject, which is used to count the concurrent readers or indicate if the uobject is write locked. Most objects (as opposed to uobjects) has usecnt variable to count how many objects actually use them. For example, in the reg_mr handler, we inc the object's usecnt right after assigning it (as you mentioned). The object's destroy function should test the usecnt and make sure no one uses it before destroying it. When this destroy function comes from the user-space, the pd object is locked, so this test would be atomic and safe. The uobject won't be destroyed, until the object itself is destroyed. So effectively, it's like taking another kref :) When you come from the kernel's context, the ULP has to make sure you aren't trying to destroy an object while using it in creation of another object. In context teardown, we rely on order, so this should be safe as well. The verbs layer itself (drivers/infiniband/verbs.c) increase the object's pd as well when needed (for example, ib_create_ah). I haven't looked at each of the handler, but I haven't changed this code too, so I guess this has nothing to do with this series. When we introduce the ioctl handlers, we'll of course need to make sure we increase the refcounts on objects correctly. > Jason Matan -- 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
