Re: [PATCH rdma-next v4] IB: Improve uverbs_cleanup_ucontext algorithm

[Yishai Hadas <yishaih@xxxxxxxxxxxxxxxxxx>]

On 6/28/2018 2:06 AM, Jason Gunthorpe wrote:
> 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.

Makes sense, however this worth a comemnt in place, see below.

> 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) {

This worth a comment why we check explicitly for RDMA_REMOVE_DESTROY and 
not using ib_is_destroy_retryable() as you pointed above.

> +		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.

Typo (double triggered..)

> + */
> +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
>
> --

Other changes look fine to me.

Yishai
--
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