On Wed, Jun 20, 2018 at 05:11:39PM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index 0df0ac9c1de3..6497263d13c8 100644
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -72,17 +72,16 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
> struct ib_qp *qp = uobject->object;
> struct ib_uqp_object *uqp =
> container_of(uobject, struct ib_uqp_object, uevent.uobject);
> - int ret;
> + int ret = list_empty(&uqp->mcast_list) ? 0 : -EBUSY;
> +
> + if (ib_is_destroy_retryable(ret, why, uobject))
> + return ret;
>
> - if (why == RDMA_REMOVE_DESTROY) {
> - if (!list_empty(&uqp->mcast_list))
> - return -EBUSY;
> - } else if (qp == qp->real_qp) {
> + if (qp == qp->real_qp)
> ib_uverbs_detach_umcast(qp, uqp);
> - }
This is super tricky, but I don't think this change is right.
If and only if we are doing a user triggered destroy (eg
RDMA_REMOVE_DESTROY) should we check the mcast list.
The items on the mcast list are not uobjects, so they will not be
destroyed via the cleanup loop.
If we are doing RDMA_REMOVE_DRIVER_REMOVE or RDMA_REMOVE_CLOSE then we
must always tear down any mcast attachment, free the list and attempt
to destroy the HW object.
ie a driver could legitimately block QP destruction because it has
attached mcast's, which would cause the new iteration scheme to fail.
> @@ -150,13 +155,11 @@ static int uverbs_free_xrcd(struct ib_uobject *uobject,
> struct ib_xrcd *xrcd = uobject->object;
> struct ib_uxrcd_object *uxrcd =
> container_of(uobject, struct ib_uxrcd_object, uobject);
> - int ret;
> + int ret = atomic_read(&uxrcd->refcnt) ? -EBUSY : 0;
I counted 5 of these, and it is a fairly ugly expression.
int ret;
ret = ib_destroy_usecnt(&uxrcd->refcnt, why, uobject);
if (ret)
return ret;
is tidier and clearer.
Anyhow, I made the above edits to this patch, but the diff is a bit
big so I will keep it on the wip branch until I hear from you.
Here is what I edited:
diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index ded6c10e61f2d4..2ddf1c716ba801 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -705,9 +705,13 @@ void uverbs_cleanup_ucontext(struct ib_ucontext *ucontext, bool device_removed)
down_write(&ucontext->cleanup_rwsem);
ucontext->cleanup_retryable = true;
while (!list_empty(&ucontext->uobjects))
- if (__uverbs_cleanup_ucontext(ucontext, reason))
- /* No entry was cleaned-up successfully during this iteration */
+ if (__uverbs_cleanup_ucontext(ucontext, reason)) {
+ /*
+ * No entry was cleaned-up successfully during this
+ * iteration
+ */
break;
+ }
ucontext->cleanup_retryable = false;
if (!list_empty(&ucontext->uobjects))
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 9b4e1e53cd9cdd..a5dbf36284b229 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -75,13 +75,14 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
struct ib_qp *qp = uobject->object;
struct ib_uqp_object *uqp =
container_of(uobject, struct ib_uqp_object, uevent.uobject);
- int ret = list_empty(&uqp->mcast_list) ? 0 : -EBUSY;
-
- if (ib_is_destroy_retryable(ret, why, uobject))
- return ret;
+ int ret;
- if (qp == qp->real_qp)
+ if (why == RDMA_REMOVE_DESTROY) {
+ if (!list_empty(&uqp->mcast_list))
+ return -EBUSY;
+ } else if (qp == qp->real_qp) {
ib_uverbs_detach_umcast(qp, uqp);
+ }
ret = ib_destroy_qp(qp);
if (ib_is_destroy_retryable(ret, why, uobject))
@@ -103,10 +104,9 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
if (ib_is_destroy_retryable(ret, why, uobject))
- goto end;
+ return ret;
kfree(ind_tbl);
-end:
return ret;
}
@@ -120,10 +120,9 @@ static int uverbs_free_wq(struct ib_uobject *uobject,
ret = ib_destroy_wq(wq);
if (ib_is_destroy_retryable(ret, why, uobject))
- goto end;
+ return ret;
ib_uverbs_release_uevent(uobject->context->ufile, &uwq->uevent);
-end:
return ret;
}
@@ -137,7 +136,6 @@ static int uverbs_free_srq(struct ib_uobject *uobject,
int ret;
ret = ib_destroy_srq(srq);
-
if (ib_is_destroy_retryable(ret, why, uobject))
return ret;
@@ -158,12 +156,14 @@ static int uverbs_free_xrcd(struct ib_uobject *uobject,
struct ib_xrcd *xrcd = uobject->object;
struct ib_uxrcd_object *uxrcd =
container_of(uobject, struct ib_uxrcd_object, uobject);
- int ret = atomic_read(&uxrcd->refcnt) ? -EBUSY : 0;
+ int ret;
+
+ ret = ib_destroy_usecnt(&uxrcd->refcnt, why, uobject);
+ if (ret)
+ return ret;
mutex_lock(&uobject->context->ufile->device->xrcd_tree_mutex);
- if (!ib_is_destroy_retryable(ret, why, uobject))
- ret = ib_uverbs_dealloc_xrcd(uobject,
- xrcd, why);
+ ret = ib_uverbs_dealloc_xrcd(uobject, xrcd, why);
mutex_unlock(&uobject->context->ufile->device->xrcd_tree_mutex);
return ret;
@@ -173,12 +173,14 @@ static int uverbs_free_pd(struct ib_uobject *uobject,
enum rdma_remove_reason why)
{
struct ib_pd *pd = uobject->object;
- int ret = atomic_read(&pd->usecnt) ? -EBUSY : 0;
+ int ret;
- if (!ib_is_destroy_retryable(ret, why, uobject))
- ib_dealloc_pd((struct ib_pd *)uobject->object);
+ ret = ib_destroy_usecnt(&pd->usecnt, why, uobject);
+ if (ret)
+ return ret;
- return ret;
+ ib_dealloc_pd((struct ib_pd *)uobject->object);
+ return 0;
}
static int uverbs_hot_unplug_completion_event_file(struct ib_uobject_file *uobj_file,
diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c
index aad95627e89626..6d0b1ce9fc1f94 100644
--- a/drivers/infiniband/core/uverbs_std_types_counters.c
+++ b/drivers/infiniband/core/uverbs_std_types_counters.c
@@ -38,9 +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;
+ int ret;
- if (ib_is_destroy_retryable(ret, why, uobject))
+ ret = ib_destroy_usecnt(&counters->usecnt, why, uobject);
+ if (ret)
return ret;
return counters->device->destroy_counters(counters);
diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
index 84fdef624a2647..f67b0895b48c35 100644
--- a/drivers/infiniband/core/uverbs_std_types_cq.c
+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
@@ -47,11 +47,13 @@ static int uverbs_free_cq(struct ib_uobject *uobject,
if (ib_is_destroy_retryable(ret, why, uobject))
return ret;
- ib_uverbs_release_ucq(uobject->context->ufile, ev_queue ?
- container_of(ev_queue,
- struct ib_uverbs_completion_event_file,
- ev_queue) : NULL,
- ucq);
+ ib_uverbs_release_ucq(
+ uobject->context->ufile,
+ ev_queue ? container_of(ev_queue,
+ struct ib_uverbs_completion_event_file,
+ ev_queue) :
+ NULL,
+ ucq);
return ret;
}
diff --git a/drivers/infiniband/core/uverbs_std_types_dm.c b/drivers/infiniband/core/uverbs_std_types_dm.c
index 550ced12d037c6..d294660a2e0674 100644
--- a/drivers/infiniband/core/uverbs_std_types_dm.c
+++ b/drivers/infiniband/core/uverbs_std_types_dm.c
@@ -37,9 +37,10 @@ static int uverbs_free_dm(struct ib_uobject *uobject,
enum rdma_remove_reason why)
{
struct ib_dm *dm = uobject->object;
- int ret = atomic_read(&dm->usecnt) ? -EBUSY : 0;
+ int ret;
- if (ib_is_destroy_retryable(ret, why, uobject))
+ ret = ib_destroy_usecnt(&dm->usecnt, why, uobject);
+ if (ret)
return ret;
return dm->device->dealloc_dm(dm);
diff --git a/drivers/infiniband/core/uverbs_std_types_flow_action.c b/drivers/infiniband/core/uverbs_std_types_flow_action.c
index 31c56cee6b7631..c1875657bc993f 100644
--- a/drivers/infiniband/core/uverbs_std_types_flow_action.c
+++ b/drivers/infiniband/core/uverbs_std_types_flow_action.c
@@ -37,9 +37,10 @@ static int uverbs_free_flow_action(struct ib_uobject *uobject,
enum rdma_remove_reason why)
{
struct ib_flow_action *action = uobject->object;
- int ret = atomic_read(&action->usecnt) ? -EBUSY : 0;
+ int ret;
- if (ib_is_destroy_retryable(ret, why, uobject))
+ ret = ib_destroy_usecnt(&action->usecnt, why, uobject);
+ if (ret)
return ret;
return action->device->destroy_flow_action(action);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6c51190ae7a1f1..026ebc87202979 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2697,7 +2697,7 @@ static inline bool ib_is_udata_cleared(struct ib_udata *udata,
*
* This function is a helper function that IB layer and low-level drivers
* can use to consider whether the destruction of the given uobject is
- * retrayable.
+ * retry-able.
* It checks the original return code, if it wasn't success the destruction
* is retryable according to the ucontext state (i.e. cleanup_retryable) and
* the remove reason. (i.e. why).
@@ -2709,6 +2709,24 @@ static inline bool ib_is_destroy_retryable(int ret, enum rdma_remove_reason why,
uobj->context->cleanup_retryable);
}
+/**
+ * ib_destroy_usecnt - Called during destruction to check the usecnt
+ * @usecnt: The usecnt atomic
+ * @why: remove reason
+ * @uobj: The uobject that is destroyed
+ *
+ * Non-zero usecnts will block destruction unless destruction was triggered
+ * triggered by a ucontext cleanup.
+ */
+static inline int ib_destroy_usecnt(atomic_t *usecnt,
+ enum rdma_remove_reason why,
+ struct ib_uobject *uobj)
+{
+ if (atomic_read(usecnt) && ib_is_destroy_retryable(-EBUSY, why, uobj))
+ return -EBUSY;
+ return 0;
+}
+
/**
* ib_modify_qp_is_ok - Check that the supplied attribute mask
* contains all required attributes and no attributes not allowed for
--
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
