On 6/19/2018 9:53 PM, Jason Gunthorpe wrote:
On Tue, Jun 19, 2018 at 07:05:35PM +0300, Leon Romanovsky wrote:
From: Yishai Hadas <yishaih@xxxxxxxxxxxx>
Improve uverbs_cleanup_ucontext algorithm to work properly even when
there are two objects from the same type and one points to the other.
For that case to work the 'destroy_order' is not used any more as it's
static per type and can't support this use case.
Instead, the new algorithm iterates over the objects in a LIFO mode as
was before, at the end of each loop if were left objects that couldn't
be destroyed it re-iterates over them give another chance to destroy them
successfully.
If was one iteration that didn't cleanup any object the last iteration
will force the cleanup as was done before this change, this is coming to
prevent memory leaks even in that fatal case.
Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxxxx>
Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
---
drivers/infiniband/core/rdma_core.c | 111 ++++++++++++---------
drivers/infiniband/core/uverbs.h | 2 +-
drivers/infiniband/core/uverbs_cmd.c | 6 +-
drivers/infiniband/core/uverbs_std_types.c | 38 ++++---
.../infiniband/core/uverbs_std_types_counters.c | 4 +-
drivers/infiniband/core/uverbs_std_types_cq.c | 4 +-
drivers/infiniband/core/uverbs_std_types_dm.c | 5 +-
.../infiniband/core/uverbs_std_types_flow_action.c | 4 +-
drivers/infiniband/core/uverbs_std_types_mr.c | 3 +-
include/rdma/ib_verbs.h | 15 ++-
include/rdma/uverbs_types.h | 11 +-
11 files changed, 112 insertions(+), 91 deletions(-)
Since devx is accepted now this will need to be respun to follow the
pattern in devx.c too.
Correct, V4 will set this pattern for devx as well.
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index df3c40533252..52dca36113dc 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -360,9 +360,10 @@ static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
/*
* We can only fail gracefully if the user requested to destroy the
- * object. In the rest of the cases, just remove whatever you can.
+ * object or when a retry may be called upon an error.
+ * In the rest of the cases, just remove whatever you can.
*/
- if (why == RDMA_REMOVE_DESTROY && ret)
+ if (ret && ib_is_remove_retry(why, uobj))
return ret;
I am wondering if this pattern should always read:
if (ib_is_destroy_retryable(ret, why, uobj))
return ret;
I'm fine with that pattern as well.
/* Otherwise proceed to force-destroy as much as possible, core code
will print a warning and leak the resources */
Which is a more regular..
@@ -645,61 +646,73 @@ void uverbs_close_fd(struct file *f)
kref_put(uverbs_file_ref, ib_uverbs_release_file);
}
-void uverbs_cleanup_ucontext(struct ib_ucontext *ucontext, bool device_removed)
+static int __uverbs_cleanup_ucontext(struct ib_ucontext *ucontext,
+ enum rdma_remove_reason reason)
{
- enum rdma_remove_reason reason = device_removed ?
- RDMA_REMOVE_DRIVER_REMOVE : RDMA_REMOVE_CLOSE;
lets keep this hunk instead of the repeated expression..
OK
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 908ee8ab3297..d0226f41f0c7 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -116,6 +116,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
ucontext->tgid = get_task_pid(current->group_leader, PIDTYPE_PID);
rcu_read_unlock();
ucontext->closing = 0;
+ ucontext->cleanup_retryable = false;
Isn't ucontext kzalloc'd? It should be.
The allocation is done by each vendor, IB layer shouldn't rely on each
of to do kzalloc.
This is done for other fields here (e.g. ucontext->closing = 0), better
stay with this.
#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
ucontext->umem_tree = RB_ROOT_CACHED;
@@ -611,12 +612,13 @@ ssize_t ib_uverbs_close_xrcd(struct ib_uverbs_file *file,
return ret ?: in_len;
}
-int ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev,
+int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject,
struct ib_xrcd *xrcd,
enum rdma_remove_reason why)
{
struct inode *inode;
int ret;
+ struct ib_uverbs_device *dev = uobject->context->ufile->device;
inode = xrcd->inode;
if (inode && !atomic_dec_and_test(&xrcd->usecnt))
@@ -624,7 +626,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev,
ret = ib_dealloc_xrcd(xrcd);
- if (why == RDMA_REMOVE_DESTROY && ret)
+ if (ret && ib_is_remove_retry(why, uobject))
atomic_inc(&xrcd->usecnt);
else if (inode)
This would read alot better as
if (ib_is_destroy_retryable(ret, why, uobj)) {
atomic_inc(&xrcd->usecnt);
return ret;
}
if (inode)
xrcd_table_delete(dev, inode);
return 0;
xrcd_table_delete(dev, inode);
OK
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 0df0ac9c1de3..4a468ef0ae72 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -74,7 +74,7 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
container_of(uobject, struct ib_uqp_object, uevent.uobject);
int ret;
- if (why == RDMA_REMOVE_DESTROY) {
+ if (ib_is_remove_retry(why, uobject)) {
if (!list_empty(&uqp->mcast_list))
return -EBUSY;
ugh, similar comment about the else.
OK
diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c
index 03b182a684a6..9aff3798e6fc 100644
--- a/drivers/infiniband/core/uverbs_std_types_counters.c
+++ b/drivers/infiniband/core/uverbs_std_types_counters.c
@@ -39,7 +39,7 @@ static int uverbs_free_counters(struct ib_uobject *uobject,
{
struct ib_counters *counters = uobject->object;
- if (why == RDMA_REMOVE_DESTROY &&
+ if (ib_is_remove_retry(why, uobject) &&
atomic_read(&counters->usecnt))
return -EBUSY;
This is also quite a common pattern.. maybe even add the usecnt:
ib_is_destroy_retryable(ret, why, uobj, &counters->usecnt)
?
This is not a fully common pattern, I prefer leaving this out of
ib_is_destroy_retryable() and consider the 'usecnt' as part of the 'ret'
value when it's applicable.
For the above:
--- a/drivers/infiniband/core/uverbs_std_types_counters.c
+++ b/drivers/infiniband/core/uverbs_std_types_counters.c
@@ -38,10 +38,10 @@ static int uverbs_free_counters(struct ib_uobject
*uobject,
enum rdma_remove_reason why)
{
struct ib_counters *counters = uobject->object;
+ int ret = atomic_read(&counters->usecnt) ? -EBUSY : 0;
- if (why == RDMA_REMOVE_DESTROY &&
- atomic_read(&counters->usecnt))
- return -EBUSY;
+ if (ib_is_destroy_retryable(ret, why, uobject))
+ return ret;
return counters->device->destroy_counters(counters);
}
--- a/drivers/infiniband/core/uverbs_std_types_cq.c
+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
@@ -44,7 +44,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject,
int ret;
ret = ib_destroy_cq(cq);
- if (!ret || why != RDMA_REMOVE_DESTROY)
+ if (!ret || !ib_is_remove_retry(why, uobject))
ib_uverbs_release_ucq(uobject->context->ufile, ev_queue ?
container_of(ev_queue,
struct ib_uverbs_completion_event_file,
The (existing) use of ! in the if is ugly, should be changed.
Will clean it up.
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index dc5d262739e5..1b17fa8a2d86 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1476,7 +1476,10 @@ struct ib_fmr_attr {
struct ib_umem;
struct pid *tgid;
#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
@@ -2684,6 +2688,15 @@ static inline bool ib_is_udata_cleared(struct ib_udata *udata,
return ib_is_buffer_cleared(udata->inbuf + offset, len);
}
+static inline bool ib_is_remove_retry(enum rdma_remove_reason why,
+ struct ib_uobject *uobj)
+{
+ if (why == RDMA_REMOVE_DESTROY || uobj->context->cleanup_retryable)
+ return true;
+
+ return false;
+}
I think a cocinelle script will complain this should just be
return why == RDMA_REMOVE_DESTROY || uobj->context->cleanup_retryable;
Also add a kdoc comment explaining what the expected use is.
Sure, will handle.
--
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
