Re: [PATCH 01/15] RDMA/ucma: Reduce the number of rdma_destroy_id() calls

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Leon Romanovsky <leonro@mellanox.com>,
	Doug Ledford <dledford@redhat.com>,
	linux-rdma@vger.kernel.org
Subject: Re: [PATCH 01/15] RDMA/ucma: Reduce the number of rdma_destroy_id() calls
Date: Tue, 1 Oct 2019 12:07:30 -0300	[thread overview]
Message-ID: <20191001150730.GD22532@ziepe.ca> (raw)
In-Reply-To: <20190930231707.48259-2-bvanassche@acm.org>

On Mon, Sep 30, 2019 at 04:16:53PM -0700, Bart Van Assche wrote:
> Instead of calling rdma_destroy_id() after waiting for the context completion
> finished, call rdma_destroy_id() from inside the ucma_put_ctx() function.
> This patch reduces the number of rdma_destroy_id() calls but does not change
> the behavior of this code.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>  drivers/infiniband/core/ucma.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> index 0274e9b704be..30c09864fd9e 100644
> +++ b/drivers/infiniband/core/ucma.c
> @@ -160,8 +160,14 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id)
>  
>  static void ucma_put_ctx(struct ucma_context *ctx)
>  {
> -	if (atomic_dec_and_test(&ctx->ref))
> -		complete(&ctx->comp);
> +	if (!atomic_dec_and_test(&ctx->ref))
> +		return;
> +	/*
> +	 * rdma_destroy_id() ensures that no event handlers are inflight
> +	 * for that id before releasing it.
> +	 */
> +	rdma_destroy_id(ctx->cm_id);
> +	complete(&ctx->comp);
>  }

Since all the refcounting here is basically insane, you can't do this
without creating new kinds of bugs related to lifetime of ctx->cm_id

The call to rdma_destroy_id must be after the xa_erase as other
threads can continue to access the context despite its zero ref via
ucma_get_ctx(() as ucma_get_ctx() is not using refcounts properly.

The xa_erase provides the needed barrier.

Maybe this patch could be fixed if the ucma_get_ctx used an
atomic_inc_not_zero ?

Jason