From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
This wasn't wrong, but the placement of two krefs didn't make any
sense. Follow some simple rules.
- A kref is held inside uobjects_list
- A kref is held inside the IDR
- A kref is held inside file->private
- A stack based kref is passed bettwen alloc_begin and
alloc_abort/alloc_commit
Any place we destroy one of the above pointers, we stick a put,
or 'move' the kref into another pointer.
The key functions have sensible semantics:
- alloc_uobj fully initializes the common members in uobj, including
the list
- Get rid of the uverbs_idr_remove_uobj helper since IDR remove
does require put, but it depends on the situation. Later
patches will re-consolidate this differently.
- alloc_abort always consumes the passed kref, done in the type
- alloc_commit always consumes the passed kref, done in the type
- rdma_remove_commit_uobject always pairs with a lookup_get
After it is all done the only control flow change is to:
- move a get from alloc_commit_fd_uobject to rdma_alloc_commit_uobject
- add a put to remove_commit_idr_uobject
- Consistenly use rdma_lookup_put in rdma_remove_commit_uobject at
the right place
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
---
drivers/infiniband/core/rdma_core.c | 83 +++++++++++++++++------------
1 file changed, 50 insertions(+), 33 deletions(-)
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index afa03d2f68260d..80e1e3cb211071 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -163,6 +163,7 @@ static struct ib_uobject *alloc_uobj(struct ib_uverbs_file *ufile,
*/
uobj->ufile = ufile;
uobj->context = ufile->ucontext;
+ INIT_LIST_HEAD(&uobj->list);
uobj->type = type;
/*
* Allocated objects start out as write locked to deny any other
@@ -198,17 +199,6 @@ static int idr_add_uobj(struct ib_uobject *uobj)
return ret < 0 ? ret : 0;
}
-/*
- * It only removes it from the uobjects list, uverbs_uobject_put() is still
- * required.
- */
-static void uverbs_idr_remove_uobj(struct ib_uobject *uobj)
-{
- spin_lock(&uobj->ufile->idr_lock);
- idr_remove(&uobj->ufile->idr, uobj->id);
- spin_unlock(&uobj->ufile->idr_lock);
-}
-
/* Returns the ib_uobject or an error. The caller should check for IS_ERR. */
static struct ib_uobject *
lookup_get_idr_uobject(const struct uverbs_obj_type *type,
@@ -329,7 +319,9 @@ static struct ib_uobject *alloc_begin_idr_uobject(const struct uverbs_obj_type *
return uobj;
idr_remove:
- uverbs_idr_remove_uobj(uobj);
+ spin_lock(&ufile->idr_lock);
+ idr_remove(&ufile->idr, uobj->id);
+ spin_unlock(&ufile->idr_lock);
uobj_put:
uverbs_uobject_put(uobj);
return ERR_PTR(ret);
@@ -354,6 +346,13 @@ static struct ib_uobject *alloc_begin_fd_uobject(const struct uverbs_obj_type *t
return uobj;
}
+ /*
+ * The kref for uobj is moved into filp->private data and put in
+ * uverbs_close_fd(). Once anon_inode_getfile() succeeds
+ * uverbs_close_fd() must be guaranteed to be called from the provided
+ * fops release callback. We piggyback our kref of uobj on the stack
+ * with the lifetime of the struct file.
+ */
filp = anon_inode_getfile(fd_type->name,
fd_type->fops,
uobj,
@@ -367,7 +366,7 @@ static struct ib_uobject *alloc_begin_fd_uobject(const struct uverbs_obj_type *t
uobj->id = new_fd;
uobj->object = filp;
uobj->ufile = ufile;
- INIT_LIST_HEAD(&uobj->list);
+ /* Matching put will be done in uverbs_close_fd() */
kref_get(&ufile->ref);
return uobj;
@@ -397,7 +396,13 @@ static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
RDMACG_RESOURCE_HCA_OBJECT);
- uverbs_idr_remove_uobj(uobj);
+
+ spin_lock(&uobj->ufile->idr_lock);
+ idr_remove(&uobj->ufile->idr, uobj->id);
+ spin_unlock(&uobj->ufile->idr_lock);
+
+ /* Matches the kref in alloc_commit_idr_uobject */
+ uverbs_uobject_put(uobj);
return ret;
}
@@ -451,24 +456,25 @@ static int __must_check _rdma_remove_commit_uobject(struct ib_uobject *uobj,
return 0;
ret = uobj->type->type_class->remove_commit(uobj, why);
- if (ib_is_destroy_retryable(ret, why, uobj)) {
- /* We couldn't remove the object, so just unlock the uobject */
- atomic_set(&uobj->usecnt, 0);
- uobj->type->type_class->lookup_put(uobj, true);
- } else {
- uobj->object = NULL;
-
- mutex_lock(&ufile->uobjects_lock);
- list_del(&uobj->list);
- mutex_unlock(&ufile->uobjects_lock);
- /* put the ref we took when we created the object */
- uverbs_uobject_put(uobj);
- }
+ if (ib_is_destroy_retryable(ret, why, uobj))
+ return ret;
+
+ uobj->object = NULL;
+
+ mutex_lock(&ufile->uobjects_lock);
+ list_del(&uobj->list);
+ mutex_unlock(&ufile->uobjects_lock);
+ /* Pairs with the get in rdma_alloc_commit_uobject() */
+ uverbs_uobject_put(uobj);
return ret;
}
-/* This is called only for user requested DESTROY reasons */
+/* This is called only for user requested DESTROY reasons
+ * rdma_lookup_get_uobject(exclusive=true) must have been called to get uobj,
+ * and after this returns the corresponding put has been done, and the kref
+ * for uobj has been consumed.
+ */
int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj)
{
int ret;
@@ -519,9 +525,6 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj)
/* This shouldn't be used anymore. Use the file object instead */
uobj->id = 0;
- /* Get another reference as we export this to the fops */
- uverbs_uobject_get(uobj);
-
/*
* NOTE: Once we install the file we loose ownership of our kref on
* uobj. It will be put by uverbs_close_fd()
@@ -554,6 +557,8 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
assert_uverbs_usecnt(uobj, true);
atomic_set(&uobj->usecnt, 0);
+ /* kref is held so long as the uobj is on the uobj list. */
+ uverbs_uobject_get(uobj);
mutex_lock(&ufile->uobjects_lock);
list_add(&uobj->list, &ufile->uobjects);
mutex_unlock(&ufile->uobjects_lock);
@@ -567,12 +572,22 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
static void alloc_abort_idr_uobject(struct ib_uobject *uobj)
{
- uverbs_idr_remove_uobj(uobj);
ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
RDMACG_RESOURCE_HCA_OBJECT);
+
+ spin_lock(&uobj->ufile->idr_lock);
+ /* The value of the handle in the IDR is NULL at this point. */
+ idr_remove(&uobj->ufile->idr, uobj->id);
+ spin_unlock(&uobj->ufile->idr_lock);
+
+ /* Pairs with the kref from alloc_begin_idr_uobject */
uverbs_uobject_put(uobj);
}
+/*
+ * This consumes the kref for uobj. It is up to the caller to unwind the HW
+ * object and anything else connected to uobj before calling this.
+ */
void rdma_alloc_abort_uobject(struct ib_uobject *uobj)
{
uobj->type->type_class->alloc_abort(uobj);
@@ -605,6 +620,7 @@ void rdma_lookup_put_uobject(struct ib_uobject *uobj, bool exclusive)
else
atomic_set(&uobj->usecnt, 0);
+ /* Pairs with the kref obtained by type->lookup_get */
uverbs_uobject_put(uobj);
}
@@ -658,6 +674,7 @@ void uverbs_close_fd(struct file *f)
struct kref *uverbs_file_ref = &uobj->ufile->ref;
_uverbs_close_fd(uobj);
+ /* Pairs with filp->private_data in alloc_begin_fd_uobject */
uverbs_uobject_put(uobj);
kref_put(uverbs_file_ref, ib_uverbs_release_file);
}
@@ -700,7 +717,7 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
obj->id, err);
list_del(&obj->list);
- /* put the ref we took when we created the object */
+ /* Pairs with the get in rdma_alloc_commit_uobject() */
uverbs_uobject_put(obj);
ret = 0;
}
--
2.18.0
--
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
