[rdma-next,v3] IB/core: Improve uverbs_cleanup_ucontext algorithm

Commit Message

Yishai Hadas June 20, 2018, 1:21 p.m. UTC

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Leon Romanovsky <[e-mail]>
>> ---
>>   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_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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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