Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

On Tue, Oct 27, 2020 at 01:55:08PM -0300, Jason Gunthorpe wrote:

> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 3d366cb79cef42..3ae878f3d173d3 100644
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
>  	if (ret)
>  		return ret;
>  
> +	if (why == RDMA_REMOVE_ABORT)
> +		return 0;
> +
>  	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
>  			   RDMACG_RESOURCE_HCA_OBJECT);
>  
> @@ -727,10 +730,8 @@ void release_ufile_idr_uobject(struct ib_uverbs_file *ufile)
>  	 *
>  	 * This is an optimized equivalent to remove_handle_idr_uobject
>  	 */
> -	xa_for_each(&ufile->idr, id, entry) {
> -		WARN_ON(entry->object);
> +	xa_for_each(&ufile->idr, id, entry)
>  		uverbs_uobject_put(entry);
> -	}

Actually this is not a good idea

This one is better:

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 3d366cb79cef42..fd012be700ccc2 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
 	if (ret)
 		return ret;
 
+	if (why == RDMA_REMOVE_ABORT)
+		return 0;
+
 	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
 			   RDMACG_RESOURCE_HCA_OBJECT);
 
@@ -845,11 +848,17 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
 		 * racing with a lookup_get.
 		 */
 		WARN_ON(uverbs_try_lock_object(obj, UVERBS_LOOKUP_WRITE));
+		if (reason == RDMA_REMOVE_DRIVER_FAILURE)
+			obj->object = NULL;
 		if (!uverbs_destroy_uobject(obj, reason, &attrs))
 			ret = 0;
 		else
 			atomic_set(&obj->usecnt, 0);
 	}
+	if (reason == RDMA_REMOVE_DRIVER_FAILURE) {
+		WARN_ON(!list_empty(&ufile->uobjects));
+		return 0;
+	}
 	return ret;
 }
 
@@ -862,9 +871,6 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
 void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 			     enum rdma_remove_reason reason)
 {
-	struct ib_uobject *obj, *next_obj;
-	unsigned long flags;
-
 	down_write(&ufile->hw_destroy_rwsem);
 
 	/*
@@ -875,25 +881,10 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 		goto done;
 
 	while (!list_empty(&ufile->uobjects))
-		if (__uverbs_cleanup_ufile(ufile, reason)) {
-			/*
-			 * No entry was cleaned-up successfully during this
-			 * iteration. It is a driver bug to fail destruction.
-			 */
-			WARN_ON(!list_empty(&ufile->uobjects));
+		if (__uverbs_cleanup_ufile(ufile, reason))
 			break;
-		}
-
-	list_for_each_entry_safe (obj, next_obj, &ufile->uobjects, list) {
-		spin_lock_irqsave(&ufile->uobjects_lock, flags);
-		list_del_init(&obj->list);
-		spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
-		/*
-		 * Pairs with the get in rdma_alloc_commit_uobject(), could
-		 * destroy uobj.
-		 */
-		uverbs_uobject_put(obj);
-	}
+	if (WARN_ON(!list_empty(&ufile->uobjects)))
+		__uverbs_cleanup_ufile(ufile, RDMA_REMOVE_DRIVER_FAILURE);
 	ufile_destroy_ucontext(ufile, reason);
 
 done:
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index edfc1d7d3766ca..7e330f4a6d33ff 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1471,6 +1471,8 @@ enum rdma_remove_reason {
 	RDMA_REMOVE_DRIVER_REMOVE,
 	/* uobj is being cleaned-up before being committed */
 	RDMA_REMOVE_ABORT,
+	/* The driver failed to destroy the uobject and is being disconnected */
+	RDMA_REMOVE_DRIVER_FAILURE,
 };
 
 struct ib_rdmacg_object {