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.
> 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;
/* 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..
> 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.
> #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);
> 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.
> 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)
?
> --- 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.
> 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;
>
> enum rdma_remove_reason {
> - /* Userspace requested uobject deletion. Call could fail */
> + /*
> + * Userspace requested uobject deletion or initial try
> + * to remove uobject via cleanup. Call could fail
> + */
> RDMA_REMOVE_DESTROY,
> /* Context deletion. This call should delete the actual object itself */
> RDMA_REMOVE_CLOSE,
> @@ -1503,6 +1506,7 @@ struct ib_ucontext {
> /* protects cleanup process from other actions */
> struct rw_semaphore cleanup_rwsem;
> enum rdma_remove_reason cleanup_reason;
> + bool cleanup_retryable;
Why the spaces after bool? Either line it up properly, or just use one space.
> 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.
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
